Re: [PATCH v7 3/3] leds/powernv: Add driver for PowerNV platform

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Vasant,

On 23.07.2015 10:08, Vasant Hegde wrote:
> On 07/23/2015 01:25 PM, Jacek Anaszewski wrote:
>> Hi Vasant,
>>
>
> Jacek,
>
> .../...
>
>>> +/* PowerNV LED data */
>>> +struct powernv_led_data {
>>> +    struct led_classdev    cdev;
>>> +    char            *loc_code;    /* LED location code */
>>> +    int            led_type;    /* OPAL_SLOT_LED_TYPE_* */
>>> +    enum led_brightness    value;        /* Brightness value */
>>
>> You don't need 'value' as brightness value is already stored in
>> cdev.brightness.
>>
>
> Agree. I'll remove.
>
>>> +/*
>>> + * LED classdev 'brightness_get' function. This schedules work
>>> + * to update LED state.
>>> + */
>>> +static void powernv_brightness_set(struct led_classdev *led_cdev,
>>> +                   enum led_brightness value)
>>> +{
>>> +    struct powernv_led_data *powernv_led =
>>> +        container_of(led_cdev, struct powernv_led_data, cdev);
>>> +
>>> +    /* Do not modify LED in unload path */
>>> +    if (led_disabled)
>>> +        return;
>>> +
>>> +    /* Prepare the request */
>>> +    powernv_led->value = value;
>>> +
>>> +     return powernv_led_set(powernv_led);
>>
>> Isn't mutex_lock/unlock missing here?
>
> Yes. I realized this after sending out the patch. I will fix this.
> .../...
>
>>> +
>>> +    max_led_type = devm_kzalloc(dev, sizeof(*max_led_type), GFP_KERNEL);
>>> +    if (!max_led_type)
>>> +        return -ENOMEM;
>>> +
>>> +    mutex_init(lock);
>>> +    *max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
>>> +
>>> +    platform_set_drvdata(pdev, lock);
>>
>> Setting only lock as drvdata looks odd to me and I haven't noticed
>> anyone doing that.
>> I'd prefer to put lock, led_disabled and max_led_type in a common
>> struct and set it as drvdata. I know that I accepted this design, but
>> I didn't take into account these details.
>
> Yeah. Even I looked into existing code and I don't see anyone using like this.
> Since it's void * and we just need lock, I added like this.
>
> If I break this into two structure, then I've to use platform_get_drvdata() call
> in multiple functions like powernv_brightness_set() to get max_let_type etc. Is
> that fine?

You could add wrappers for grouping instructions required to
retrieve given properties of the common struct.
Alternatively you could add a pointer to the common struct
in the struct powernv_led_data. You can play with these approaches
and choose the one which looks better.

-- 
Best Regards,
Jacek Anaszewski