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

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

On 07/21/2015 11:24 AM, Vasant Hegde wrote:
> On 07/20/2015 03:10 AM, Jacek Anaszewski wrote:
>> Hi Vasant,
>>
> 
> Jacek,
> 
>> I've revised your patch and found few more issues.
>> Please refer to my comments below.
> 
> Thanks.
> 
> .../...
> 
>>>>
>>>> Please don't exceed 75 character line length limit.
>>>
>>> Ok. I will fix it.. But I thought 80 character is the limit.
>>
>> checkpatch.pl reports this.
> 
> Ah! I was running checkpatch.pl against source. Let me fix this.
> .../...
> 
>>>>> +/*
>>>>> + * LED set routines have been implemented as work queue tasks scheduled
>>>>> + * on the global work queue. Individual task calls OPAL interface to set
>>>>> + * the LED state which might sleep for some time.
>>>>> + */
>>>>> +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 */
>>>>> +    struct mutex        lock;
>>
>> You're unnecessarily adding mutex for each LED class device.
>> The shared resource to protect is here powernv led interface,
>> so one mutex will suffice.
> 
> 
> Ok. Let me move that to common structure.
> 
>>
>>
>>>>> +    struct work_struct    work_led;    /* LED update workqueue */
>>>>> +};
>>>>> +
>>>>> +struct powernv_leds_priv {
>>>>> +    int num_leds;
>>>>> +    struct powernv_led_data powernv_leds[];
>>>>> +};
>>
>> powernv_led_data doesn't have to be retained in the array. You access
>> the array elements only upon creation of LED class devices. When using
>> managed resource allocation you don't need to bother with freeing
>> resources, so you don't need to keep reference to the data.
>>
>> I propose to drop struct powernv_leds_priv and instead introduce
>> a structure that would aggregate common driver data like mutex,
>> led_disable and max_led_type.
> 
> I still think we need two structures.. One for common driver data like mutex,
> led_disable etc and other one for led data itself .. like
> 	struct powernv_led_data {
> 		struct led_classdev     cdev;
> 		char                    *loc_code;     <-- pointer to DT node
> 		int                     led_type;       /* OPAL_SLOT_LED_TYPE_* */
> 		enum led_brightness     value;          /* Brightness value */
> 	};
> 
> 	struct powernv_led_common {
> 		bool led_disable;
> 		int 	max_led_type;
> 		struct mutex            lock;
> 	};

Jacek,

Alternatively I can club both into single structure like below

 	struct powernv_led_data {
 		struct led_classdev     cdev;
 		char                    *loc_code;     <-- pointer to DT node
 		int                     led_type;       /* OPAL_SLOT_LED_TYPE_* */
 		enum led_brightness     value;          /* Brightness value */

 		int 		*max_led_type;
 		struct mutex            *lock;
 	};

	static  bool 	led_disable;


In this case I've to keep led_disable outside the structure as I need to access
this variable in powernv_led_remove().

One remaining issue with these approach (where we don't have array of
powernv_led ) is,
powernv_let_set() function can sleep. Current code (v6) calls flush_work before
unloading
module. That way we are sure work is complete before unloading module.
With new approach, I'm not sure how I can make sure work is completed before
loading module. Does new workqueue approach has sleep functionality? Is there
way to make sure work is completed before
unloading module?

-Vasant