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

Vasant,

On 21.07.2015 08:55, Vasant Hegde wrote:

On 07/21/2015 11:24 AM, Vasant Hegde wrote:

quoted

On 07/20/2015 03:10 AM, Jacek Anaszewski wrote:

quoted

Hi Vasant,

Jacek,

quoted

I've revised your patch and found few more issues.
Please refer to my comments below.

Thanks.

.../...

quoted

quoted

quoted

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.
.../...

quoted

quoted

quoted

quoted

+/*
+ * 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.

quoted

quoted

quoted

quoted

+    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().

OK, this looks good to me.

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.

led_classdev_unregister calls cancel_work_sync so brightness_set op
will not be called after driver detach.

 >  Does new workqueue approach has sleep functionality? Is there

way to make sure work is completed before
unloading module?

It is approached from the other side - we are making sure that
work will not be executed after driver detach.

-- 
Best Regards,
Jacek Anaszewski