Re: [PATCH v8 1/1] leds: LED driver for TI LP3952 6-Channel Color LED

[Tony <tony.makkiel@daqri.com>]

On 07/07/16 15:34, Jacek Anaszewski wrote:

>>>
>>> Why actually do you need this array? Couldn't you provide the LED
>>> identifier in the DSD entry corresponding to a LED? You can follow
>>> the scheme used in Device Tree by introducing "reg" property.
>>
>> The names in the array are the 6 identifiers in the DSD entry. For
>> example
>> the DSD property for blue2 led used is
>>
>> "Package ()
>> {
>>          Package (2) {"blue2", "led_blue2"},
>> "
>> acpi_dev_get_property will return "led_blue2" in obj->string.pointer.
>>
>> There isn't separate DSD for individual leds. There is only 1 DSD for
>> the led controller identified by "Name (_HID, "TXNW3952")". The
>> individual led names are separate properties withing th DSD (eg:
>> led_blue2 identified by blue2).
>
> OK, thanks for the explanation. BTW LED name pattern is
> devicename:colour:function.
>
>>>
>>>>>> +    for (i = 0; i < LP3952_LED_ALL; i++) {
>>>>>> +        priv->leds[i].cdev.brightness = LED_OFF;
>>>>>> +        priv->leds[i].cdev.max_brightness = LP3952_BRIGHT_MAX;
>>>>>> +        priv->leds[i].cdev.brightness_set_blocking =
>>>>>> +                lp3952_set_brightness;
>>>>>> +        priv->leds[i].channel = i;
>>>>>> +        priv->leds[i].priv = priv;
>>>>>> +
>>>>>> +        ret = lp3952_get_label(priv->leds[i].name, led_name_hdl[i],
>>>>>> +                       &priv->client->dev);
>>>>>> +        if (ret)
>>>>>> +            break;
>>>>>
>>>>> You're assuming here that all LEDs will always be present, which
>>>>> doesn't necessarily have to be true. How about just skipping
>>>>> the LED and moving to the next one if given label was not found?
>>>>> You could move this check to the beginning of the loop then.
>>>>
>>>> If we skip leds, there could be problem, if all the leds were
>>>> skipped(For example, no ACPI name data present).
>>>
>>> If all the LEDs were skipped, then no LED class device would be
>>> registered. In such a case you could return an error from
>>> lp3952_probe().
>>>
>>>>
>>>> At present, if a label is not present, the probe will fail.
>>>> How about reverting to a default name (eg: "lp3952:blue2") if led name
>>>> read fails?
>>>
>>> There is no point in registering the LED if its DSD entry is not
>>> valid. In case of Device Tree we fail LED registration in such
>>> cases.
>>
>> Sorry, I thought we were skipping LEDs due to invalid DSD. If that is
>> not the case, isn't the assumption all LEDS will be present true?
>> I am guessing leds might be missing, if some of the leds are not
>> physically connected to controller output. But the controller would
>> have 6 led controls all the time.
>
> Right, but there is no point in exposing corresponding
> LED class devices to user space if the user will be unable
> to notice any effect because the LED is not connected.

Got it, Thank you :) Will skip registration if led not found.
>
>>>
>>>> This would also cover cases in which none of the led names
>>>> were read properly.
>>>
>>> Whole driver probing should fail then.
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> +        priv->leds[i].cdev.name = priv->leds[i].name;
>>>>>> +
>>>>>> +        ret = devm_led_classdev_register(&priv->client->dev,
>>>>>> +                         &priv->leds[i].cdev);
>>>>>> +        if (ret < 0) {
>>>>>> +            dev_err(&priv->client->dev,
>>>>>> +                "couldn't register LED %s\n",
>>>>>> +                priv->leds[i].cdev.name);
>>>>>> +            break;
>>>>>> +        }
>>>>>> +    }
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe
>>>> linux-acpi" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>>
>>>
>>>
>>
>>
>
>