Received-SPF: pass (google.com: domain of linux-kernel+bounces-13417-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20231030-fix-rtl8366rb-v2-1-e66e1ef7dbd2@linaro.org>
 <20231030141623.ufzhb4ttvxi3ukbj@skbuf> <CACRpkdaN2rTSHXDxwuS4czCzWyUkazY4Fn5vVLYosqF0=qi-Bw@mail.gmail.com>
 <20231030222035.oqos7v7sdq5u6mti@skbuf> <CACRpkdZ4+QrSA0+JCOrx_OZs4gzt1zx1kPK5bdqxp0AHfEQY3g@mail.gmail.com>
 <20231030233334.jcd5dnojruo57hfk@skbuf> <CACRpkdbLTNVJusuCw2hrHDzx5odw8vw8hMWvvvvgEPsAFwB8hg@mail.gmail.com>
 <CAJq09z4+3g7-h5asYPs_3g4e9NbPnxZQK+NxggYXGGxO+oHU1g@mail.gmail.com>
 <CACRpkdZ-M5mSUeVNhdahQRpm+oA1zfFkq6kZEbpp=3sKjdV9jA@mail.gmail.com>
 <CAJq09z6QwLNEc5rEGvE3jujZ-vb+vtUQLS-fkOnrdnYqk5KvxA@mail.gmail.com>
 <CACRpkdaoBo0S0RgLhacObd3pbjtWAfr6s3oizQAHqdB76gaG5A@mail.gmail.com> <CAJq09z4YSGyU6QuZL1uEB9vH39-WbR2dZhy7MiD=5yZb0Urz1Q@mail.gmail.com>
In-Reply-To: <CAJq09z4YSGyU6QuZL1uEB9vH39-WbR2dZhy7MiD=5yZb0Urz1Q@mail.gmail.com>
From: Linus Walleij <linus.walleij@linaro.org>
Date: Sat, 30 Dec 2023 13:28:54 +0100
Message-ID: <CACRpkdZOKz-DdZgwwxj9FsJZ+GNMCXUjTDLo5wVgjw5OrfOZQA@mail.gmail.com>
Subject: Re: [PATCH net v2] net: dsa: tag_rtl4_a: Bump min packet size
To: Luiz Angelo Daros de Luca <luizluca@gmail.com>, Ansuel Smith <ansuelsmth@gmail.com>, 
	Andrew Lunn <andrew@lunn.ch>
Cc: Vladimir Oltean <olteanv@gmail.com>, Florian Fainelli <f.fainelli@gmail.com>, 
	"David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, 
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Dec 30, 2023 at 6:19=E2=80=AFAM Luiz Angelo Daros de Luca
<luizluca@gmail.com> wrote:

> I took a look at the LED code. It looks like you got it a little bit wron=
g.

You are right...

> If LEDs are not disabled, it will use the RTL8366RB_LED_FORCE for all
> 4 LED groups. That RTL8366RB_LED_FORCE keeps the LEDs on. I would use
> RTL8366RB_LED_LINK_ACT by default to make it blink on link activity
> (or make it configurable as the comment suggests) but it is not wrong.
> I cannot evaluate the RTL8366RB_INTERRUPT_CONTROL_REG usage when you
> disable the LEDs but it seems to be odd.

The problem is that since I don't have a device with LEDs connected
to tHE RTL8366RB it is all just dry coding.

I would suggest if you can test it just make a basic patch that will
at least turn on the LEDs to some default setting that works for
you?

> I though that maybe we could setup a LED driver to expose the LEDs
> status in sysfs. However, I'm not sure it is worth it. If you change a
> LED behavior, it would break the HW triggering rule for all the group.
> I'm not sure the LED API is ready to expose LEDs with related fate. It
> would, indeed, be useful as a readonly source or just to
> enable/disable a LED.

The LED subsystem supports hardware triggering etc thanks to the
elaborate work by Christian (ansuel). You can see an example of how
this is done in:
drivers/net/dsa/qca/qca8k-leds.c

Christian also extended the LEDs subsystem with the necessary
callbacks to support HW-backed LED control.

This can be used already to achieve HW triggers for the LEDs
from sysfs. (See callbacks .hw_control_is_supported,
.hw_control_set etc etc).

I was working to implement this for the Marvell switches but Andrew
wanted to do some more structured approach with a LED library
for DSA switches.

Yours,
Linus Walleij