Re: [PATCH v4 1/2] leds: flash: add driver to support flash LED module in QCOM PMICs
From: Fenglin Wu
Date: Wed Nov 02 2022 - 23:19:43 EST
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