Messages in this thread | | | Date | Thu, 07 Jul 2016 16:19:03 +0200 | From | Jacek Anaszewski <> | Subject | Re: [PATCH v2 1/2] drivers: led: is31fl319x: 1/3/6/9-channel light effect led driver |
| |
On 07/07/2016 03:00 PM, H. Nikolaus Schaller wrote: > Hi Jacek, > >> Am 07.07.2016 um 13:32 schrieb Jacek Anaszewski <j.anaszewski@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.anaszewski@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 <hns@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 <hns@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.
Exactly.
> > 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", ®); >>>>> + 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, ®map_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.
max_brightness property and corresponding sysfs attribute were added in 2009. See:
1bd465e6b0 ("leds: allow led-drivers to use a variable range of brightness values")
>> >>>> >>>> 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?
E.g. drivers/leds/leds-is31fl32xx.c
>> >>>> >>>>> + 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 <hns@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 majordomo@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 > > > >
-- Best regards, Jacek Anaszewski
| |