lkml.org 
[lkml]   [2018]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 10/24] leds: lp8860: Use led_compose_name()
    From
    Date
    Jacek

    On 11/08/2018 02:48 PM, Jacek Anaszewski wrote:
    > Dan,
    >
    > On 11/08/2018 07:16 PM, Dan Murphy wrote:
    >> On 11/06/2018 04:07 PM, Jacek Anaszewski wrote:
    >>> Switch to using generic LED support for composing LED class
    >>> device name.
    >>>
    >>> Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
    >>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
    >>> ---
    >>> 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 39c72a9..7c12ccd 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);
    >>> - }
    >>
    >> We need to keep a loop here looking for additional nodes as this device has string support and just
    >> not cluster support.
    >>
    >> Just need to get the other LED patches done.
    >
    > In the current shape only the properties from the last parsed child node
    > will be used for initializing the LED class device. So, this should be
    > meant also a fix removing this obscurity. Worth mentioning in the commit
    > message. Or I'm not getting it right?


    Yes I agree as this driver in the current shape only supports cluster mode only not string support.
    I have some stashed patches locally that register each string individually for this device.

    I have not worked out a good algorithm to set the register appropriately.

    If you want to remove the loop then I can add it back once I add string support.

    Dan

    >
    >>> + child_node = of_get_next_available_child(np, NULL);
    >>> + 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);
    >>
    >> Should we use the #defines here for display_cluster?
    >
    > Right, will fix in the next version.
    >
    >>> + 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;
    >>>
    >>
    >>
    >


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

    \
     
     \ /
      Last update: 2018-11-12 15:07    [W:5.044 / U:0.052 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site