Re: [PATCH v3 3/3] DT: leds: Add Qualcomm Light Pulse Generator binding

On Wed 22 Nov 12:42 PST 2017, Jacek Anaszewski wrote:

On 11/20/2017 10:45 PM, Bjorn Andersson wrote:

quoted

On Mon 20 Nov 12:35 PST 2017, Jacek Anaszewski wrote:

quoted

On 11/20/2017 08:58 PM, Bjorn Andersson wrote:

quoted

On Sun 19 Nov 13:35 PST 2017, Jacek Anaszewski wrote:

quoted

Hi Bjorn,

Thanks for the update.

On 11/15/2017 08:13 AM, Bjorn Andersson wrote:

quoted

This adds the binding document describing the three hardware blocks
related to the Light Pulse Generator found in a wide range of Qualcomm
PMICs.

Signed-off-by: Bjorn Andersson <redacted>
---

Changes since v2:
- Squashed all things into one node
- Removed quirks from the binding, compatible implies number of channels, their
  configuration etc.
- Binding describes LEDs connected as child nodes
- Support describing multi-channel LEDs
- Change style of the binding document, to match other LED bindings

Changes since v1:
- Dropped custom pattern properties
- Renamed cell-index to qcom,lpg-channel to clarify its purpose

 .../devicetree/bindings/leds/leds-qcom-lpg.txt     | 66 ++++++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt
new file mode 100644
index 000000000000..9cee6f9f543c
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.txt

@@ -0,0 +1,66 @@
+Binding for Qualcomm Light Pulse Generator
+
+The Qualcomm Light Pulse Generator consists of three different hardware blocks;
+a ramp generator with lookup table, the light pulse generator and a three
+channel current sink. These blocks are found in a wide range of Qualcomm PMICs.
+
+Required properties:
+- compatible: one of:
+	      "qcom,pm8916-pwm",
+	      "qcom,pm8941-lpg",
+	      "qcom,pm8994-lpg",
+	      "qcom,pmi8994-lpg",
+	      "qcom,pmi8998-lpg",
+
+Optional properties:
+- qcom,power-source: power-source used to drive the output, as defined in the
+		     datasheet. Should be specified if the TRILED block is
+		     present

Range of possible values is missing here.

There seems to be a 4-way mux in all variants, but the wiring is
different in the different products. E.g. in pm8941 1 represents VPH_PWR
while in pmi8994 this is pulled from a dedicated pin named VIN_RGB.

Would you like me to list the 4 options for each compatible?

Could you please explain why user would prefer one power source
over the other? Is it that they have different max current limit?

The mux in pm8941 is connected to ground (0V), vph_pwr (3.6V), internal
5V and min(5V, charger). In pmi8994 it's ground, vdd_rgb (a dedicated
pin) and 4.2V. PMI8998 is a slight variation of PMI8994 and I expect
there to be more variants.

So it's different voltage level and, potentially, current limit.

I'd replace this property with led-max-microamp
(see Documentation/devicetree/bindings/leds/common.txt) and let
the driver to decide which power source to choose, basing on that limit.

Unfortunately I don't think specifying this in uA is possible, at least
some of these inputs does not have a knows (at least not platform
defined) current limit.

Like in the case on PM8941, the knobs this property tweaks is: "should
we output 3.6V or 5V" and both values depend on external power-supplies.

For most cases specifying this as led-microvolt would be possible, but
then there are the levels that are min(5V, charger).


One thing we've done in some other drivers with similar "enum" like
types is to provide an dt-bindings include file with these constants.
This makes the dts self documented and doesn't require additional
translation of the values.

quoted

[..]

quoted

One more question regarding TRILED - in your design it will be
exposed as a single LED class device with one brightness file,
right? Does it mean that all three LEDs will be applied the
same brightness after writing it to the sysfs file?

Correct, each LED described in DT will become one LED and can have more
than one (any number of) physical channel associated. The current
implementation applies the same brightness (and pattern) to all channels
associated with a LED.

The rgb DT node name would be a bit misleading in this case, since
RGB usually implies the possibility of having different intensity
of each color.

In the sense of the devicetree this is exactly what it describes, the
fact that we haven't figured out a way to implement this is, in my view,
a separate topic.

quoted

The open question is still how to pass a color from user space, the
brightness_set and pattern_set would need to be modified to map a list
of brightnesses to the individual channels or to adapt the brightness by
some color-modifier(?).

Pavel made and attempt of reworking Heiner Kallweit's HSV approach
few months ago [0]. You can take a look and share your opinion
or even continue this effort.

I did not consider using HSV to get around the problem of everything
operating on "brightness", but this seems like a quite nice solution.

In the case of lpg_brightness_set() this would map nicely into the case
where a LED is either a single channel or three channels, and until we
land those patches the driver would just implement H = S = 0.

And for the pattern setting, we can retain the proposed interface of
pattern being a sequence of brightness/delay values and then map this in
the driver as we apply the patterns.

Regards,
Bjorn