Hi Jan,
Thank you for the patch.
Nice, comprehensive commit message.
The patch looks good in general, I have just one trivial nit below.
On 08/21/2018 12:53 PM, Jan Kundrát wrote:
> Our board uses this chip with no connection between the SoC's reset
> output and this chip's reset input. If we don't reset the chip and if
> nothing initializes all registers in some other manner, there will be
> discrepancies in what the LED class' sysfs entries report and the actual
> state of the LEDs.
>
> We are also preconfiguring the chip for HW blinking from the boot loader
> as an indicator of the I'm-busy-booting state. Once the kernel is up, we
> are relying on the DT configuration to blink our status LED in a
> different color, and when userspace is up, we finally pass control there
> via the standard sysfs entries. Without a full reset from kernel, the
> visual result is very confusing and one might get, e.g, purple flashes
> instead of a nice solid green light.
>
> Performing a SW reset is a bit tricky because the address for the SW
> reset is different than the actual chip's address. This also means that
> there's just one shared SWRST address for all chips which on that
> particular I2C bus. We cannot therefore send the SWRST during each
> chip's probe because doing that would reset all connected chips,
> including those which we initialized before. This patch attempts to
> solve this by trying to register the SWRST-only I2C address just once.
> When probing for other chips, a failure to bind to this shared address
> should not be fatal.
>
> This approach will still break on crazy setups with, e.g., a LTC4316
> address translator in between the bus master and the slave. I think that
> this is acceptable and it's better than trying to bitmask the SWRST out
> of the actual address from the DT, for example.
>
> The SWRST address is special -- the datasheet says that the chip won't
> ACK anything else the magic SWRST byte sequence. If that's indeed the
> case, it might be possible to reuse this address by another I2C chip.
> That's another scenario which this patch breaks -- the I2C "device" at
> the magic I2C address will be marked as used, and nobody else can use it
> without extra force.
>
> Tested with a single TLC59116.
>
> Signed-off-by: Jan Kundrát <jan.kundrat@xxxxxxxxx>
> ---
> drivers/leds/leds-tlc591xx.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/leds/leds-tlc591xx.c b/drivers/leds/leds-tlc591xx.c
> index f5357f6d9e58..efbe9c83c514 100644
> --- a/drivers/leds/leds-tlc591xx.c
> +++ b/drivers/leds/leds-tlc591xx.c
> @@ -42,6 +42,9 @@
>
> #define ldev_to_led(c) container_of(c, struct tlc591xx_led, ldev)
>
> +#define TLC591XX_RESET_BYTE_0 0xa5
> +#define TLC591XX_RESET_BYTE_1 0x5a
> +
> struct tlc591xx_led {
> bool active;
> unsigned int led_no;
> @@ -53,21 +56,25 @@ struct tlc591xx_priv {
> struct tlc591xx_led leds[TLC591XX_MAX_LEDS];
> struct regmap *regmap;
> unsigned int reg_ledout_offset;
> + struct i2c_client *swrst_client;
> };
>
> struct tlc591xx {
> unsigned int max_leds;
> unsigned int reg_ledout_offset;
> + u8 swrst_addr;
> };
>
> static const struct tlc591xx tlc59116 = {
> .max_leds = 16,
> .reg_ledout_offset = 0x14,
> + .swrst_addr = 0x6b,
> };
>
> static const struct tlc591xx tlc59108 = {
> .max_leds = 8,
> .reg_ledout_offset = 0x0c,
> + .swrst_addr = 0x4b,
> };
>
> static int
> @@ -140,6 +147,8 @@ tlc591xx_destroy_devices(struct tlc591xx_priv *priv, unsigned int j)
> if (priv->leds[i].active)
> led_classdev_unregister(&priv->leds[i].ldev);
> }
> +
> + i2c_unregister_device(priv->swrst_client);
> }
>
> static int
> @@ -245,6 +254,19 @@ tlc591xx_probe(struct i2c_client *client,
> priv->leds[reg].ldev.default_trigger =
> of_get_property(child, "linux,default-trigger", NULL);
> }
> +
> + priv->swrst_client = i2c_new_dummy(client->adapter, tlc591xx->swrst_addr);
> + if (priv->swrst_client) {
> + err = i2c_smbus_write_byte_data(priv->swrst_client,
> + TLC591XX_RESET_BYTE_0, TLC591XX_RESET_BYTE_1);
> + if (err) {
> + dev_warn(dev, "SW reset failed\n");
> + }
Braces are not necessary for single statement blocks.
Moreover - I infer that you don't want to break on error here.
If so, then we don't need the assignment and it is possible
to get rid of two lines by changing it
if (i2c_smbus_write_byte_data(priv->swrst_client,
TLC591XX_RESET_BYTE_0, TLC591XX_RESET_BYTE_1))
dev_warn(dev, "SW reset failed\n");
I modified it as above and pushed to the for-4.20 branch
of linux-leds.git. Please let me know if you have any objections
regarding this change.
--
Best regards,
Jacek Anaszewski
