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