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

[Vasant Hegde <hegdevasant@linux.vnet.ibm.com>]

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?

-Vasant