On Thu, Sep 09, 2021 at 08:54:28AM -0500, Samuel Holland wrote:

On 9/9/21 6:36 AM, Maxime Ripard wrote:

quoted

Hi,

On Sun, Sep 05, 2021 at 11:17:19PM -0500, Samuel Holland wrote:

quoted

Hi,

On 9/3/21 5:36 AM, Maxime Ripard wrote:

quoted

On Thu, Sep 02, 2021 at 06:42:28PM -0500, Samuel Holland wrote:

quoted

Some Allwinner sunxi SoCs, starting with the R329, contain an LED
controller designed to drive RGB LED pixels. Add a driver for it using
the multicolor LED framework, and with LEDs defined in the device tree.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/leds/Kconfig      |   8 +
 drivers/leds/Makefile     |   1 +
 drivers/leds/leds-sunxi.c | 562 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 571 insertions(+)
 create mode 100644 drivers/leds/leds-sunxi.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index ed800f5da7d8..559d2ca0a7f4 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig

@@ -297,6 +297,14 @@ config LEDS_SUNFIRE
 	  This option enables support for the Left, Middle, and Right
 	  LEDs on the I/O and CPU boards of SunFire UltraSPARC servers.
 
+config LEDS_SUNXI
+	tristate "LED support for Allwinner sunxi LED controller"
+	depends on LEDS_CLASS
+	depends on ARCH_SUNXI || COMPILE_TEST
+	help
+	  This option enables support for the LED controller provided in
+	  some Allwinner sunxi SoCs.
+

Same comment for the name

Are you concerned about the help text only, or do you also want me to rename the
Kconfig symbol?

The driver, the driver symbols and the Kconfig symbol would be nice

quoted

I am happy to change the help text to something like: "This option enables
support for the LED controller provided in the Allwinner R329 and D1 SoCs."

But I don't know of any satisfying way to rename the Kconfig symbol. There is no
general category name for "R329 and D1."

Yeah, this is not ideal, but the issue is that nothing is telling us
whether or not it will support *only* the R329 and D1. Chances are it's
going to be featured in a number of other SoCs in the future, so if we
were to have the entire list of supported SoCs in the Kconfig symbol and
driver name, we'd have to always change them everytime a new SoC support
is introduced.

This is why I named it LEDS_SUNXI: until and unless a "v2" hardware
block shows up, this is the LED controller in all sunxi SoCs [that have
an LED controller at all]. And at that point, naming the new driver
LEDS_SUNXI_V2 makes more sense to me than trying to guess the pattern
for SoC support, where there likely is no pattern.

sunxi and sunxi_v2 doesn't mean anything though. The original A10 is
sunxi, yet it's very far from being supported by that driver. At least
putting the first SoC name has some sort of relationship to the hardware
supported.

quoted

It would be a pain, and it's pretty much guaranteed that someone is
going to forget at some point. To mitigate this, we took the approach to
use the same semantic than the DT compatible: the driver name doesn't
really define the list of all the SoCs supported but matches every SoC
(more or less) compatible with that SoC.

Ok, but this still doesn't tell me what you expect the driver to be
named. Again, there is no general name for "every SoC (more or less)
compatible with R329".

leds-sun50i-r329?

We tried to guess the pattern around the time H6 came out, and named a
bunch of things "sun50i" (like the IOMMU driver) that are now showing up
on sun8i (A50) and sun20i (D1) SoCs.

I'm not sure what's wrong with that? We name and sort things in
chronological order, and there's new SoCs from multiple families in
parallel.

And it turns out most of the changes were not even new for H6
(sun50iw6), but already present in the A63 (sun50iw3).

That's indeed a bit unfortunate, but it's not worth changing at this
point.

I'm in sort of the same situation here. I know the hardware exists on
the R329 (sun50iw11); and from looking at the A100 (sun50iw10) pinctrl
driver, I know some LED controller exists there as well. But I don't
have a manual for the A100 to verify that LED controller is compatible.
So I don't even know if R329 is the first supported SoC.

Let's just stick to something we know for sure and name it after the
r329

To be clear: do you want me to name the driver "sun50i_r329_ledc"?

Yes

Maxime