[PATCH v7 3/3] leds/powernv: Add driver for PowerNV platform
Jacek Anaszewski
jacek.anaszewski at gmail.com
Thu Jul 23 22:26:14 AEST 2015
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
More information about the Linuxppc-dev
mailing list