Messages in this thread | | | Date | Thu, 3 Nov 2022 11:19:08 +0800 | Subject | Re: [PATCH v4 1/2] leds: flash: add driver to support flash LED module in QCOM PMICs | From | Fenglin Wu <> |
| |
>>>>>>>>> led_classdev_flash >>>>>>>>>> >>>>>>>>> *fled_cdev, u32 brightness) >>>>>>>>> +{ >>>>>>>>> + struct qcom_flash_led *led = >>>>>>>>> container_of(fled_cdev, >>>>>>>>> struct >>>>>>>>>> qcom_flash_led, flash); >>>>>>>>> + >>>>>>>>> + led->flash_current_ma = min_t(u32, led- >>>>>>>>>> max_flash_current_ma, > brightness / 1000); >>>>>>>>> + return 0; >>>>>>>>> +} >>>>>>> >>>>>>> This doesn't seem to work in torch mode for me on PMI8998. >>>>>>> If >>>>>>> the >>>>>>> brightness is 0, the torch is OFF as expected, but when the >>>>>>> torch >>>>>>> is >>>>>>> ON, you cannot control the brightness. The brightness is >>>>>>> the >>>>>>> same >>>>>>> for >>>>>>> [1, 255]. If the hardware cannot change the brightness in >>>>>>> torch >>>>>>> mode, >>>>>>> it would be nice to report a max_brightness value of 1 >>>>>>> instead >>>>>>> of >>>>>>> 255 >>>>>>> for userspace. This could be a limitation of the PMI8998, >>>>>>> not >>>>>>> sure. >>>>>>> Do you have any insights here? This driver is already 99% >>>>>>> working >>>>>>> for >>>>>>> SDM845+PMI8998. >>>>>>> >>>>>>> Flash mode works great, timeout & brightness can be >>>>>>> configured >>>>>>> just >>>>>>> fine. >>>>>>> >>>>>> >>>>>> Here, function qcom_flash_brightness_set() is used for >>>>>> setting >>>>>> brightness in flash mode and it was supposed to be working >>>>>> with >>>>>> following commands combination: >>>>>> echo xxx > flash_brightness >>>>>> echo 1 > flash_strobe (you will need to toggle >>>>>> flash_strobe >>>>>> when you >>>>>> enabling it again) >>>>>> >>>>>> Can you check if you can see the brightness change in flash >>>>>> mode >>>>>> when >>>>>> you updating the flash_brightness value with these commands? >>>>>> >>>>>> You can echo any value between [0, max_flash_brightness] into >>>>>> the >>>>>> flash_brightness, there is a "max_flash_brightness" sysfs >>>>>> node >>>>>> and >>>>>> the >>>>>> value comes from DT property "flash-max-microamp", ex. my >>>>>> dtsi >>>>>> node >>>>>> has >>>>>> "flash-max-microamp = <2000000>;" defined so I am having >>>>>> following >>>>>> value >>>>>> for max_flash_brightness: >>>>>> >>>>>> kalama:/sys/class/leds/white:flash-0 # cat >>>>>> max_flash_brightness >>>>>> 2000000 >>>>>> >>>>>> And by default flash mode uses 12500uA resolution so you >>>>>> won't >>>>>> see >>>>>> brightness change when you update it with values between [1, >>>>>> 12500]. >>>>> >>>>> Yes, this works perfectly fine on PMI8998. I can adjust the >>>>> flash >>>>> brightness and flash timeout perfectly. >>>>> >>>>>> >>>>>> If you want to update the brightness for torch node, you can >>>>>> directly >>>>>> update the "brightness" node with values between [0, 255], >>>>>> and >>>>>> it's >>>>>> mapping to the torch current between [0, led-max-microamp]. >>>>>> "led-max-microamp" is also has value coming from the DT >>>>>> property. >>>>>> >>>>>> This worked at my side on both pm8150C and pm8550.I think it >>>>>> should >>>>>> work >>>>>> on PMI8998 as well because the flash module in it is very >>>>>> similar >>>>>> to >>>>>> the >>>>>> one in PM8150c. Let me know if you still see such issues at >>>>>> your >>>>>> side. >>>>>> Thanks >>>>> >>>>> Only here I have an issue with PMI8998: the "brightness" nodes >>>>> should >>>>> change the brightness of the torch, but it doesn't make a >>>>> difference >>>>> here. When I do: >>>>> >>>>> - echo 0 > brightness --> LED turns OFF >>>>> - echo 255 > brightness --> LED turns ON >>>>> - echo 100 > brightness --> LED is still ON, but brightness is >>>>> the >>>>> same >>>>> as with 255. >>>>> >>>>> Could it be that PMI8998 is slightly different here than with >>>>> PM8150c >>>>> for example? Maybe it doesn't support brightness for torch? >>>>> >>>>> Here's my DTS: >>>>> >>>>> *PMI8998* >>>>> >>>>> pmi8998_flash: led-controller@d300 { >>>>> compatible = "qcom,pm6150l-flash-led", "qcom,spmi- >>>>> flash- >>>>> led"; >>>>> reg = <0xd300>; >>>>> status = "disabled"; >>>>> }; >>>>> >>>>> *SDM845 SHIFT axolotl* >>>>> >>>>> &pmi8998_flash { >>>>> status = "okay"; >>>>> >>>>> led-0 { >>>>> function = LED_FUNCTION_FLASH; >>>>> color = <LED_COLOR_ID_WHITE>; >>>>> led-sources = <1>; >>>>> led-max-microamp = <180000>; >>>>> flash-max-microamp = <1000000>; >>>>> flash-max-timeout-us = <1280000>; >>>>> }; >>>>> >>>>> led-1 { >>>>> function = LED_FUNCTION_FLASH; >>>>> color = <LED_COLOR_ID_YELLOW>; >>>>> led-sources = <2>; >>>>> led-max-microamp = <180000>; >>>>> flash-max-microamp = <1000000>; >>>>> flash-max-timeout-us = <1280000>; >>>>> }; >>>>> }; >>>>> >>>> Thank you for getting back all the details. The devicetree node >>>> looks >>>> good to me. >>>> >>>> I checked again and confirmed that the flash modules in PMI8998 >>>> and >>>> PM8150C have the same register definition for ITARGETx (0xD343 + >>>> x) >>>> and >>>> IRESOLUTION (0xD347), these are the only 2 settings would impact >>>> LED >>>> brightness, and in torch mode, IRESOLUTION is fixed to 5mA and >>>> only >>>> ITARGET is updated accordingly. >>>> >>>> I updated my workspace to use the same current for torch mode as >>>> your >>>> settings in devicetree and tried again on my PM8150C device,I >>>> could >>>> notice the brightness change when echoing different values to the >>>> brightness sysfs node, however, when I updating values between >>>> 255 >>>> and >>>> 200, it wasn't a very noticeable brightness change with naked >>>> eys, >>>> but I >>>> could see the ITRAGETx register changed accordingly when reading >>>> its >>>> value back from the regmap debugfs node. >>>> >>>> Can you try the same and see if the register got updated >>>> accordingly >>>> when you updating brightness values? If yes, I would wonder if >>>> the >>>> LED >>>> component on your device has upper current limit in torch mode >>>> close >>>> to >>>> (100 / 255 ) * 180mA so you that you can't observe the brightness >>>> change >>>> when updating between 100 to 255. Another easier thing to try, >>>> can >>>> you >>>> echo a lower brightness value, such as 10, to see if you can >>>> notice >>>> the >>>> brightness change? >>>> >>> >>> Aha! It does seem to work partially! >>> >>> I tried it with brightness 0 --> 255 --> 1. This order keeps the >>> highest brightness at all times, matching 255. The other way >>> around: 0 >>> --> 1 --> 255 keeps the lowest brightness, matching 1. >>> Changing the brightness only works if the LED was turned OFF first: >>> >>> 0 --> 255 --> 0 --> 1 >>> 0 --> 1 --> 0 --> 255 >>> >>> both work fine, verified it with the following shell commands >>> (assumed >>> that the LED was turned OFF to start): >>> >>> echo 1 > brightness && sleep 3 && echo 0 > brightness && echo 255 > >>> brightness >>> echo 255 > brightness && sleep 3 && echo 0 > brightness && echo 1 >>>> >>> brightness >>> >>> Maybe a register needs to be reset when the brightness changes? >>> >> >> Hi Dylan, >> >> Would you mind to help me testing this small change on PMI8998? It >> simply toggles the CHANNEL_EN bit when updating LED brightness. If it >> works, I will add it specifically for PMI8998 in patch v5. >> >> @@ -241,8 +243,21 @@ static int set_flash_strobe(struct >> qcom_flash_led >> *led, enum led_strobe s >> trobe, >> chan_mask |= BIT(chan_id - 1); >> } >> >> - /* enable/disable flash channels */ >> mask = chan_mask; >> + /* >> + * For flash module inside PMI8998, if strobe(true) is called >> when >> + * the LED is already enabled, disable the channel 1st and >> then >> + * enable it again. This could happen when updating LED >> brightness >> + * after LED is turned on. >> + */ >> + if (led->enabled && (led->enabled == state)) { >> + rc = >> regmap_field_update_bits(chip->r_fields[REG_CHAN_EN], mask, 0); >> + if (rc < 0) >> + return rc; >> + } >> + >> + /* enable/disable flash channels */ >> val = state ? mask : 0; >> rc = regmap_field_update_bits(chip->r_fields[REG_CHAN_EN], >> mask, val); >> if (rc < 0) >> >> Thanks >> Fenglin Wu > > With this change I can indeed change the brightness. > However, it works now 50% of the time: > > - echo 255 > brightness --> LED turns ON, max brightness: 255 > - echo 1 > brightness --> LED turns OFF > - echo 1 > brightness --> LED turns ON, min brightness: 1 > > This behavior didn't occur before I applied this change. > I had a look at the change but I cannot really pinpoint why this is > happening... > > Kind regards, > Dylan Van Assche > Thanks Dylan. It seems much more complex than what I thought to support the flash module in PMI8998 with the same driver. Then I will not include PMI8998 support in patch v5 and I will consider to push a following patch after this driver got accepted, I will try if I can find a PMI8998 device and fully test the driver before I push that. Thanks
Fenglin Wu
| |