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)? 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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html