From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
To: "Nícolas F. R. A. Prado" <nfraprado@protonmail.com>,
"Pavel Machek" <pavel@ucw.cz>, "Dan Murphy" <dmurphy@ti.com>,
"Bjorn Andersson" <bjorn.andersson@linaro.org>,
"Andy Gross" <agross@kernel.org>,
"Rob Herring" <robh+dt@kernel.org>
Cc: linux-leds@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
Brian Masney <masneyb@onstation.org>, Luca Weiss <luca@z3ntu.xyz>,
Russell King <linux@armlinux.org.uk>,
Georgi Djakov <georgi.djakov@linaro.org>,
linux-kernel@vger.kernel.org,
~postmarketos/upstreaming@lists.sr.ht,
lkcamp@lists.libreplanetbr.org, andrealmeid@collabora.com
Subject: Re: [PATCH v2 1/4] dt-bindings: leds: Add binding for qcom-spmi-flash
Date: Sat, 30 Jan 2021 21:32:23 +0100 [thread overview]
Message-ID: <597508ab-7d53-e1cc-a63f-b66e5dc7016d@gmail.com> (raw)
In-Reply-To: <20210126140240.1517044-2-nfraprado@protonmail.com>
Hi Nicolas,
On 1/26/21 3:04 PM, Nícolas F. R. A. Prado wrote:
> Add devicetree binding for QCOM SPMI Flash LEDs, which are part of
> PM8941, and are used both as lantern and camera flash.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@protonmail.com>
> ---
> Changes in v2:
> - Add this commit
>
> .../bindings/leds/leds-qcom-spmi-flash.yaml | 94 +++++++++++++++++++
> .../dt-bindings/leds/leds-qcom-spmi-flash.h | 15 +++
> 2 files changed, 109 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-spmi-flash.yaml
> create mode 100644 include/dt-bindings/leds/leds-qcom-spmi-flash.h
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-spmi-flash.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-spmi-flash.yaml
> new file mode 100644
> index 000000000000..169716e14f67
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-qcom-spmi-flash.yaml
> @@ -0,0 +1,94 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-qcom-spmi-flash.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm SPMI Flash LEDs
> +
> +maintainers:
> + - Nícolas F. R. A. Prado <nfraprado@protonmail.com>
> +
> +description: |
> + The Qualcomm SPMI Flash LEDs are part of Qualcomm PMICs and are used primarily
> + as a camera or video flash. They can also be used as a lantern when on torch
> + mode.
> + The PMIC is connected to Host processor via SPMI bus.
> +
> +properties:
> + compatible:
> + const: qcom,spmi-flash
> +
> + reg:
> + maxItems: 1
> +
> + flash-boost-supply:
> + description: SMBB regulator for LED flash mode
> +
> + torch-boost-supply:
> + description: SMBB regulator for LED torch mode
> +
> +patternProperties:
> + "^led[0-1]$":
> + type: object
> + $ref: common.yaml#
> +
> + properties:
> + qcom,clamp-curr:
> + description: current to clamp at, in uA
You're already using led-max-microamp and flash-max-microamp,
so I'm not sure how this is related to those.
> + $ref: /schemas/types.yaml#definitions/uint32
> +
> + qcom,headroom:
> + description: |
> + headroom to use. Use one of QCOM_SPMI_FLASH_HEADROOM_* defined in
> + include/dt-bindings/leds/leds-qcom-spmi-flash.h
More information would be welcome here on how it impacts device
operation.
> + $ref: /schemas/types.yaml#definitions/uint32
> + minimum: 0
> + maximum: 3
> +
> + qcom,startup-dly:
> + description: |
> + delay before flashing. Use one of QCOM_SPMI_FLASH_STARTUP_DLY_*
> + defined in include/dt-bindings/leds/leds-qcom-spmi-flash.h
I don't see much gain in having this exposed.
We don't have related constructs in the API to handle that, so I propose
to always set it to 0 and not expose this setting at all.
> + $ref: /schemas/types.yaml#definitions/uint32
> + minimum: 0
> + maximum: 3
> +
> + qcom,safety-timer:
> + description: include for safety timer use, otherwise watchdog timer will be used
> + type: boolean
What's the difference between watchdog timer and safety timer?
Either way, I propose to choose the option best fitting for handling
flash timeout setting and hide this inside the driver.
> +required:
> + - compatible
> + - reg
> + - flash-boost-supply
> + - torch-boost-supply
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/leds/common.h>
> + #include <dt-bindings/leds/leds-qcom-spmi-flash.h>
> +
> + qcom,leds@d300 {
> + compatible = "qcom,spmi-flash";
> + reg = <0xd300 0x100>;
> + flash-boost-supply = <&pm8941_5vs1>;
> + torch-boost-supply = <&pm8941_5v>;
> +
> + led0 {
> + led-sources = <0>;
Please use reg property for this purpose instead.
> + function = LED_FUNCTION_FLASH;
> + color = <LED_COLOR_ID_WHITE>;
> + led-max-microamp = <200000>;
> + flash-max-microamp = <1000000>;
> + flash-max-timeout-us = <1280000>;
You should mention this properties above and provide constraints
(min, max, step).
> + default-state = "off";
There's no point in having this in the example.
> + qcom,clamp-curr = <200000>; > + qcom,headroom = <QCOM_SPMI_FLASH_HEADROOM_500MV>;
> + qcom,startup-dly = <QCOM_SPMI_FLASH_STARTUP_DLY_128US>;
> + qcom,safety-timer;
> + };
> + };
> +...
> diff --git a/include/dt-bindings/leds/leds-qcom-spmi-flash.h b/include/dt-bindings/leds/leds-qcom-spmi-flash.h
> new file mode 100644
> index 000000000000..8bd54a8e831d
> --- /dev/null
> +++ b/include/dt-bindings/leds/leds-qcom-spmi-flash.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _DT_BINDINGS_LEDS_QCOM_SPMI_FLASH_H
> +#define _DT_BINDINGS_LEDS_QCOM_SPMI_FLASH_H
> +
> +#define QCOM_SPMI_FLASH_HEADROOM_250MV 0
> +#define QCOM_SPMI_FLASH_HEADROOM_300MV 1
> +#define QCOM_SPMI_FLASH_HEADROOM_400MV 2
> +#define QCOM_SPMI_FLASH_HEADROOM_500MV 3
> +
> +#define QCOM_SPMI_FLASH_STARTUP_DLY_10US 0
> +#define QCOM_SPMI_FLASH_STARTUP_DLY_32US 1
> +#define QCOM_SPMI_FLASH_STARTUP_DLY_64US 2
> +#define QCOM_SPMI_FLASH_STARTUP_DLY_128US 3
> +
> +#endif
>
--
Best regards,
Jacek Anaszewski
next prev parent reply other threads:[~2021-01-30 20:33 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-26 14:03 [PATCH v2 0/4] Add support for QCOM SPMI Flash LEDs Nícolas F. R. A. Prado
2021-01-26 14:04 ` [PATCH v2 1/4] dt-bindings: leds: Add binding for qcom-spmi-flash Nícolas F. R. A. Prado
2021-01-27 14:00 ` Rob Herring
2021-01-27 14:28 ` Bjorn Andersson
2021-01-30 20:32 ` Jacek Anaszewski [this message]
2021-01-26 14:05 ` [PATCH v2 2/4] leds: Add driver for QCOM SPMI Flash LEDs Nícolas F. R. A. Prado
2021-01-27 4:23 ` Bjorn Andersson
2021-01-30 20:37 ` Jacek Anaszewski
2021-02-19 11:02 ` Pavel Machek
2021-02-21 11:28 ` Jacek Anaszewski
2021-04-25 20:19 ` Pavel Machek
2021-02-19 11:01 ` Pavel Machek
2021-01-26 14:06 ` [PATCH v2 3/4] ARM: qcom_defconfig: Enable " Nícolas F. R. A. Prado
2021-01-27 14:29 ` Bjorn Andersson
2021-01-26 14:06 ` [PATCH v2 4/4] ARM: dts: qcom: pm8941: Add nodes for " Nícolas F. R. A. Prado
2021-01-27 14:32 ` Bjorn Andersson
2021-01-30 20:31 ` [PATCH v2 0/4] Add support " Jacek Anaszewski
2021-02-19 11:04 ` Pavel Machek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=597508ab-7d53-e1cc-a63f-b66e5dc7016d@gmail.com \
--to=jacek.anaszewski@gmail.com \
--cc=agross@kernel.org \
--cc=andrealmeid@collabora.com \
--cc=bjorn.andersson@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=dmurphy@ti.com \
--cc=georgi.djakov@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=lkcamp@lists.libreplanetbr.org \
--cc=luca@z3ntu.xyz \
--cc=masneyb@onstation.org \
--cc=nfraprado@protonmail.com \
--cc=pavel@ucw.cz \
--cc=robh+dt@kernel.org \
--cc=~postmarketos/upstreaming@lists.sr.ht \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).