| Message ID | 20200919180304.2885-1-marek.behun@nic.cz |
|---|---|
| Headers | show |
| Series | Start moving parsing of `linux,default-trigger` to LED core (a cleanup of LED drivers) | expand |
Hello On 9/19/20 1:02 PM, Marek Behún wrote: > Do not use device_for_each_child_node. Since this driver works only with > once child node present, use device_get_next_child_node instead. > This also saves one level of indentation. > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > Cc: Dan Murphy <dmurphy@ti.com> > --- > drivers/leds/leds-lm36274.c | 50 +++++++++++++++++-------------------- > 1 file changed, 23 insertions(+), 27 deletions(-) > > diff --git a/drivers/leds/leds-lm36274.c b/drivers/leds/leds-lm36274.c > index 4a9f786bb9727..e0fce74a76675 100644 > --- a/drivers/leds/leds-lm36274.c > +++ b/drivers/leds/leds-lm36274.c > @@ -72,40 +72,36 @@ static int lm36274_parse_dt(struct lm36274 *chip) > char label[LED_MAX_NAME_SIZE]; > struct device *dev = &chip->pdev->dev; > const char *name; > - int child_cnt; > - int ret = -EINVAL; > + int ret; > > /* There should only be 1 node */ > - child_cnt = device_get_child_node_count(dev); > - if (child_cnt != 1) > + if (device_get_child_node_count(dev) != 1) > return -EINVAL; > > - device_for_each_child_node(dev, child) { > - ret = fwnode_property_read_string(child, "label", &name); > - if (ret) > - snprintf(label, sizeof(label), "%s::", > - chip->pdev->name); > - else > - snprintf(label, sizeof(label), "%s:%s", > - chip->pdev->name, name); > - > - chip->num_leds = fwnode_property_count_u32(child, "led-sources"); > - if (chip->num_leds <= 0) > - return -ENODEV; > - > - ret = fwnode_property_read_u32_array(child, "led-sources", > - chip->led_sources, > - chip->num_leds); > - if (ret) { > - dev_err(dev, "led-sources property missing\n"); > - return ret; > - } > - > - fwnode_property_read_string(child, "linux,default-trigger", > - &chip->led_dev.default_trigger); > + child = device_get_next_child_node(dev, NULL); > + > + ret = fwnode_property_read_string(child, "label", &name); > + if (ret) > + snprintf(label, sizeof(label), "%s::", chip->pdev->name); > + else > + snprintf(label, sizeof(label), "%s:%s", chip->pdev->name, name); > > + chip->num_leds = fwnode_property_count_u32(child, "led-sources"); > + if (chip->num_leds <= 0) > + return -ENODEV; > + > + ret = fwnode_property_read_u32_array(child, "led-sources", > + chip->led_sources, chip->num_leds); > + if (ret) { > + dev_err(dev, "led-sources property missing\n"); > + return ret; > } > > + fwnode_property_read_string(child, "linux,default-trigger", > + &chip->led_dev.default_trigger); > + > + fwnode_handle_put(child); > + > chip->lmu_data.regmap = chip->regmap; > chip->lmu_data.max_brightness = MAX_BRIGHTNESS_11BIT; > chip->lmu_data.msb_brightness_reg = LM36274_REG_BRT_MSB; Question is this device on a piece of hardware you are testing on? Just wondering how you functionally tested all these changes you submitted Reviewed-by: Dan Murphy <dmurphy@ti.com>
On Tue, 22 Sep 2020 10:42:49 -0500 Dan Murphy <dmurphy@ti.com> wrote: > Hello > > On 9/19/20 1:02 PM, Marek Behún wrote: > > Do not use device_for_each_child_node. Since this driver works only with > > once child node present, use device_get_next_child_node instead. > > This also saves one level of indentation. > > > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > > Cc: Dan Murphy <dmurphy@ti.com> > > --- > > drivers/leds/leds-lm36274.c | 50 +++++++++++++++++-------------------- > > 1 file changed, 23 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/leds/leds-lm36274.c b/drivers/leds/leds-lm36274.c > > index 4a9f786bb9727..e0fce74a76675 100644 > > --- a/drivers/leds/leds-lm36274.c > > +++ b/drivers/leds/leds-lm36274.c > > @@ -72,40 +72,36 @@ static int lm36274_parse_dt(struct lm36274 *chip) > > char label[LED_MAX_NAME_SIZE]; > > struct device *dev = &chip->pdev->dev; > > const char *name; > > - int child_cnt; > > - int ret = -EINVAL; > > + int ret; > > > > /* There should only be 1 node */ > > - child_cnt = device_get_child_node_count(dev); > > - if (child_cnt != 1) > > + if (device_get_child_node_count(dev) != 1) > > return -EINVAL; > > > > - device_for_each_child_node(dev, child) { > > - ret = fwnode_property_read_string(child, "label", &name); > > - if (ret) > > - snprintf(label, sizeof(label), "%s::", > > - chip->pdev->name); > > - else > > - snprintf(label, sizeof(label), "%s:%s", > > - chip->pdev->name, name); > > - > > - chip->num_leds = fwnode_property_count_u32(child, "led-sources"); > > - if (chip->num_leds <= 0) > > - return -ENODEV; > > - > > - ret = fwnode_property_read_u32_array(child, "led-sources", > > - chip->led_sources, > > - chip->num_leds); > > - if (ret) { > > - dev_err(dev, "led-sources property missing\n"); > > - return ret; > > - } > > - > > - fwnode_property_read_string(child, "linux,default-trigger", > > - &chip->led_dev.default_trigger); > > + child = device_get_next_child_node(dev, NULL); > > + > > + ret = fwnode_property_read_string(child, "label", &name); > > + if (ret) > > + snprintf(label, sizeof(label), "%s::", chip->pdev->name); > > + else > > + snprintf(label, sizeof(label), "%s:%s", chip->pdev->name, name); > > > > + chip->num_leds = fwnode_property_count_u32(child, "led-sources"); > > + if (chip->num_leds <= 0) > > + return -ENODEV; > > + > > + ret = fwnode_property_read_u32_array(child, "led-sources", > > + chip->led_sources, chip->num_leds); > > + if (ret) { > > + dev_err(dev, "led-sources property missing\n"); > > + return ret; > > } > > > > + fwnode_property_read_string(child, "linux,default-trigger", > > + &chip->led_dev.default_trigger); > > + > > + fwnode_handle_put(child); > > + > > chip->lmu_data.regmap = chip->regmap; > > chip->lmu_data.max_brightness = MAX_BRIGHTNESS_11BIT; > > chip->lmu_data.msb_brightness_reg = LM36274_REG_BRT_MSB; > > Question is this device on a piece of hardware you are testing on? No, unfortunately. But this driver is rather simple, in comparison to the others. As Linus said: "If it compiles, it is good; if it boots up, it is perfect." :D So if someone tested it, it would be perfect. Marek > Just wondering how you functionally tested all these changes you submitted > > Reviewed-by: Dan Murphy <dmurphy@ti.com> >
Marek On 9/22/20 10:58 AM, Marek Behun wrote: > On Tue, 22 Sep 2020 10:42:49 -0500 > Dan Murphy <dmurphy@ti.com> wrote: > >> <snip> >> Question is this device on a piece of hardware you are testing on? > No, unfortunately. But this driver is rather simple, in comparison to > the others. > > As Linus said: > "If it compiles, it is good; if it boots up, it is perfect." > :D > > So if someone tested it, it would be perfect. Not sure how a comment made in 1998 applies to the state of the kernel today. With this much change to the driver there should have been some level of functional testing. So I pulled out my hardware and gave it a whirl. Gave my TB on the LM36274 patches. Dan
On Sat, Sep 19, 2020 at 8:03 PM Marek Behún <marek.behun@nic.cz> wrote: > By using struct led_init_data when registering we do not need to parse > `label` DT property. Moreover `label` is deprecated and if it is not > present but `color` and `function` are, LED core will compose a name > from these properties instead. > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > Cc: Linus Walleij <linus.walleij@linaro.org> Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij