Messages in this thread | | | Date | Thu, 26 Nov 2015 10:43:25 +0100 | From | Jacek Anaszewski <> | Subject | Re: [PATCH v6 2/2] leds: rt5033: Add RT5033 Flash led device driver |
| |
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.
>>> + >>> + 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.
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.
> >>> +#define RT5033_FLED_CTRL4_VTRREG_MAX 0x60 >> >> Rename DEF to MASK.
Hmm, here it should be: Rename MAX to MASK.
-- Best Regards, Jacek Anaszewski
| |