Re: [PATCH 11/25] leds: lp8860: Use led_compose_name()

[Dan Murphy]

Jacek

On 3/11/19 12:24 PM, Jacek Anaszewski wrote:
> Dan,
> 
> On 3/11/19 1:28 PM, Dan Murphy wrote:
>> On 3/10/19 1:28 PM, Jacek Anaszewski wrote:
>>> Switch to using generic LED support for composing LED class
>>> device name.
>>>
>>> While at it, avoid iterating through available child of nodes
>>> in favor of obtaining single expected child node using single
>>> call to of_get_next_available_child().
>>>
>>> Signed-off-by: Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx>
>>> Cc: Dan Murphy <dmurphy@xxxxxx>
>>> ---
>>> Â drivers/leds/leds-lp8860.c | 38 +++++++++++++++++++-------------------
>>> Â 1 file changed, 19 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
>>> index 39c72a908f3b..7c12ccdc1f2f 100644
>>> --- a/drivers/leds/leds-lp8860.c
>>> +++ b/drivers/leds/leds-lp8860.c
>>> @@ -22,7 +22,6 @@
>>> Â #include <linux/of_gpio.h>
>>> Â #include <linux/gpio/consumer.h>
>>> Â #include <linux/slab.h>
>>> -#include <uapi/linux/uleds.h>
>>> Â Â #define LP8860_DISP_CL1_BRT_MSBÂÂÂÂÂÂÂ 0x00
>>> Â #define LP8860_DISP_CL1_BRT_LSBÂÂÂÂÂÂÂ 0x01
>>> @@ -87,6 +86,8 @@
>>> Â Â #define LP8860_CLEAR_FAULTSÂÂÂÂÂÂÂ 0x01
>>> Â +#define LP8860_NAMEÂÂÂÂÂÂÂÂÂÂÂ "lp8860"
>>> +
>>> Â /**
>>> ÂÂ * struct lp8860_led -
>>> ÂÂ * @lock - Lock for reading/writing the device
>>> @@ -96,7 +97,6 @@
>>> ÂÂ * @eeprom_regmap - EEPROM register map
>>> ÂÂ * @enable_gpio - VDDIO/EN gpio to enable communication interface
>>> ÂÂ * @regulator - LED supply regulator pointer
>>> - * @label - LED label
>>> ÂÂ */
>>> Â struct lp8860_led {
>>> ÂÂÂÂÂ struct mutex lock;
>>> @@ -106,7 +106,6 @@ struct lp8860_led {
>>> ÂÂÂÂÂ struct regmap *eeprom_regmap;
>>> ÂÂÂÂÂ struct gpio_desc *enable_gpio;
>>> ÂÂÂÂÂ struct regulator *regulator;
>>> -ÂÂÂ char label[LED_MAX_NAME_SIZE];
>>> Â };
>>> Â Â struct lp8860_eeprom_reg {
>>> @@ -387,25 +386,26 @@ static int lp8860_probe(struct i2c_client *client,
>>> ÂÂÂÂÂ struct lp8860_led *led;
>>> ÂÂÂÂÂ struct device_node *np = client->dev.of_node;
>>> ÂÂÂÂÂ struct device_node *child_node;
>>> -ÂÂÂ const char *name;
>>> +ÂÂÂ struct led_init_data init_data;
>>> Â ÂÂÂÂÂ led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);
>>> ÂÂÂÂÂ if (!led)
>>> ÂÂÂÂÂÂÂÂÂ return -ENOMEM;
>>> Â -ÂÂÂ for_each_available_child_of_node(np, child_node) {
>>> -ÂÂÂÂÂÂÂ led->led_dev.default_trigger = of_get_property(child_node,
>>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "linux,default-trigger",
>>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ NULL);
>>> -
>>> -ÂÂÂÂÂÂÂ ret = of_property_read_string(child_node, "label", &name);
>>> -ÂÂÂÂÂÂÂ if (!ret)
>>> -ÂÂÂÂÂÂÂÂÂÂÂ snprintf(led->label, sizeof(led->label), "%s:%s",
>>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ id->name, name);
>>> -ÂÂÂÂÂÂÂ else
>>> -ÂÂÂÂÂÂÂÂÂÂÂ snprintf(led->label, sizeof(led->label),
>>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "%s::display_cluster", id->name);
>>> -ÂÂÂ }
>>> +ÂÂÂ child_node = of_get_next_available_child(np, NULL);
>>
>> Again this device has multiple outputs that can be banked or separated.
>>
>> I would prefer to leave the iteration and just change the name generation.
> 
> In [0] you agreed on removing that. This code is simply broken since
> there is only one instance of struct lp8860_led allocated.
> 

Thanks for the reminder on my ack

Dan

>>> +ÂÂÂ if (!child_node)
>>> +ÂÂÂÂÂÂÂ return -EINVAL;
>>> +
>>> +ÂÂÂ init_data.fwnode = of_fwnode_handle(child_node),
>>> +
>>> +ÂÂÂ led->led_dev.default_trigger = of_get_property(child_node,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "linux,default-trigger",
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ NULL);
>>> +
>>> +ÂÂÂ ret = led_compose_name(init_data.fwnode, LP8860_NAME,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ":display_cluster", init_data.name);
>>> +ÂÂÂ if (ret)
>>> +ÂÂÂÂÂÂÂ return ret;
>>> Â ÂÂÂÂÂ led->enable_gpio = devm_gpiod_get_optional(&client->dev,
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "enable", GPIOD_OUT_LOW);
>>> @@ -420,7 +420,6 @@ static int lp8860_probe(struct i2c_client *client,
>>> ÂÂÂÂÂÂÂÂÂ led->regulator = NULL;
>>> Â ÂÂÂÂÂ led->client = client;
>>> -ÂÂÂ led->led_dev.name = led->label;
>>> ÂÂÂÂÂ led->led_dev.brightness_set_blocking = lp8860_brightness_set;
>>> Â ÂÂÂÂÂ mutex_init(&led->lock);
>>> @@ -447,7 +446,8 @@ static int lp8860_probe(struct i2c_client *client,
>>> ÂÂÂÂÂ if (ret)
>>> ÂÂÂÂÂÂÂÂÂ return ret;
>>> Â -ÂÂÂ ret = devm_led_classdev_register(&client->dev, &led->led_dev);
>>> +ÂÂÂ ret = devm_led_classdev_register_ext(&client->dev, &led->led_dev,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &init_data);
>>> ÂÂÂÂÂ if (ret) {
>>> ÂÂÂÂÂÂÂÂÂ dev_err(&client->dev, "led register err: %d\n", ret);
>>> ÂÂÂÂÂÂÂÂÂ return ret;
>>>
>>
>>
> 
> [0] https://lkml.org/lkml/2018/11/12/2231
> 


-- 
------------------
Dan Murphy