Re: [PATCH RFC 3/3] leds: Add driver for the ISSI IS31FL32xx family of LED drivers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 03/01/2016 07:45 PM, David Rivshin (Allworx) wrote:
On Tue, 01 Mar 2016 09:24:59 +0100
Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx> wrote:

On 02/29/2016 07:26 PM, David Rivshin (Allworx) wrote:
On Mon, 29 Feb 2016 10:47:44 +0100
Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx> wrote:

On 02/26/2016 10:58 PM, David Rivshin (Allworx) wrote:
On Fri, 26 Feb 2016 10:47:46 +0100
Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx> wrote:

On 02/25/2016 08:12 PM, David Rivshin (Allworx) wrote:
On Thu, 25 Feb 2016 11:55:58 +0100
Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx> wrote:

On 02/25/2016 03:24 AM, David Rivshin (Allworx) wrote:
On Wed, 24 Feb 2016 17:04:58 +0100
Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx> wrote:

Hi David,

Thanks for the patch. Very nice driver. I have few comments
below.

Thanks Jacek, I have responded the comments inline. I also wanted to
double check whether you noticed some questions I had in the cover
letter [1]. As I mentioned in another email to Rob, in hindsight I'm
guessing I should have included them in the patch comments as well (or
instead of).

I saw them. I assumed that the review itself will address those
questions.

Fair enough, thanks for the confirmation.

Your review comments here effectively answered some of the questions, but
the big one I'm still unsure of is whether it actually makes sense to
have all 4 of these devices supported by a single driver.

It's perfectly fine. Many drivers implement this pattern.

OK, then I'll assume you think this driver is not yet too complicated
for it's own good. Out of curiosity, might that view change if the
3216 specific features were ever implemented, especially GPIO and HW
animation support? Gut feel is that would make 3216 specific code
bigger than the rest of the code combined.

I don't think so.

Thanks, that helps calibrate my intuition for the future.

Bigger question is what should be done in terms of the overlap in device
support between this driver and leds-sn3218? If you think I should leave
the *3218 support in this driver, then I would propose:
      - remove leds-sn3218 and its separate binding doc
      - add the "si-en,sn3218" compatible string to this driver and binding doc
Note that while I expect this driver to work with the 3218 chips, I do
not have one to test against. If we go down this route I would definitely
want Stefan to test so that I don't accidentally break him.

I'd prefer to have a single driver for the same hardware. Stefan, would
it be possible for you to test David's driver with the hardware you
have an access to?

Stefan, one thing to note: the existing sn3218 driver/binding uses 0-based
'reg' values, and this driver/binding uses 1-based 'reg' values. So your
devicetree(s) would need to be updated for that (as well as the compatible
string).

Actually, If your driver can successfully handle Si-En SN3218 I'd prefer
to drop leds-sn3218 along with its bindings and add related compatible
to your bindings documentation.

Agreed. The changing of compatible string would only need to be done with
the current version of the series.
In the next version I'll add a 4th patch (unless you'd prefer a separate
patch not part of the series?) that removes leds-sn3218 and moves that
support into the is31fl32xx driver.

Please add it as 4th patch to this set.

OK.

I didn't see a final answer from Rob as to which way is most appropriate
for these devices yet, so I don't know which way this will end up in the
final patch.

Also I feel I should point out some differences between the 3218 support
in this driver versus the leds-sn3218 driver, in case they have any
impact:
      - (as previously mentioned) leds-sn3218 turns off an LEDs enable
        bit if the brightness is set to 0. This driver just sets the PWM
        to 0 and leaves the enable bits always on.

Setting brightness to 0 is an equivalent to turning the device in
a power down mode or at least in the state where the current consumption
is as low as possible. A hardware configuration that is most fitting
for this requirements should be chosen.

As far as I can tell from the datasheets, setting the PWM duty cycle for
a given channel to 0 should have the same net effect as setting the enable
bit of that channel to 0. I assume the purpose of the enable bits is to
make it easier to turn an LED on/off without adjusting the PWM duty cycle,
but just using always the PWM duty cycle register conveniently maps to
the leds API.

ack.

      - leds-sn3218 uses a regmap, I think mostly to deal with the enable
        bits, but it also has the benefit of showing up in debugfs. This
        could be seen as useful in and of itself by some users. On the other
        hand regmap introduces another mutex on every write.

I will not insist on using regmap if you don't refer to the current
state of hw registers in your driver.

Currently I have not had a need to refer to the current state of any HW
registers. I could imagine that might be needed in the future if extra
functionality is implemented, but it wasn't so far.

Regmap exposes nice debugfs interface, so this, and the fact that there
are uncovered hw features, can be thought of as a sufficient
argument in favour of using regmap even now. But it's up to you.

If it's OK with you, I think I'll leave it without regmap for now. I
don't really relish the thought of having 4 large blocks of reg_default
(especially the 3216 has a large register set for animation), and I
haven't yet worked out how/if I could dynamically generate them from
the chipdefs in a reasonable way.

I'm OK with it.

      - leds-sn3218 implements the shutdown callback. Actually, I think I
        should add that to this driver in any event.

Do you see use cases or hardware configurations that need such
a callback? I didn't oppose in case of Stefan's driver, but if we are
at it, we can consult Stefan if he saw that use case?

I'd say that shutdown op is usually useful when CPU and LED
controller are powered from different sources, which can result in
a situation when CPU is turned off, but LED remains on.

That is exactly what happens today on my board: if the system is rebooted
or shut down the LEDs all stay in whatever state they were last in. This
could also be handled by userspace shutdown scripts easily enough, but I
thought it surprising when the system reboots but LEDs stay on.

That's the shutdown callback is for.

I'll add a shutdown callback that will do the same as the remove callback,
and ensure all LEDs go dark.
Is there anything verboten with having one just call the other, or just
registering the same function for both callbacks?

Please enclose current content of your remove callback in a new
function, and call this function from both remove and shutdown.

OK.

On the
other hand someone might have a good reason to want to leave an LED on
through a reboot.

If you have such a use case, then you can add DT property for this.
E.g. retain-state-on-shutdown.

I don't have such a use case right now, but noted for the future.

There is also an inconsistency on what happens during remove() vs shutdown().
In led_classdev_unregister() brightness is set to LED_OFF, so that happens
for all drivers on remove(). But only some drivers implement a shutdown()
which also turns off LEDs, most do not. For instance, leds-gpio turns off
all LEDs, but leds-pwm does not.
   >
Is the general policy that LEDs should be forced off by the driver when the
kernel halts or reboots the CPU? Or left alone and let userspace deal with
it?

I think that this depends on use cases and hardware configurations
available on the market. People added the callback when they needed it.
There is no defined policy for this.

And should this (in principle) be the same as what happens when a module
is unloaded (which currently always turns LEDs off)?

Turning the LED off on removal is logically justified IMO.

Agreed. I just found the inconsistency in some/most drivers between remove
and shutdown unexpected. Given that not all LED drivers turn off on shutdown,
(and my use case desires them to turn off, including a leds-pwm instance),
I'll just write 0 to /sys/class/leds/*/brightness unconditionally in a
userspace shutdown script.

Doesn't it stand in contradiction with your above statement?:

"I'll add a shutdown callback that will do the same as the remove callback,"

I'm a bit lost - does your current is31fl32xx_remove() turn all
the sub-LEDs off?

Yes, is31fl32xx_remove (and soon is31fl32xx_shutdown) will turn all the
LED channels off. However, I also have a leds-pwm instance and that does
not turn off LEDs on shutdown(), so during a reboot those LEDs currently
left on. Also, it's always possible that future boards might use a
different device (and therefore driver) to drive LEDs. Very few LED
existing drivers have a shutdown callback, so I expect them to have the
same behavior as leds-pwm.

For obvious reasons I'd rather not have userspace know what LEDs are
controlled through what specific driver (and what drivers do/don't
turn off LEDs on their own). So if the kernel does not guarantee that
all LEDs are turned off on a reboot, it's cleanest from a userspace
perspective to just turn all LEDs off unconditionally via the sysfs
interface.

Unless I misunderstood your earlier statement:
     "There is no defined policy for this."
? If you consider the current leds-pwm behavior a bug (as opposed to a
choice), then I might submit a patch to add a shutdown callback for
leds-pwm instead. Although I could imagine someone complaining about
such a change being backwards-incompatible if they were somehow relying
on current behavior. And there is still the vast majority of other LED
drivers which I think likely have the same basic issue, though I can't
test those to verify.

Thanks for the explanation. Your analysis makes me leaning towards
considering shutdown callback not fitting for LED class devices.
It imposes the shutdown policy, which may be not fitting for all
hardware configurations.

Please don't implement shutdown callback.

      - leds-sn3218 just puts the chip in software-shutdown mode on remove/
        shutdown. This driver uses the reset register to put the device in
        poweron state, and software-shutdown is part of the poweron state.
        Only difference would be if the next code to use the device does
        not do it's own full initialization (which seems unlikely, or at
        least unwise), but instead just clears software-shutdown.

I believe that my above explanations address this question, i.e.
both brightness = 0 an remove/shutdown should set the device
in a power down mode.

I think there is some confusion, there are 3 separate controls:
    - per-LED PWM duty cycle
    - per-LED enable bit
    - device-wide shutdown mode
Shutdown-mode results in all LEDs going dark (regardless of any other
register state), and I think implies that it also uses less power
(compared to just turning them all off with either of the other controls).
Registers do retain their value and the I2C interface continues to work.
I suspect that all it does is turn off an internal oscillator that drives
the PWM circuits, but the documentation is not clear.

The distinction I was making was that the leds-sn3218 driver *only* turned
on the Shutdown-mode, while this driver reset all other registers in the
device to their default values as well. Though in practice I don't expect
that to make a difference.

If documentation isn't clear about that, you can always measure current
consumption in both cases. Note, that this is not required, you can
follow your intuition.

I may try to do that out of curiosity.

I won't
clutter this email with a duplicate of the details (it's somewhat long),
but if you could check the cover letter and give some guidance, I would
appreciate it.

[1] http://www.spinics.net/lists/linux-leds/msg05564.html
         http://thread.gmane.org/gmane.linux.leds/4530


On 02/23/2016 07:17 PM, David Rivshin (Allworx) wrote:
From: David Rivshin <drivshin@xxxxxxxxxxx>

The IS31FL32xx family of LED drivers are I2C devices with multiple
constant-current channels, each with independent 256-level PWM control.

HW Docs: http://www.issi.com/US/product-analog-fxled-driver.shtml

This has been tested on the IS31FL3236 and IS31FL3216 on an ARM
(TI am335x) platform.

The programming paradigm of these devices is similar in the following
ways:
       - All registers are 8 bit
       - All LED control registers are write-only
       - Each LED channel has a PWM register (0-255)
       - PWM register writes are shadowed until an Update register is poked
       - All have a concept of Software Shutdown, which disables output

However, there are some differences in devices:
       - 3236/3235 have a separate Control register for each LED,
         (3218/3216 pack the enable bits into fewer registers)
       - 3236/3235 have a per-channel current divisor setting
       - 3236/3235 have a Global Control register that can turn off all LEDs
       - 3216 is unique in a number of ways
          - OUT9-OUT16 can be configured as GPIOs instead of LED controls
          - LEDs can be programmed with an 8-frame animation, with
            programmable delay between frames
          - LEDs can be modulated by an input audio signal
          - Max output current can be adjusted from 1/4 to 2x globally
          - Has a Configuration register instead of a Shutdown register

This driver currently only supports the base PWM control function
of these devices. The following features of these devices are not
implemented, although it should be possible to add them in the future:
       - All devices are capable of going into a lower-power "software
         shutdown" mode.
       - The is31fl3236 and is31fl3235 can reduce the max output current
         per-channel with a divisor of 1, 2, 3, or 4.
       - The is31fl3216 can use some LED channels as GPIOs instead.
       - The is31fl3216 can animate LEDs in hardware.
       - The is31fl3216 can modulate LEDs according to an audio input.
       - The is31fl3216 can reduce/increase max output current globally.

Signed-off-by: David Rivshin <drivshin@xxxxxxxxxxx>
---
       drivers/leds/Kconfig           |   9 +
       drivers/leds/Makefile          |   1 +
       drivers/leds/leds-is31fl32xx.c | 442 +++++++++++++++++++++++++++++++++++++++++
       3 files changed, 452 insertions(+)
       create mode 100644 drivers/leds/leds-is31fl32xx.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1034696..8f6c46f 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -580,6 +580,15 @@ config LEDS_SN3218
       	  This driver can also be built as a module. If so the module
       	  will be called leds-sn3218.

+config LEDS_IS31FL32XX
+	tristate "Driver for ISSI IS31FL32XX I2C LED driver chip family"

2 x "[Dd]river".

How about:

"LED Support for ISSI IS31FL32XX I2C LED chip family" ?

Yes, I found that awkward as well. HW folks (and the datasheets) seem
always refer to devices of this type as "LED Driver"s (which can lead
to some interesting confusions). Taking a cue from the LP5521/23/62
entries, how about:
      "LED Support for the ISSI IS31FL32XX I2C LED driver chip family" ?

"LED Support" means "LED class driver". Driver is a software support
for hardware chip. What discrepancy do you see in the description
I proposed?

I think in this case "driver" also means "hardware device which drives
a physical LED".

Let's not confuse these notions. From Linux perspective "driver" refers
to a piece of software used for controlling a hardware.

It seems that "LED driver" is the term universally used
to describe this type of HW device in datasheets.

There are also e.g. "LED controllers", "LED current regulators".
Let's stick to the convention predominantly used in the LED subsystem
kernel config menu.

So it seemed useful to
use exactly that phrase in the description of what hardware this software
supports. I could see someone interpreting the phrase "LED chip" as
referring to an actual LED device.
I don't feel very strongly on this topic, but for the sake of discussion,
maybe "LED controller" would avoid any possible confusion in both
directions?

Right, so let's use the following:

"LED Support for ISSI IS31FL32XX I2C LED controller family"

I understand "LED Support" as "Linux LED subsystem support".

STGM. Done.

Perhaps that's the best of both worlds?

+	depends on LEDS_CLASS && I2C && OF
+	help
+	  Say Y here to include support for the ISSI 31FL32XX LED driver family.

s/driver/chip/

+	  They are I2C devices with multiple constant-current channels, each
+	  with independent 256-level PWM control. This will only work with
+	  device tree enabled devices.

We can skip the last sentence I think.

OK. FYI, I think I got that verbiage from LEDS_SYSCON.

Having "depends on OF" is self-explanatory here.

Noted.

+
       comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"

       config LEDS_BLINKM
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 89c9b6f..3fdf313 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_LEDS_KTD2692)		+= leds-ktd2692.o
       obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
       obj-$(CONFIG_LEDS_SEAD3)		+= leds-sead3.o
       obj-$(CONFIG_LEDS_SN3218)		+= leds-sn3218.o
+obj-$(CONFIG_LEDS_IS31FL32XX)		+= leds-is31fl32xx.o

       # LED SPI Drivers
       obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
new file mode 100644
index 0000000..8dea518
--- /dev/null
+++ b/drivers/leds/leds-is31fl32xx.c
@@ -0,0 +1,442 @@
+/*
+ * linux/drivers/leds-is31fl32xx.c
+ *
+ * Driver for ISSI IS31FL32xx family of I2C LED controllers
+ *
+ * Copyright 2015 Allworx Corp.
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * HW Docs: http://www.issi.com/US/product-analog-fxled-driver.shtml
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+
+#ifdef DEBUG
+ #undef dev_dbg
+ #define dev_dbg dev_info
+#endif

What's the benefit of the above?

It gave me a way to easily see debug output from the driver while it
was parsing the DT (especially if the driver was built-in). Early on
there were other things within that #ifdef as well.
Regardless, passing ddebug_query on the kernel commandline is a more
appropriate way of accomplishing that; I'll remove for the next version.

Thanks.

+/* Used to indicate a device has no such register */
+#define IS31FL32XX_REG_NONE 0xFF
+
+#define IS31FL3216_CONFIG_REG 0x00
+#define IS31FL3216_LIGHTING_EFFECT_REG 0x03
+#define IS31FL3216_CHANNEL_CONFIG_REG 0x04
+
+struct is31fl32xx_priv;
+struct is31fl32xx_led_data {
+	struct led_classdev cdev;
+	u8 channel; /* 1-based, max priv->cdef->channels */
+	struct is31fl32xx_priv *priv;
+};
+
+struct is31fl32xx_priv {
+	const struct is31fl32xx_chipdef *cdef;
+	struct i2c_client *client;
+	unsigned int num_leds;
+	struct is31fl32xx_led_data leds[0];

Is there any specific reason for not having *leds here instead?

I followed a pattern from leds-pwm where it did a single allocation
for both priv and priv->leds[]. See sizeof_is31fl32xx_priv(), and
its use, below. I saw the benefit as one fewer small allocation, so
slightly more kind to the allocator (and devres). If you'd prefer to
do it as two allocations, I'll make the change.

OK, I had to look at this one more time. I like the idea.

OK, I'll keep it as-is.

+};
+
+/**
+ * struct is31fl32xx_chipdef - chip-specific attributes
+ * @channels            : Number of LED channels
+ * @shutdown_reg        : address of Shutdown register (optional)
+ * @pwm_update_reg      : address of PWM Update register
+ * @global_control_reg	: address of Global Control register (optional)
+ * @reset_reg           : address of Reset register (optional)
+ * @pwm_register_base	: address of first PWM register
+ * @pwm_registers_reversed: : true if PWM registers count down instead of up
+ * @led_control_register_base : address of first LED control register (optional)
+ * @enable_bits_per_led_control_register: number of LEDs enable bits in each
+ * @reset_func:         : pointer to reset function
+ *
+ * For all optional register addresses, the sentinel value %IS31FL32XX_REG_NONE
+ * indicates that this chip has no such register.
+ *
+ * If non-NULL, @reset_func will be called during probing to set all
+ * necessary registers to a known initialization state. This is needed
+ * for chips that do not have a @reset_reg.
+ *
+ * @enable_bits_per_led_control_register must be >=1 if
+ * @led_control_register_base != %IS31FL32XX_REG_NONE.
+ */
+struct is31fl32xx_chipdef {
+	u8	channels;
+	u8	shutdown_reg;
+	u8	pwm_update_reg;
+	u8	global_control_reg;
+	u8	reset_reg;
+	u8	pwm_register_base;
+	bool	pwm_registers_reversed;
+	u8	led_control_register_base;
+	u8	enable_bits_per_led_control_register;
+	int (*reset_func)(struct is31fl32xx_priv *priv);
+};
+
+static const struct is31fl32xx_chipdef is31fl3236_cdef = {
+	.channels				= 36,
+	.shutdown_reg				= 0x00,
+	.pwm_update_reg				= 0x25,
+	.global_control_reg			= 0x4a,
+	.reset_reg				= 0x4f,
+	.pwm_register_base			= 0x01,
+	.led_control_register_base		= 0x26,
+	.enable_bits_per_led_control_register	= 1,
+};
+
+static const struct is31fl32xx_chipdef is31fl3235_cdef = {
+	.channels				= 28,
+	.shutdown_reg				= 0x00,
+	.pwm_update_reg				= 0x25,
+	.global_control_reg			= 0x4a,
+	.reset_reg				= 0x4f,
+	.pwm_register_base			= 0x05,
+	.led_control_register_base		= 0x2a,
+	.enable_bits_per_led_control_register	= 1,
+};
+
+static const struct is31fl32xx_chipdef is31fl3218_cdef = {
+	.channels				= 18,
+	.shutdown_reg				= 0x00,
+	.pwm_update_reg				= 0x16,
+	.global_control_reg			= IS31FL32XX_REG_NONE,
+	.reset_reg				= 0x17,
+	.pwm_register_base			= 0x01,
+	.led_control_register_base		= 0x13,
+	.enable_bits_per_led_control_register	= 6,
+};
+
+static int is31fl3216_reset(struct is31fl32xx_priv *priv);
+static const struct is31fl32xx_chipdef is31fl3216_cdef = {
+	.channels				= 16,
+	.shutdown_reg				= IS31FL32XX_REG_NONE,
+	.pwm_update_reg				= 0xB0,
+	.global_control_reg			= IS31FL32XX_REG_NONE,
+	.reset_reg				= IS31FL32XX_REG_NONE,
+	.pwm_register_base			= 0x10,
+	.pwm_registers_reversed			= true,
+	.led_control_register_base		= 0x01,
+	.enable_bits_per_led_control_register	= 8,
+	.reset_func				= is31fl3216_reset,
+};
+
+static int is31fl32xx_write(struct is31fl32xx_priv *priv, u8 reg, u8 val)
+{
+	int ret;
+
+	dev_dbg(&priv->client->dev, "writing register 0x%02X=0x%02X", reg, val);
+
+	ret =  i2c_smbus_write_byte_data(priv->client, reg, val);
+	if (ret) {
+		dev_err(&priv->client->dev,
+			"register write to 0x%02X failed (error %d)",
+			reg, ret);
+	}
+	return ret;
+}
+
+/*
+ * Custom reset function for IS31FL3216 because it does not have a RESET
+ * register the way that the other IS31FL32xx chips do. We don't bother
+ * writing the GPIO and animation registers, because the registers we
+ * do write ensure those will have no effect.
+ */
+static int is31fl3216_reset(struct is31fl32xx_priv *priv)
+{
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < priv->cdef->channels; i++) {
+		ret = is31fl32xx_write(priv, priv->cdef->pwm_register_base+i,
+				       0x00);
+		if (ret)
+			return ret;
+	}
+	ret = is31fl32xx_write(priv, priv->cdef->pwm_update_reg, 0);
+	if (ret)
+		return ret;
+	ret = is31fl32xx_write(priv, IS31FL3216_LIGHTING_EFFECT_REG, 0x00);
+	if (ret)
+		return ret;
+	ret = is31fl32xx_write(priv, IS31FL3216_CHANNEL_CONFIG_REG, 0x00);
+	if (ret)
+		return ret;
+	ret = is31fl32xx_write(priv, IS31FL3216_CONFIG_REG, 0x00);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+
+static int is31fl32xx_brightness_set(struct led_classdev *led_cdev,
+				     enum led_brightness brightness)
+{
+	const struct is31fl32xx_led_data *led_data =
+		container_of(led_cdev, struct is31fl32xx_led_data, cdev);
+	const struct is31fl32xx_chipdef *cdef = led_data->priv->cdef;
+	u8 pwm_register_offset;
+	int ret;
+
+	dev_dbg(led_cdev->dev, "%s: %d\n", __func__, brightness);
+
+	/* NOTE: led_data->channel is 1-based */
+	if (cdef->pwm_registers_reversed)
+		pwm_register_offset = cdef->channels - led_data->channel;
+	else
+		pwm_register_offset = led_data->channel - 1;
+
+	ret = is31fl32xx_write(led_data->priv,
+			       cdef->pwm_register_base + pwm_register_offset,
+			       brightness);
+	if (ret)
+		return ret;

I infer that nothing wrong happens in case current process is preempted
here by the call originating from the other sub-LED?

I do not believe so. All the driver-specific data used here is read-only
after probing. chipdefs are entirely const, and the only thing in priv
that's referenced is the chipdef pointer which logically could not change
post-probe. Actually nothing else in priv is modified post-probe either.

The I2C core code has a mutex on the bus, so two writes cannot happen at
once.

In all these devices there is a unique PWM duty-cycle register for each
LED channel (which is what is being written here), so no register writes
for one LED channel effect any others.

I believe the worst that could happen is that the device would see:
	PWM_REG_A write X
	PWM_REG_B write Y
	UPDATE_REG write 0
	UPDATE_REG write 0
instead of
	PWM_REG_A write X
	UPDATE_REG write 0
	PWM_REG_B write Y
	UPDATE_REG write 0
but that makes no difference to the functionality. Poking the update
register merely applies all PWM register writes up to that point (I'm
assuming to allow atomically changing the state of multiple LEDs at
once).

Thanks for this comprehensive explanation.

Should I put some part of this explanation in a comment somewhere? Seems
like the kind of thing someone else might wonder about in the future also.

Good idea.

Done.

I should note here (as mentioned in cover letter), I made a choice to
always leave the per-LED "enable" bits on, and let the PWM just get set
to 0 naturally to turn an LED off. This differs from the existing SN3218
driver, which used regmap_update_bits, and is then protected by a per-
regmap mutex.

ack.

+	return is31fl32xx_write(led_data->priv, cdef->pwm_update_reg, 0);
+
+}
+
+static int is31fl32xx_init_regs(struct is31fl32xx_priv *priv)
+{
+	const struct is31fl32xx_chipdef *cdef = priv->cdef;
+	int ret;
+
+	if (cdef->reset_reg != IS31FL32XX_REG_NONE) {
+		ret = is31fl32xx_write(priv, cdef->reset_reg, 0);
+		if (ret)
+			return ret;
+	}
+	if (cdef->reset_func) {
+		ret = cdef->reset_func(priv);
+		if (ret)
+			return ret;
+	}
+	if (cdef->led_control_register_base != IS31FL32XX_REG_NONE) {
+		u8 value =
+		    GENMASK(cdef->enable_bits_per_led_control_register-1, 0);
+		u8 num_regs = cdef->channels /
+				cdef->enable_bits_per_led_control_register;
+		int i;
+
+		for (i = 0; i < num_regs; i++) {
+			ret = is31fl32xx_write(priv,
+					       cdef->led_control_register_base+i,
+					       value);
+			if (ret)
+				return ret;
+		}
+	}
+	if (cdef->shutdown_reg != IS31FL32XX_REG_NONE) {
+		ret = is31fl32xx_write(priv, cdef->shutdown_reg, BIT(0));
+		if (ret)
+			return ret;
+	}
+	if (cdef->global_control_reg != IS31FL32XX_REG_NONE) {
+		ret = is31fl32xx_write(priv, cdef->global_control_reg, 0x00);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static inline size_t sizeof_is31fl32xx_priv(int num_leds)
+{
+	return sizeof(struct is31fl32xx_priv) +
+		      (sizeof(struct is31fl32xx_led_data) * num_leds);
+}
+
+static int is31fl32xx_parse_child_dt(const struct device *dev,
+				     const struct device_node *child,
+				     struct is31fl32xx_led_data *led_data)
+{
+	struct led_classdev *cdev = &led_data->cdev;
+	int ret = 0;
+	u32 reg;
+
+	cdev->name = of_get_property(child, "label", NULL) ? : child->name;
+
+	ret = of_property_read_u32(child, "reg", &reg);
+	if (ret || reg < 1 || reg > led_data->priv->cdef->channels) {
+		dev_err(dev,
+			"Child node %s does not have a valid reg property\n",
+			child->name);
+		return -EINVAL;
+	}
+	led_data->channel = reg;
+
+	cdev->default_trigger = of_get_property(child, "linux,default-trigger",
+						NULL);
+	cdev->brightness = LED_OFF;

devm_kzalloc secures that.

OK, I will remove.

+	ret = of_property_read_u32(child, "max-brightness",
+				   &cdev->max_brightness);
+	if (ret == -EINVAL) {
+		cdev->max_brightness = 255;

s/255/LED_FULL/

Noted, although (from the patch 2 discussion) max-brightness property is
removed/replaced, this would go away anyways.

+	} else if (ret) {
+		dev_dbg(dev,
+			"Child node %s has an invalid max-brightness property\n",
+			child->name);
+		return -EINVAL;
+	}
+
+	cdev->brightness_set_blocking = is31fl32xx_brightness_set;

Please add empty line here.

Done.

+	return 0;
+}
+
+static struct is31fl32xx_led_data *is31fl32xx_find_led_data(
+					struct is31fl32xx_priv *priv,
+					u8 channel)
+{
+	size_t i;
+
+	for (i = 0; i < priv->num_leds; i++) {
+		if (priv->leds[i].channel == channel)
+			return &priv->leds[i];
+	}

Ditto.

Done.

+	return NULL;
+}
+
+static int is31fl32xx_parse_dt(struct device *dev,
+			       struct is31fl32xx_priv *priv)
+{
+	struct device_node *child;
+
+	for_each_child_of_node(dev->of_node, child) {
+		struct is31fl32xx_led_data *led_data =
+			&priv->leds[priv->num_leds];
+		int ret = 0;
+		const struct is31fl32xx_led_data *other_led_data;
+
+		led_data->priv = priv;
+
+		ret = is31fl32xx_parse_child_dt(dev, child, led_data);
+		if (ret)
+			continue;

I prefer failing in such cases,

OK, I will change to an 'goto err' which will have an 'of_node_put()'
and 'return ret'.

I will say, however, that while testing the error-detection in the
parsing logic, it was very convenient to construct a single devicetree
with a variety of errors. Then a single boot would test multiple
cases at once.

Good idea for testing, but in case some failure occurs during DT child
node parsing in the release environment you're left with unused
allocated memory.

Agreed. BTW, I assume from this that it's common to say "if there's
anything wrong in one part of your DT, there is no guarantee as to what
parts will actually be used"? I say this because what we're saying is that
if one LED node on this device is faulty, that all of them are ignored.
Analogy might be to a whole I2C bus being ignored because one of the
devices on it failed to probe. To the devicetree it's still a parent/child
bus/address relationship, even though the driver implementation is
very different.

I2C example is too generic I think. I2C controller is not as tightly
coupled with I2C clients as LED controller with its current outputs.
Besides, I2C controller doesn't have an idea what devices will attach
to the bus it controls upon probing, contrarily to a LED controller.

OK. I realize that in code there is a large distinction in these cases,
but I wasn't sure if that would be reflected in how errors in parsing
the devicetree should handled. Sounds like there is at least a de-facto
distinction between "a device and its children" and "a bus and its
children".

+
+		/* Detect if channel is already in use by another child */
+		other_led_data = is31fl32xx_find_led_data(priv,
+							  led_data->channel);
+		if (other_led_data) {
+			dev_err(dev,
+				"%s ignored: channel %d already used by %s",
+				led_data->cdev.name,
+				led_data->channel,
+				other_led_data->cdev.name);
+			continue;

Ditto.

OK.

+		}
+
+		ret = devm_led_classdev_register(dev, &led_data->cdev);
+		if (ret == 0) {
+			priv->num_leds++;
+		} else {
+			dev_err(dev, "failed to register PWM led for %s: %d\n",
+				led_data->cdev.name, ret);

Should I also fail here, then? Right now it will continue trying to
register future LED devices if a classdev_register fails for some
reason, and will successfully load even if all of them fail.

Please fail here too. If we can't setup the sub-LED that is advertised
in a DT child node, then it means that something went wrong.
This is clear error case.

Done.

+		}
+	}
+
+	return 0;
+}
+
+static const struct of_device_id of_is31fl31xx_match[] = {
+	{ .compatible = "issi,is31fl3236", .data = &is31fl3236_cdef, },
+	{ .compatible = "issi,is31fl3235", .data = &is31fl3235_cdef, },
+	{ .compatible = "issi,is31fl3218", .data = &is31fl3218_cdef, },
+	{ .compatible = "issi,is31fl3216", .data = &is31fl3216_cdef, },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, of_is31fl31xx_match);
+
+static int is31fl32xx_probe(struct i2c_client *client,
+				  const struct i2c_device_id *id)
+{
+	const struct is31fl32xx_chipdef *cdef;
+	const struct of_device_id *of_dev_id;
+	struct device *dev = &client->dev;
+	struct is31fl32xx_priv *priv;
+	int count;
+	int ret = 0;
+
+	of_dev_id = of_match_device(of_is31fl31xx_match, dev);
+	if (!of_dev_id)
+		return -EINVAL;
+
+	cdef = of_dev_id->data;
+
+	count = of_get_child_count(dev->of_node);
+	if (!count)
+		return -EINVAL;
+
+	priv = devm_kzalloc(dev, sizeof_is31fl32xx_priv(count),
+			    GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->client = client;
+	priv->cdef = cdef;
+	i2c_set_clientdata(client, priv);
+
+	ret = is31fl32xx_init_regs(priv);
+	if (ret)
+		return ret;
+
+	ret = is31fl32xx_parse_dt(dev, priv);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int is31fl32xx_remove(struct i2c_client *client)
+{
+	struct is31fl32xx_priv *priv = i2c_get_clientdata(client);
+
+	/* If there is a reset reg, then it does everything we need */
+	if (priv->cdef->reset_reg != IS31FL32XX_REG_NONE)
+		return is31fl32xx_write(priv, priv->cdef->reset_reg, 0);
+
+	/* If there is a reset func, then it does everything we need */
+	if (priv->cdef->reset_func)
+		return priv->cdef->reset_func(priv);
+
+	/* If we can't reset, then try just using software-shutdown mode */
+	if (priv->cdef->shutdown_reg != IS31FL32XX_REG_NONE)
+		return is31fl32xx_write(priv, priv->cdef->shutdown_reg, 0x00);
+
+	return 0;
+}
+
+/*
+ * i2c-core requires that id_table be non-NULL, even though
+ * it is not used for DeviceTree based instantiation.
+ */
+static const struct i2c_device_id is31fl31xx_id[] = {
+	{},
+};
+
+MODULE_DEVICE_TABLE(i2c, is31fl31xx_id);
+
+static struct i2c_driver is31fl32xx_driver = {
+	.driver = {
+		.name	= "is31fl32xx",
+		.of_match_table = of_is31fl31xx_match,
+	},
+	.probe		= is31fl32xx_probe,
+	.remove		= is31fl32xx_remove,
+	.id_table	= is31fl31xx_id,
+};
+
+module_i2c_driver(is31fl32xx_driver);
+
+MODULE_AUTHOR("David Rivshin <drivshin@xxxxxxxxxxx>");
+MODULE_DESCRIPTION("ISSI IS31FL32xx LED driver");
+MODULE_LICENSE("GPL v2");

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html





--
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux