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
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
