Messages in this thread | | | Date | Tue, 01 Dec 2015 08:55:35 +0100 | From | Jacek Anaszewski <> | Subject | Re: [PATCH v6 2/2] leds: rt5033: Add RT5033 Flash led device driver |
| |
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@vger.kernel.org > 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
| |