Hi Jacek,

> Am 07.07.2016 um 13:32 schrieb Jacek Anaszewski <j.anaszew...@samsung.com>:
> 
> Hi Nikolaus,
> 
> On 07/07/2016 11:03 AM, H. Nikolaus Schaller wrote:
>> Hi Jacek,
>> 
>>> Am 07.07.2016 um 10:47 schrieb Jacek Anaszewski <j.anaszew...@samsung.com>:
>>> 
>>> Hi Nikolaus,
>>> 
>>> Thanks for the updated version. Some comments below.
>>> 
>>> On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:
>>>> This is a driver for the Integrated Silicon Solution Inc. LED driver
>>>> chips series IS31FL319x. They can drive 1, 3, 6  or up to 9
>>>> LEDs.
>>>> 
>>>> Each LED is individually controllable in brightness (through pwm)
>>>> in 256 steps so that RGB LEDs can show any of ca. 16 Mio colors.
>>>> 
>>>> The maximum current of the LEDs can be programmed and limited to
>>>> 5 .. 40mA through a device tree property.
>>>> 
>>>> The chip is connected through I2C and can have one of 4 addresses
>>>> in the range 0x64 .. 0x67 depending on how the AD pin is connected. The
>>>> address is defined by the reg property as usual.
>>>> 
>>>> The chip also has a shutdown input which could be connected to a GPIO,
>>>> but this driver uses software shutdown if all LEDs are inactivated.
>>>> 
>>>> The chip also has breathing and audio features which are not fully
>>>> supported by this driver.
>>>> 
>>>> Tested-on: OMAP5 based Pyra handheld prototype.
>>>> Signed-off-by: H. Nikolaus Schaller <h...@goldelico.com>
>>>> ---
>>>>  drivers/leds/Kconfig           |   8 +
>>>>  drivers/leds/Makefile          |   1 +
>>>>  drivers/leds/leds-is31fl319x.c | 354 
>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 363 insertions(+)
>>>>  create mode 100644 drivers/leds/leds-is31fl319x.c
>>>> 
>>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>>> index 5ae2834..65b1045 100644
>>>> --- a/drivers/leds/Kconfig
>>>> +++ b/drivers/leds/Kconfig
>>>> @@ -570,6 +570,14 @@ config LEDS_SEAD3
>>>>      This driver can also be built as a module. If so the module
>>>>      will be called leds-sead3.
>>>> 
>>>> +config LEDS_IS31FL319X
>>>> +  tristate "LED Support for ISSI IS31FL319x I2C LED controller family"
>>>> +  depends on LEDS_CLASS && I2C && OF
>>>> +  help
>>>> +    This option enables support for LEDs connected to ISSI IS31FL319x
>>>> +    fancy LED driver chips accessed via the I2C bus.
>>>> +    Driver supports individual PWM brightness control for each channel.
>>>> +
>>>>  config LEDS_IS31FL32XX
>>>>    tristate "LED support for ISSI IS31FL32XX I2C LED controller family"
>>>>    depends on LEDS_CLASS && I2C && OF
>>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>>> index cb2013d..b6ce9f9 100644
>>>> --- a/drivers/leds/Makefile
>>>> +++ b/drivers/leds/Makefile
>>>> @@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MENF21BMC)             += 
>>>> leds-menf21bmc.o
>>>>  obj-$(CONFIG_LEDS_KTD2692)                += leds-ktd2692.o
>>>>  obj-$(CONFIG_LEDS_POWERNV)                += leds-powernv.o
>>>>  obj-$(CONFIG_LEDS_SEAD3)          += leds-sead3.o
>>>> +obj-$(CONFIG_LEDS_IS31FL319X)             += leds-is31fl319x.o
>>>>  obj-$(CONFIG_LEDS_IS31FL32XX)             += leds-is31fl32xx.o
>>>> 
>>>>  # LED SPI Drivers
>>>> diff --git a/drivers/leds/leds-is31fl319x.c 
>>>> b/drivers/leds/leds-is31fl319x.c
>>>> new file mode 100644
>>>> index 0000000..bbf8850
>>>> --- /dev/null
>>>> +++ b/drivers/leds/leds-is31fl319x.c
>>>> @@ -0,0 +1,354 @@
>>>> +/*
>>>> + * Copyright 2015-16 Golden Delicious Computers
>>>> + *
>>>> + * Author: Nikolaus Schaller <h...@goldelico.com>
>>>> + *
>>>> + * Based on leds-tca6507.c
>>>> + *
>>>> + * This file is subject to the terms and conditions of version 2 of
>>>> + * the GNU General Public License.  See the file COPYING in the main
>>>> + * directory of this archive for more details.
>>>> + *
>>>> + * LED driver for the IS31FL3191/3/6/99 to drive 1, 3, 6 or 9 light
>>>> + * effect LEDs.
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/err.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/leds.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/regmap.h>
>>>> +#include <linux/slab.h>
>>>> +
>>>> +/* register numbers */
>>>> +#define IS31FL319X_SHUTDOWN       0x00
>>>> +#define IS31FL319X_CTRL1  0x01
>>>> +#define IS31FL319X_CTRL2  0x02
>>>> +#define IS31FL319X_CONFIG1        0x03
>>>> +#define IS31FL319X_CONFIG2        0x04
>>>> +#define IS31FL319X_RAMP_MODE      0x05
>>>> +#define IS31FL319X_BREATH_MASK    0x06
>>>> +#define IS31FL319X_PWM1           0x07
>>>> +#define IS31FL319X_PWM2           0x08
>>>> +#define IS31FL319X_PWM3           0x09
>>>> +#define IS31FL319X_PWM4           0x0a
>>>> +#define IS31FL319X_PWM5           0x0b
>>>> +#define IS31FL319X_PWM6           0x0c
>>>> +#define IS31FL319X_PWM7           0x0d
>>>> +#define IS31FL319X_PWM8           0x0e
>>>> +#define IS31FL319X_PWM9           0x0f
>>>> +#define IS31FL319X_DATA_UPDATE    0x10
>>>> +#define IS31FL319X_T0_1           0x11
>>>> +#define IS31FL319X_T0_2           0x12
>>>> +#define IS31FL319X_T0_3           0x13
>>>> +#define IS31FL319X_T0_4           0x14
>>>> +#define IS31FL319X_T0_5           0x15
>>>> +#define IS31FL319X_T0_6           0x16
>>>> +#define IS31FL319X_T0_7           0x17
>>>> +#define IS31FL319X_T0_8           0x18
>>>> +#define IS31FL319X_T0_9           0x19
>>>> +#define IS31FL319X_T123_1 0x1a
>>>> +#define IS31FL319X_T123_2 0x1b
>>>> +#define IS31FL319X_T123_3 0x1c
>>>> +#define IS31FL319X_T4_1           0x1d
>>>> +#define IS31FL319X_T4_2           0x1e
>>>> +#define IS31FL319X_T4_3           0x1f
>>>> +#define IS31FL319X_T4_4           0x20
>>>> +#define IS31FL319X_T4_5           0x21
>>>> +#define IS31FL319X_T4_6           0x22
>>>> +#define IS31FL319X_T4_7           0x23
>>>> +#define IS31FL319X_T4_8           0x24
>>>> +#define IS31FL319X_T4_9           0x25
>>>> +#define IS31FL319X_TIME_UPDATE    0x26
>>>> +#define IS31FL319X_RESET  0xff
>>>> +
>>>> +#define IS31FL319X_REG_CNT        (IS31FL319X_RESET + 1)
>>>> +
>>>> +#define NUM_LEDS 9        /* max for 3199 chip */
>>>> +
>>>> +struct is31fl319x_chip {
>>>> +  struct i2c_client       *client;
>>>> +  struct regmap           *regmap;
>>>> +
>>>> +  struct is31fl319x_led {
>>>> +          struct is31fl319x_chip  *chip;
>>>> +          struct led_classdev     led_cdev;
>>>> +  } leds[NUM_LEDS];
>>>> +};
>>>> +
>>>> +static const struct i2c_device_id is31fl319x_id[] = {
>>>> +  { "is31fl3190", 1 },
>>>> +  { "is31fl3191", 1 },
>>>> +  { "is31fl3193", 3 },
>>>> +  { "is31fl3196", 6 },
>>>> +  { "is31fl3199", 9 },
>>>> +  { "sn3199", 9 },
>>>> +  { }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(i2c, is31fl319x_id);
>>>> +
>>>> +static int is31fl319x_brightness_set(struct led_classdev *led_cdev,
>>>> +                             enum led_brightness brightness)
>>>> +{
>>>> +  struct is31fl319x_led *led = container_of(led_cdev,
>>>> +                                            struct is31fl319x_led,
>>>> +                                            led_cdev);
>>>> +  struct is31fl319x_chip *is31 = led->chip;
>>>> +  int ret;
>>>> +
>>>> +  int i;
>>>> +  u8 ctrl1, ctrl2;
>>> 
>>> You can initialize these variables here.
>> 
>> Ok.
>> 
>>> 
>>>> +
>>>> +  dev_dbg(&is31->client->dev, "%s %d: %d\n", __func__, (led - is31->leds),
>>>> +          brightness);
>>>> +
>>>> +  /* update PWM register */
>>>> +  ret = regmap_write(is31->regmap, IS31FL319X_PWM1 + (led - is31->leds),
>>>> +                     brightness);
>>>> +  if (ret < 0)
>>>> +          return ret;
>>>> +
>>>> +  ctrl1 = 0;
>>>> +  ctrl2 = 0;
>>> 
>>> And remove above two lines.
>>> 
>>>> +
>>>> +  /* read current brightness of all PWM channels */
>>>> +  for (i = 0; i < NUM_LEDS; i++) {
>>>> +          unsigned int pwm_value;
>>>> +          bool on;
>>>> +
>>>> +          /*
>>>> +           * since neither led_cdev nor the chip can provide
>>>> +           * the current setting, we read from the regmap cache
>>>> +           */
>>>> +
>>>> +          ret = regmap_read(is31->regmap, IS31FL319X_PWM1 + i,
>>>> +                            &pwm_value);
>>>> +
>>>> +          dev_dbg(&is31->client->dev, "%s read %d: ret=%d: %d\n",
>>>> +                  __func__, i, ret, pwm_value);
>>>> +          on = ret >= 0 && pwm_value > LED_OFF;
>>>> +
>>>> +          if (i < 3)
>>>> +                  ctrl1 |= on << i;       /* 0..2 => bit 0..2 */
>>>> +          else if (i < 6)
>>>> +                  ctrl1 |= on << (i+1);   /* 3..5 => bit 4..6 */
>>>> +          else
>>>> +                  ctrl2 |= on << (i-6);   /* 6..8 => bit 0..2 */
>>>> +  }
>>>> +
>>>> +
>>>> +  if (ctrl1 > 0 || ctrl2 > 0) {
>>>> +          dev_dbg(&is31->client->dev, "power up %02x %02x\n",
>>>> +                  ctrl1, ctrl2);
>>>> +          regmap_write(is31->regmap, IS31FL319X_CTRL1, ctrl1);
>>>> +          regmap_write(is31->regmap, IS31FL319X_CTRL2, ctrl2);
>>>> +          /* update PWMs */
>>>> +          regmap_write(is31->regmap, IS31FL319X_DATA_UPDATE, 0x00);
>>>> +          /* enable chip from shut down */
>>>> +          ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x01);
>>>> +  } else {
>>>> +          dev_dbg(&is31->client->dev, "power down\n");
>>>> +          /* shut down (no need to clear CTRL1/2) */
>>>> +          ret = regmap_write(is31->regmap, IS31FL319X_SHUTDOWN, 0x00);
>>>> +  }
>>> 
>>> You need a mutex protection in this function, so as to prevent other
>>> processes from jumping in in the middle of setting up a brightness,
>>> which requires writing several registers.
>> 
>> Uh?
>> I thought that registering is31fl319x_brightness_set as
>> led_cdev.brightness_set_blocking
>> does the trick that we don't need a private mutex here because it
>> can be called only once anyways. At least that was suggested during v1 
>> review.
> 
> Blocking doesn't mean that no one else can call the function in
> the same time, but to the fact that the caller can be blocked
> for the time needed for satisfying some condition. e.g. completion
> of I2C transmission. It needs different treatment in the LED core,
> as brightness setting can't sleep in general, due to the fact,
> that it can be called from atomic context. In this case LED core
> delegates brightness setting to a work queue task and
> led_set_brightness() returns without blocking the caller.

Ok. That is what I didn't think about: multiple LED triggers running
in parallel.

And with the new structure each one is independently calling set_brightness.
i2c traffic has its mutex but not the step between writing and reading the PWM
values to control chip shut down. The risk is that it is shut down in the wrong
moment by a race.

Thanks for spotting this.

> 
> Excerpt from linux/leds.h:
> 
> =================================================================
> 
> /* Set LED brightness level
> * Must not sleep. Use brightness_set_blocking for drivers
> * that can sleep while setting brightness.
> */
> void            (*brightness_set)(struct led_classdev *led_cdev,
>                                  enum led_brightness brightness);
> /*
> * Set LED brightness level immediately - it can block the caller for
> * the time required for accessing a LED device register.
> */
> int (*brightness_set_blocking)(struct led_classdev *led_cdev,
>                               enum led_brightness brightness);
> 
> =================================================================
> 
> 
> In the [1] I suggested the following :
> 
> "You can serialize the operations in brightness_set_blocking with
> a mutex."
> 
>>> 
>>>> +  return ret;
>>>> +}
>>>> +
>>>> +static struct led_info *
>>>> +is31fl319x_parse_dt(struct i2c_client *client, int num_leds)
>>>> +{
>>>> +  struct device_node *np = client->dev.of_node, *child;
>>>> +  struct led_info *is31_leds;
>>>> +  int count;
>>>> +
>>>> +  if (!np)
>>>> +          return ERR_PTR(-ENODEV);
>>>> +
>>>> +  count = of_get_child_count(np);
>>>> +  dev_dbg(&client->dev, "child count %d\n", count);
>>>> +  if (!count || count > NUM_LEDS)
>>>> +          return ERR_PTR(-ENODEV);
>>>> +
>>>> +  is31_leds = devm_kzalloc(&client->dev,
>>>> +                  sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);
>>>> +  if (!is31_leds)
>>>> +          return ERR_PTR(-ENOMEM);
>>>> +
>>>> +  for_each_child_of_node(np, child) {
>>>> +          struct led_info led;
>>> 
>>> Using this local variable adds too much noise here.
>>> I suggest avoiding the use of struct led_info. It is semantically
>>> inconsistent in the context of DT parsing, since you don't have
>>> any flags to parse.
>>> 
>>>> +          u32 reg;
>>>> +          int ret;
>>>> +
>>>> +          led.name = NULL;
>>>> +          ret = of_property_read_string(child, "label", &led.name);
>>>> +          if (ret < 0 && ret != -EINVAL)  /* is optional */
>>>> +                  return ERR_PTR(ret);
>>> 
>>> You could assign the name directly to related struct led_classdev
>>> property. Moreover, if label is not present the child node name
>>> should be used for LED class device name. Please refer to the other
>>> drivers and common LED bindings.
>> 
>> Ok.
>> 
>>> 
>>>> +          led.default_trigger = NULL;
>>>> +          ret = of_property_read_string(child, "linux,default-trigger",
>>>> +                  &led.default_trigger);
>>> 
>>> Here also please assign the trigger directly to struct led_classdev.
>> 
>> Ok.
>> 
>>> 
>>>> +          if (ret < 0 && ret != -EINVAL)  /* is optional */
>>>> +                  return ERR_PTR(ret);
>>>> +          led.flags = 0;
>>>> +          ret = of_property_read_u32(child, "reg", &reg);
>>>> +          dev_dbg(&client->dev, "name = %s reg = %d\n", led.name, reg);
>>>> +          reg -= 1;       /* index 0 is at reg = 1 */
>>>> +          if (ret != 0 || reg < 0 || reg >= num_leds)
>>>> +                  continue;
>>>> +
>>>> +          if (is31_leds[reg].name)
>>>> +                  dev_err(&client->dev, "duplicate led line %d for %s -> 
>>>> %s\n",
>>>> +                          reg, is31_leds[reg].name, led.name);
>>>> +          is31_leds[reg] = led;
>>>> +  }
>>>> +
>>>> +  return is31_leds;
>>>> +}
>>>> +
>>>> +static const struct of_device_id of_is31fl319x_leds_match[] = {
>>>> +  { .compatible = "issi,is31fl3190", (void *) 1 },
>>>> +  { .compatible = "issi,is31fl3191", (void *) 1 },
>>>> +  { .compatible = "issi,is31fl3193", (void *) 3 },
>>>> +  { .compatible = "issi,is31fl3196", (void *) 6 },
>>>> +  { .compatible = "issi,is31fl3199", (void *) 9 },
>>>> +  { .compatible = "si-en,sn3199", (void *) 9 },
>>>> +  {},
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, of_is31fl319x_leds_match);
>>>> +
>>>> +static bool is31fl319x_readable_reg(struct device *dev, unsigned int reg)
>>>> +{ /* we have no readable registers */
>>>> +  return false;
>>>> +}
>>>> +
>>>> +static bool is31fl319x_volatile_reg(struct device *dev, unsigned int reg)
>>>> +{ /* volatile registers are not cached */
>>>> +  switch (reg) {
>>>> +  case IS31FL319X_DATA_UPDATE:
>>>> +  case IS31FL319X_TIME_UPDATE:
>>>> +  case IS31FL319X_RESET:
>>>> +          return true;    /* always write-through */
>>>> +  default:
>>>> +          return false;
>>>> +  }
>>>> +}
>>>> +
>>>> +struct regmap_config regmap_config = {
>>>> +  .reg_bits = 8,
>>>> +  .val_bits = 8,
>>>> +  .max_register = IS31FL319X_REG_CNT,
>>>> +  .cache_type = REGCACHE_FLAT,
>>>> +  .readable_reg = is31fl319x_readable_reg,
>>>> +  .volatile_reg = is31fl319x_volatile_reg,
>>>> +};
>>>> +
>>>> +static int is31fl319x_probe(struct i2c_client *client,
>>>> +          const struct i2c_device_id *id)
>>>> +{
>>>> +  struct is31fl319x_chip *is31;
>>>> +  struct i2c_adapter *adapter;
>>>> +  struct led_info *leds;
>>>> +  int err;
>>>> +  int i = 0;
>>>> +
>>>> +  adapter = to_i2c_adapter(client->dev.parent);
>>>> +
>>>> +  dev_dbg(&client->dev, "probe IS31FL319x for num_leds = %d\n",
>>>> +          (int) id->driver_data);
>>>> +
>>>> +  if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
>>>> +          return -EIO;
>>>> +
>>>> +  leds = is31fl319x_parse_dt(client, (int) id->driver_data);
>>>> +  if (IS_ERR(leds)) {
>>>> +          dev_err(&client->dev, "DT parsing error %d\n",
>>>> +                  (int) PTR_ERR(leds));
>>>> +          return PTR_ERR(leds);
>>>> +  }
>>>> +
>>>> +  is31 = devm_kzalloc(&client->dev, sizeof(*is31), GFP_KERNEL);
>>>> +  if (!is31)
>>>> +          return -ENOMEM;
>>>> +
>>>> +  is31->client = client;
>>>> +  is31->regmap = devm_regmap_init_i2c(client, &regmap_config);
>>>> +  if (IS_ERR(is31->regmap)) {
>>>> +          dev_err(&client->dev, "failed to allocate register map\n");
>>>> +          return PTR_ERR(is31->regmap);
>>>> +  }
>>>> +
>>>> +  i2c_set_clientdata(client, is31);
>>>> +
>>>> +  /* check for write-reply from chip (we can't read any registers) */
>>>> +  err = regmap_write(is31->regmap, IS31FL319X_RESET, 0x00);
>>> 
>>> Doesn't powering the device up result in loading registers with default
>>> values?
>> 
>> Yes, but not a reboot.
>> 
>> And we have to write *some* register anyways here to be able to detect
>> if the chip is present or does not respond.
>> 
>> So what would writing a different register change?
> 
> OK, let's leave that as is.
> 
>>> 
>>>> +  if (err < 0) {
>>>> +          dev_err(&client->dev, "no response from chip write: err = %d\n",
>>>> +                  err);
>>>> +          return -EIO;    /* does not answer */
>>>> +  }
>>>> +
>>>> +  /* initialize chip and regmap so that we never try to read from i2c */
>>> 
>>> You can initialize default register values without writing them,
>>> by using regmap's reg_defaults property.
>> 
>> Hm,
>> 
>>> 
>>>> +  regmap_write(is31->regmap, IS31FL319X_CTRL1, 0x00);
>>>> +  regmap_write(is31->regmap, IS31FL319X_CTRL2, 0x00);
>>>> +  for (i = 0; i < NUM_LEDS; i++)
>>>> +          regmap_write(is31->regmap, IS31FL319X_PWM1 + i, 0x00);
>> 
>> would replace a small loop with a bigger table. But if you prefer this I can 
>> change.
> 
> Writing registers only to initialize regmap cache looks odd.
> Moreover it unnecessarily extends driver probing time.

Ok, we will change.

> 
>>>> +
>>>> +  if (client->dev.of_node) {
>>>> +          u32 val;
>>>> +          u8 config2 = 0;
>>>> +
>>>> +          if (of_property_read_u32(client->dev.of_node,
>>>> +                                   "led-max-microamp", &val)) {
>>> 
>>> of_property_read_u32 returns 0 on success.
>> 
>> Ok.
>> 
>>> 
>>> led-max-microamp needs to be defined per each sub-LED (child node).
>>> The greatest value should be used for setting CS bit field of CTRL2
>>> register, and max_brightness values of the LEDs with lower max microamp
>>> value should be limited accordingly.
>> 
>> Hm. The chip does not allow to limit individual LEDs. So we should take the
>> *minimum* of all subnodes.
> 
> But the LED core does. That's a max_brightness property is for.

Ok, that seems to be a new feature we have no use case for since all our
LEDs have the same current specification. But we will check how it can be done.

> 
>>> 
>>> Moreover, this is still DT parsing and should find its place in
>>> is31fl319x_parse_dt().
>>> 
>>>> +                  if (val > 40000)
>>>> +                          val = 40000;
>>>> +                  if (val < 5000)
>>>> +                          val = 5000;
>>>> +                  config2 |= (((64000 - val) / 5000) & 0x7) << 4; /* CS */
>>>> +          }
>>>> +          if (of_property_read_u32(client->dev.of_node, "audio-gain-db",
>>>> +                                   &val)) {
>>> 
>>> Please move this to is31fl319x_parse_dt().
>> 
>> Well, the main reason is that this needs to introduce another temporary 
>> struct
>> to pass values from parse_dt to probe and copy values there.
> >
>> Would it be acceptable to inline all DT parsing?
> 
> Don't reinvent the wheel. Please compare how other LED class drivers
> perform DT parsing.

I have looked into other drivers but have not seen a common pattern. Some
have no DT (e.g. 88pm860x), some have some struct led_config_data (e.g. aat1290)
and some do it inlined in the probe function (e.g. bcm6328) and some do it
in a separate parse_dt (is31fl32).

So which driver would you recommend as best practice?

> 
>>> 
>>>> +                  if (val > 21)
>>>> +                          val = 21;
>>>> +                  config2 |= val / 3; /* AGS */
>>>> +          }
>>>> +          regmap_write(is31->regmap, IS31FL319X_CONFIG2, config2);
>>>> +  }
>>>> +
>>>> +  for (i = 0; i < NUM_LEDS; i++) {
>>>> +          struct is31fl319x_led *l = is31->leds + i;
>>>> +
>>>> +          l->chip = is31;
>>>> +          if (leds[i].name && !leds[i].flags) {
>>>> +                  l->led_cdev.name = leds[i].name;
>>>> +                  l->led_cdev.default_trigger
>>>> +                          = leds[i].default_trigger;
>>>> +                  l->led_cdev.brightness_set_blocking
>>>> +                          = is31fl319x_brightness_set;
>>>> +                  /* NOTE: is31fl319x_brightness_set will be called
>>>> +                   * immediately after register() before we return
>>>> +                   */
>>> 
>>> Who will call it? I believe that this is true only if default-on
>>> trigger is registered.
>> 
>> Yes, as soon as anyone can call it we must be aware.
> 
> This is common for all LED class devices. Please drop the comment.

Ok. Might belong somewhere else (e.g. to devm_led_classdev_register
documentation).

> 
>>> 
>>>> +                  err = devm_led_classdev_register(&client->dev,
>>>> +                                              &l->led_cdev);
>>>> +                  if (err < 0)
>>>> +                          return err;
>>>> +          }
>>>> +  }
>>>> +
>>>> +  dev_dbg(&client->dev, "probed\n");
>>> 
>>> Please remove this debug log. You can verify it by checking presence of
>>> the device in the sysfs.
>> 
>> Ok.
>> 
>>> 
>>>> +  return 0;
>>>> +}
>>>> +
>>>> +static struct i2c_driver is31fl319x_driver = {
>>>> +  .driver   = {
>>>> +          .name    = "leds-is31fl319x",
>>>> +          .of_match_table = of_match_ptr(of_is31fl319x_leds_match),
>>>> +  },
>>>> +  .probe    = is31fl319x_probe,
>>>> +  .id_table = is31fl319x_id,
>>>> +};
>>>> +
>>>> +module_i2c_driver(is31fl319x_driver);
>>>> +
>>>> +MODULE_AUTHOR("H. Nikolaus Schaller <h...@goldelico.com>");
>>>> +MODULE_DESCRIPTION("IS31FL319X LED driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>> 
>>> 
>>> 
>>> --
>>> Best regards,
>>> Jacek Anaszewski
>> 
>> Will fix asap.
>> 
>> Thanks and BR,
>> Nikolaus Schaller
>> 
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>> 
> 
> [1] https://lkml.org/lkml/2016/4/20/754
> 
> -- 
> Best regards,
> Jacek Anaszewski

BR and thanks,
Nikolaus


Reply via email to