| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 07 Jul 2016 16:19:03 +0200
From: Jacek Anaszewski <j.anaszewski@...sung.com>
To: "H. Nikolaus Schaller" <hns@...delico.com>
Cc: Rob Herring <robh+dt@...nel.org>, Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
Richard Purdie <rpurdie@...ys.net>, devicetree@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>, linux-leds@...r.kernel.org,
kernel@...a-handheld.com, Marek Belisko <marek@...delico.com>,
Discussions about the Letux Kernel
<letux-kernel@...nphoenux.org>,
Andrey Utkin <andrey_utkin@...tmail.com>
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@...sung.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@...sung.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@...delico.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@...delico.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@...delico.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@...r.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
Powered by blists - more mailing lists