Re: [PATCH v2 0/4] Add support for QCOM SPMI Flash LEDs

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Hi Nicolas.

On 1/26/21 3:03 PM, Nícolas F. R. A. Prado wrote:
> Hi,
> 
> this patch series adds support for Qualcomm's SPMI Flash LEDs present in the
> PM8941 PMIC. It is used as part of MSM8974 based devices, like the Nexus 5
> (hammerhead), as a camera flash or as a lantern when in torch mode.
> 
> Patch 1 adds the dt-bindings for the driver, together with a header for the
> values of some properties.
> 
> Patch 2 adds the driver, which was ported from downstream [1], and is now using
> the flash LED class framework.
> 
> Patch 3 enables the driver as a module in qcom_defconfig, and also enables
> CONFIG_LEDS_CLASS_FLASH since it is required by the driver.
> 
> Patch 4 adds the device tree nodes configuring the driver in the pm8941 dtsi.
> 
> After the feedback I received from the v1 RFC patch (thank you Jacek and
> Bjorn!), I implemented the flash LED class framework, renamed the driver to
> qcom-spmi-flash and added the dt-bindings. I also did a whole lot of cleanup.
> 
> Some caveats:
> - I still didn't implement get_strobe() and get_fault() for the flash LEDs,
>    because I'm still not sure how to do it. get_strobe() in particular I'm not
>    even sure if is possible, since after the flash turns off automatically after
>    the timeout, I don't see any change in the SPMI registers. So I'm unsure how
>    one would get the current strobe state.

strobe_get is optional - you can leave it uninitialized if there is no
obvious way to get strobe status.

Regarding faults - I see you have FLASH_FAULT_DETECT but have no
information on its impact whatsoever. Usually devices report the faults
by settings some register bits and then we can map those errors to
LED flash framework generic errors.

> - I have yet to add the V4L2 flash wrapper for the flash LEDs. I still didn't do
>    it because I wasn't sure if it was needed, so wanted to double check. But
>    being a camera flash it seems that would be useful. Also, it would be great if
>    someone could point me how I would go about testing the flash usage through
>    V4L2.

You need a V4L2 media device driver with which this driver would
register a V4L2 flash LED sub-device. Such a device is usually
implemented for platform ISP devices. Provided it is present in the
mainline you would have to associate this driver DT node with the
media device DT node. Then you can test the V4L2 Flash control with
v4l2-ctl or yavta user space tools.

Let's skip the V4L2 support for now - it can be added later, if needed.

> Another thing worth mentioning: for v1 the dt nodes were added in hammerhead's
> dts (just to simplify testing), but I have now moved them to pm8941's dtsi,
> since it was like that in downstream. So if folks using devices based on
> PM8941/MSM8974 other than the Nexus 5 could test it, that would be great, since
> I have only tested on the Nexus 5.
> 
> v1 RFC: https://lore.kernel.org/lkml/20201106165737.1029106-1-nfraprado@protonmail.com/
> 
> [1] https://github.com/AICP/kernel_lge_hammerhead/blob/n7.1/drivers/leds/leds-qpnp.c
> 
> Nícolas F. R. A. Prado (4):
>    dt-bindings: leds: Add binding for qcom-spmi-flash
>    leds: Add driver for QCOM SPMI Flash LEDs
>    ARM: qcom_defconfig: Enable QCOM SPMI Flash LEDs
>    ARM: dts: qcom: pm8941: Add nodes for QCOM SPMI Flash LEDs
> 
>   .../bindings/leds/leds-qcom-spmi-flash.yaml   |   94 ++
>   arch/arm/boot/dts/qcom-pm8941.dtsi            |   38 +
>   arch/arm/configs/qcom_defconfig               |    2 +
>   drivers/leds/Kconfig                          |    8 +
>   drivers/leds/Makefile                         |    1 +
>   drivers/leds/leds-qcom-spmi-flash.c           | 1153 +++++++++++++++++
>   .../dt-bindings/leds/leds-qcom-spmi-flash.h   |   15 +
>   7 files changed, 1311 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-spmi-flash.yaml
>   create mode 100644 drivers/leds/leds-qcom-spmi-flash.c
>   create mode 100644 include/dt-bindings/leds/leds-qcom-spmi-flash.h
> 

-- 
Best regards,
Jacek Anaszewski