On 12/01/2015 02:54 AM, Ingi Kim wrote:
Hi Jacek,
On 2015년 11월 30일 19:59, Jacek Anaszewski wrote:
Hi Ingi,
On 11/30/2015 03:31 AM, Ingi Kim wrote:
Hi Jacek,
On 2015년 11월 26일 18:43, Jacek Anaszewski wrote:
Hi Ingi,
On 11/26/2015 09:02 AM, Ingi Kim wrote:
[...]
+torch_unlock:
+ mutex_unlock(&led->lock);
+ return ret;
+}
+
+static int rt5033_led_flash_brightness_set(struct led_classdev_flash *fled_cdev,
+ u32 brightness)
+{
+ struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
+ struct rt5033_led *led = sub_led_to_led(sub_led);
+
+ mutex_lock(&led->lock);
+ sub_led->flash_brightness = brightness;
+ mutex_unlock(&led->lock);
Mutex protection is redundant in this case. You would need it if device
state was also changed here.
Okay, I'll remove it.
BTW why flash brightness can't be written to the device here?
Flash brightness is only affected when FLED flashed (strobing).
So, I think it is better to be written in rt5033_led_flash_strobe_set function.
strobe_set op should strobe the flash ASAP, and delegating brightness
setting there extends a delay between calling strobe_set op
and actual flash strobe. If you set the brightness here, then you
wouldn't have to do that in the strobe_set op, of course unless the
the brightness is altered through the API of the other LED, in two
separate LEDs case.
The brightness may be able to change its brightness in two separate LEDs case as you see.
So, I think it would be better to write brightness setting in strobe_op.
Could you motivate your statement, please? Why would it be better?
In consideration of delay, of course, the brightness is set just when it would be changed.
I think that joint iout arrangement will be prevailing - this is the
case for your board, isn't it? With the modification I am proposing
the gain is clear.
You're right, thanks.
Did you mean that flash attributes should be written
on their ops(flash brightness, flash timeout)?
Both in those ops and conditionally in the strobe_set op,
in order to handle two LEDs case, when the other LED has
altered any of the shared settings.
let me update the driver on your suggestion.
+
+ return 0;
+}
+
+static int rt5033_led_flash_timeout_set(struct led_classdev_flash *fled_cdev,
+ u32 timeout)
+{
+ struct rt5033_sub_led *sub_led = flcdev_to_sub_led(fled_cdev);
+ struct rt5033_led *led = sub_led_to_led(sub_led);
+
+ mutex_lock(&led->lock);
+ sub_led->flash_timeout = timeout;
+ mutex_unlock(&led->lock);
Ditto.
Timeout should be also written here.
The timeout may be able to change its flash timeout in two separate LEDs case as you see.
So, I think it would be better to write timeout setting in strobe_op.
In consideration of delay, of course, the timeout is set just when it would be changed.
If you will add regmap_write in both ops, then mutex protection will
have to be preserved, to assure consistency between registers state
and sub_led->flash_brightness and sub_led->flash_timeout state.
Thanks, but mutex protection is useless in this case.
so I try to remove it.
+#define RT5033_FLED_CTRL4_VTRREG_MAX 0x60
Rename DEF to MASK.
Hmm, here it should be: Rename MAX to MASK.
Oh
Okay, Thanks :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
