Messages in this thread | | | Subject | Re: [PATCH v4 8/9] leds: lp50xx: Add the LP50XX family of the RGB LED driver | From | Dan Murphy <> | Date | Wed, 31 Jul 2019 13:46:17 -0500 |
| |
Jacek
Thanks for looking at the patch
On 7/30/19 4:24 PM, Jacek Anaszewski wrote: > Hi Dan, > > Thank you for the patch I see few things to improve. > > On 7/25/19 8:28 PM, Dan Murphy wrote: >> Introduce the LP5036/30/24/18 RGB LED driver. >> The difference in these parts are the number of >> LED outputs where the: >> >> LP5036 can control 36 LEDs >> LP5030 can control 30 LEDs >> LP5024 can control 24 LEDs >> LP5018 can control 18 LEDs >> >> The device has the ability to group LED output into control banks >> so that multiple LED banks can be controlled with the same mixing and >> brightness. Inversely the LEDs can also be controlled independently. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- >> drivers/leds/Kconfig | 7 + >> drivers/leds/Makefile | 1 + >> drivers/leds/leds-lp50xx.c | 789 +++++++++++++++++++++++++++++++++++++ >> 3 files changed, 797 insertions(+) >> create mode 100644 drivers/leds/leds-lp50xx.c >> >> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >> index 7f780d5b8490..69c037020f6b 100644 >> --- a/drivers/leds/Kconfig >> +++ b/drivers/leds/Kconfig >> @@ -350,6 +350,13 @@ config LEDS_LP3952 >> To compile this driver as a module, choose M here: the >> module will be called leds-lp3952. >> >> +config LEDS_LP50XX >> + tristate "LED Support for TI LP5036/30/24/18 LED driver chip" >> + depends on LEDS_CLASS && REGMAP_I2C >> + help >> + If you say yes here you get support for the Texas Instruments >> + LP5036, LP5030, LP5024 and LP5018 LED driver. >> + >> config LEDS_LP55XX_COMMON >> tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501" >> depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501 >> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile >> index 897b810257dd..438a5499f3ee 100644 >> --- a/drivers/leds/Makefile >> +++ b/drivers/leds/Makefile >> @@ -34,6 +34,7 @@ obj-$(CONFIG_LEDS_GPIO_REGISTER) += leds-gpio-register.o >> obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o >> obj-$(CONFIG_LEDS_LP3944) += leds-lp3944.o >> obj-$(CONFIG_LEDS_LP3952) += leds-lp3952.o >> +obj-$(CONFIG_LEDS_LP50XX) += leds-lp50xx.o >> obj-$(CONFIG_LEDS_LP55XX_COMMON) += leds-lp55xx-common.o >> obj-$(CONFIG_LEDS_LP5521) += leds-lp5521.o >> obj-$(CONFIG_LEDS_LP5523) += leds-lp5523.o >> diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c >> new file mode 100644 >> index 000000000000..5a32410e4f34 > [...] >> +static int lp50xx_probe_dt(struct lp50xx *priv) >> +{ >> + u32 led_strings[LP5036_MAX_LED_MODULES]; >> + struct fwnode_handle *child = NULL; >> + struct fwnode_handle *led_node = NULL; >> + struct led_init_data init_data; >> + struct lp50xx_led *led; >> + int num_colors; >> + u32 color_id; >> + int led_number; >> + size_t i = 0; >> + int ret; >> + >> + priv->enable_gpio = devm_gpiod_get_optional(&priv->client->dev, >> + "enable", GPIOD_OUT_LOW); >> + if (IS_ERR(priv->enable_gpio)) { >> + ret = PTR_ERR(priv->enable_gpio); >> + dev_err(&priv->client->dev, "Failed to get enable gpio: %d\n", >> + ret); >> + return ret; >> + } >> + >> + priv->regulator = devm_regulator_get(&priv->client->dev, "vled"); >> + if (IS_ERR(priv->regulator)) >> + priv->regulator = NULL; >> + >> + if (priv->model_id == LP5009) >> + priv->max_leds = LP5009_MAX_LED_MODULES; >> + else if (priv->model_id == LP5012) >> + priv->max_leds = LP5012_MAX_LED_MODULES; >> + else if (priv->model_id == LP5018) >> + priv->max_leds = LP5018_MAX_LED_MODULES; >> + else if (priv->model_id == LP5024) >> + priv->max_leds = LP5024_MAX_LED_MODULES; >> + else if (priv->model_id == LP5030) >> + priv->max_leds = LP5030_MAX_LED_MODULES; >> + else >> + priv->max_leds = LP5036_MAX_LED_MODULES; > You could simplify that by initializing data property of > lp50xx_leds_match array elements and then call of_match_device. > You can compare e.g. drivers/leds/leds-pca9532.c
Well I am not using of_ calls in driver anymore.
I would prefer to use device_get_match_data(&client->dev);
>> + device_for_each_child_node(&priv->client->dev, child) { >> + led = &priv->leds[i]; >> + if (fwnode_property_present(child, "ti,led-bank")) { >> + ret = fwnode_property_read_u32_array(child, >> + "ti,led-bank", >> + NULL, 0); >> + ret = fwnode_property_read_u32_array(child, >> + "ti,led-bank", >> + led_strings, >> + ret); >> + >> + priv->num_of_banked_leds = ARRAY_SIZE(led_strings); >> + >> + ret = lp50xx_set_banks(priv, led_strings); >> + if (ret) { >> + dev_err(&priv->client->dev, >> + "Cannot setup banked LEDs\n"); >> + fwnode_handle_put(child); >> + goto child_out; >> + } >> + led->ctrl_bank_enabled = 1; >> + >> + } else { >> + ret = fwnode_property_read_u32(child, "reg", >> + &led_number); >> + >> + led->led_number = led_number; >> + } >> + if (ret) { >> + dev_err(&priv->client->dev, >> + "led sourcing property missing\n"); >> + fwnode_handle_put(child); >> + goto child_out; >> + } >> + >> + if (led_number > priv->max_leds) { >> + dev_err(&priv->client->dev, >> + "led-sources property is invalid\n"); >> + ret = -EINVAL; >> + fwnode_handle_put(child); >> + goto child_out; >> + } >> + >> + init_data.fwnode = child; >> + init_data.devicename = priv->client->name; > We'd rather not have devicename in the name of this LED class device > according to the new LED naming convention. It's not a hot-pluggable > device, so for properly designed DT we should not encounter name > clash.
Ok I had this code from the previous led label patchset.
I will fix it
> >> + init_data.default_label = ":"; > default_label should not be set for new drivers. See the comment in > the include/linux/leds.h: > > /* > * string to be used for devicename section of LED class device > * either for label based LED name composition path or for fwnode > * based when devname_mandatory is true > */ > > This is new driver and we should not add any convenience fallbacks > for label based DT bindings. Label DT property has been marked > deprecated, so for new drivers it should be deemed non-existent. > Same as above
>> + >> + fwnode_property_read_string(child, "linux,default-trigger", >> + &led->led_dev.default_trigger); >> + num_colors = 0; >> + >> + fwnode_for_each_child_node(child, led_node) { >> + ret = fwnode_property_read_u32(led_node, "color", >> + &color_id); >> + if (ret) >> + dev_err(&priv->client->dev, >> + "Cannot read color\n"); >> + >> + led->mc_cdev.available_colors |= (1 << color_id); >> + num_colors++; >> + >> + } >> + >> + led->priv = priv; >> + led->mc_cdev.ops = &lp50xx_mc_ops; >> + led->mc_cdev.num_leds = num_colors; >> + led->mc_cdev.led_cdev = &led->led_dev; >> + led->led_dev.brightness_set_blocking = lp50xx_brightness_set; >> + led->led_dev.brightness_get = lp50xx_brightness_get; >> + ret = led_classdev_multicolor_register_ext(&priv->client->dev, >> + &led->mc_cdev, >> + &init_data); > Why not devm_* prefixed version?
I did create a devm_ version like you did for flash I will change it over
Dan
> > [...] >
| |