diff mbox series

[v15,1/8] dt-bindings: mfd: Initial commit of silergy, sy7636a.yaml

Message ID 20211110122948.188683-2-alistair@alistair23.me (mailing list archive)
State New, archived
Headers show
Series Add support for the silergy,sy7636a | expand

Commit Message

Alistair Francis Nov. 10, 2021, 12:29 p.m. UTC 
Initial support for the Silergy SY7636A Power Management chip
and regulator.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
---
 .../bindings/mfd/silergy,sy7636a.yaml         | 79 +++++++++++++++++++
 1 file changed, 79 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml

Comments

Andreas Kemnade Nov. 17, 2021, 9:39 p.m. UTC | #1 
On Wed, 10 Nov 2021 22:29:41 +1000
Alistair Francis <alistair@alistair23.me> wrote:

> Initial support for the Silergy SY7636A Power Management chip
> and regulator.
> 
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> ---
>  .../bindings/mfd/silergy,sy7636a.yaml         | 79 +++++++++++++++++++
>  1 file changed, 79 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
> new file mode 100644
> index 000000000000..0566f9498e2f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
[...]
> +  regulators:
> +    type: object
> +
> +    properties:
> +      compatible:
> +        const: silergy,sy7636a-regulator
> +
> +      vcom:
> +        type: object
> +        $ref: /schemas/regulator/regulator.yaml#
> +        properties:
> +          regulator-name:
> +            const: vcom
> +
hmm, this is what? If I understand it correctly, vcom means some
voltage for compensation. On other comparable pmics (e.g. TPS65185
which has also a sane public datasheet, MAX17135) I have seen some
methods to measure a voltage while the display is doing something
defined and then program this voltage non-volatile for compensation
during manufacturing.

If I understand the code correctly all the bunch of voltages are
powered up if this one is enabled.
So at least a description should be suitable.

The other comparable PMICs have at least regulators named VCOM, DISPLAY
(controls several regulators, started with delays configured via
registers) and V3P3. MAX17135 source can be found in NXP kernels, 
TPS65185 in Kobo vendor kernels.

So I would expect to see something similar here and a description or at
least not such a misleading name as vcom if it is for some reason not
feasible to separate the regulators.

Regards,
Andreas
Alistair Francis Nov. 23, 2021, 1:29 p.m. UTC | #2 
On Thu, Nov 18, 2021 at 7:40 AM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> On Wed, 10 Nov 2021 22:29:41 +1000
> Alistair Francis <alistair@alistair23.me> wrote:
>
> > Initial support for the Silergy SY7636A Power Management chip
> > and regulator.
> >
> > Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  .../bindings/mfd/silergy,sy7636a.yaml         | 79 +++++++++++++++++++
> >  1 file changed, 79 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
> > new file mode 100644
> > index 000000000000..0566f9498e2f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
> [...]
> > +  regulators:
> > +    type: object
> > +
> > +    properties:
> > +      compatible:
> > +        const: silergy,sy7636a-regulator
> > +
> > +      vcom:
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +        properties:
> > +          regulator-name:
> > +            const: vcom
> > +
> hmm, this is what? If I understand it correctly, vcom means some
> voltage for compensation. On other comparable pmics (e.g. TPS65185
> which has also a sane public datasheet, MAX17135) I have seen some
> methods to measure a voltage while the display is doing something
> defined and then program this voltage non-volatile for compensation
> during manufacturing.
>
> If I understand the code correctly all the bunch of voltages are
> powered up if this one is enabled.
> So at least a description should be suitable.
>
> The other comparable PMICs have at least regulators named VCOM, DISPLAY
> (controls several regulators, started with delays configured via
> registers) and V3P3. MAX17135 source can be found in NXP kernels,
> TPS65185 in Kobo vendor kernels.
>
> So I would expect to see something similar here and a description or at
> least not such a misleading name as vcom if it is for some reason not
> feasible to separate the regulators.

This is a vcom in the sense of voltage for compensation. We just
currently don't support setting the vcom.

I had a look at the Kobo code and this is similar to
https://github.com/akemnade/linux/blob/kobo/epdc-pmic-5.15/drivers/regulator/sy7636-regulator.c#L614

So I think that vcom is still the appropriate name for this.

Alistair

>
> Regards,
> Andreas
Andreas Kemnade Nov. 25, 2021, 10:59 p.m. UTC | #3 
On Tue, 23 Nov 2021 23:29:26 +1000
Alistair Francis <alistair23@gmail.com> wrote:

> On Thu, Nov 18, 2021 at 7:40 AM Andreas Kemnade <andreas@kemnade.info> wrote:
> >
> > On Wed, 10 Nov 2021 22:29:41 +1000
> > Alistair Francis <alistair@alistair23.me> wrote:
> >  
> > > Initial support for the Silergy SY7636A Power Management chip
> > > and regulator.
> > >
> > > Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  .../bindings/mfd/silergy,sy7636a.yaml         | 79 +++++++++++++++++++
> > >  1 file changed, 79 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
> > > new file mode 100644
> > > index 000000000000..0566f9498e2f
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml  
> > [...]  
> > > +  regulators:
> > > +    type: object
> > > +
> > > +    properties:
> > > +      compatible:
> > > +        const: silergy,sy7636a-regulator
> > > +
> > > +      vcom:
> > > +        type: object
> > > +        $ref: /schemas/regulator/regulator.yaml#
> > > +        properties:
> > > +          regulator-name:
> > > +            const: vcom
> > > +  
> > hmm, this is what? If I understand it correctly, vcom means some
> > voltage for compensation. On other comparable pmics (e.g. TPS65185
> > which has also a sane public datasheet, MAX17135) I have seen some
> > methods to measure a voltage while the display is doing something
> > defined and then program this voltage non-volatile for compensation
> > during manufacturing.
> >
> > If I understand the code correctly all the bunch of voltages are
> > powered up if this one is enabled.
> > So at least a description should be suitable.
> >
> > The other comparable PMICs have at least regulators named VCOM, DISPLAY
> > (controls several regulators, started with delays configured via
> > registers) and V3P3. MAX17135 source can be found in NXP kernels,
> > TPS65185 in Kobo vendor kernels.
> >
> > So I would expect to see something similar here and a description or at
> > least not such a misleading name as vcom if it is for some reason not
> > feasible to separate the regulators.  
> 
> This is a vcom in the sense of voltage for compensation. We just
> currently don't support setting the vcom.
> 
> I had a look at the Kobo code and this is similar to
> https://github.com/akemnade/linux/blob/kobo/epdc-pmic-5.15/drivers/regulator/sy7636-regulator.c#L614
> 
> So I think that vcom is still the appropriate name for this.
> 
seems that you did not get me. If I understand the code behind it
correctly, it turns on all power rails (the +-15V stuff, VEE and so on)
with the defined delays, not just vcom because it sets
SY7636A_OPERATION_MODE_CRL_ONOFF. Controlling VCOM separately is possible
by using SY7636A_OPERATION_MODE_CRL_VCOMCTL in combintion with a
vcom_en gpio.

I do not see a reason to turn on vcom only without the other higher
voltage rails, so the behaviour is not necessarily wrong but if I read
the binding documentation I would expect that just vcom is turned on.
That is the mismatch I am talking about.

If we agree on this idea that one regulator is enabling everything, I
would adapt my EPDC drm driver and tps65185 driver (which are both in
my clean up to be upstreamable-queue). 

Just another thing to compare with:
https://github.com/Freescale/linux-fslc/blob/4.1-2.0.x-imx/drivers/regulator/max17135-regulator.c
that seems to be the starting point for kobo vendor kernel epd pmics.
They seem to have taken the source and modified things. There we have
regulators with empty ops for that step-up converted stuff, a separate
vcom and a display regulator which really controls all of these step-up
things.

Regards,
Andreas
Alistair Francis Nov. 29, 2021, 11:41 a.m. UTC | #4 
On Fri, Nov 26, 2021 at 8:59 AM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> On Tue, 23 Nov 2021 23:29:26 +1000
> Alistair Francis <alistair23@gmail.com> wrote:
>
> > On Thu, Nov 18, 2021 at 7:40 AM Andreas Kemnade <andreas@kemnade.info> wrote:
> > >
> > > On Wed, 10 Nov 2021 22:29:41 +1000
> > > Alistair Francis <alistair@alistair23.me> wrote:
> > >
> > > > Initial support for the Silergy SY7636A Power Management chip
> > > > and regulator.
> > > >
> > > > Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > > Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  .../bindings/mfd/silergy,sy7636a.yaml         | 79 +++++++++++++++++++
> > > >  1 file changed, 79 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
> > > > new file mode 100644
> > > > index 000000000000..0566f9498e2f
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
> > > [...]
> > > > +  regulators:
> > > > +    type: object
> > > > +
> > > > +    properties:
> > > > +      compatible:
> > > > +        const: silergy,sy7636a-regulator
> > > > +
> > > > +      vcom:
> > > > +        type: object
> > > > +        $ref: /schemas/regulator/regulator.yaml#
> > > > +        properties:
> > > > +          regulator-name:
> > > > +            const: vcom
> > > > +
> > > hmm, this is what? If I understand it correctly, vcom means some
> > > voltage for compensation. On other comparable pmics (e.g. TPS65185
> > > which has also a sane public datasheet, MAX17135) I have seen some
> > > methods to measure a voltage while the display is doing something
> > > defined and then program this voltage non-volatile for compensation
> > > during manufacturing.
> > >
> > > If I understand the code correctly all the bunch of voltages are
> > > powered up if this one is enabled.
> > > So at least a description should be suitable.
> > >
> > > The other comparable PMICs have at least regulators named VCOM, DISPLAY
> > > (controls several regulators, started with delays configured via
> > > registers) and V3P3. MAX17135 source can be found in NXP kernels,
> > > TPS65185 in Kobo vendor kernels.
> > >
> > > So I would expect to see something similar here and a description or at
> > > least not such a misleading name as vcom if it is for some reason not
> > > feasible to separate the regulators.
> >
> > This is a vcom in the sense of voltage for compensation. We just
> > currently don't support setting the vcom.
> >
> > I had a look at the Kobo code and this is similar to
> > https://github.com/akemnade/linux/blob/kobo/epdc-pmic-5.15/drivers/regulator/sy7636-regulator.c#L614
> >
> > So I think that vcom is still the appropriate name for this.
> >
> seems that you did not get me. If I understand the code behind it
> correctly, it turns on all power rails (the +-15V stuff, VEE and so on)
> with the defined delays, not just vcom because it sets
> SY7636A_OPERATION_MODE_CRL_ONOFF. Controlling VCOM separately is possible
> by using SY7636A_OPERATION_MODE_CRL_VCOMCTL in combintion with a
> vcom_en gpio.
>
> I do not see a reason to turn on vcom only without the other higher
> voltage rails, so the behaviour is not necessarily wrong but if I read
> the binding documentation I would expect that just vcom is turned on.
> That is the mismatch I am talking about.

Ah! Ok I understand. I'll rename it to vdd then.

Alistair

>
> If we agree on this idea that one regulator is enabling everything, I
> would adapt my EPDC drm driver and tps65185 driver (which are both in
> my clean up to be upstreamable-queue).
>
> Just another thing to compare with:
> https://github.com/Freescale/linux-fslc/blob/4.1-2.0.x-imx/drivers/regulator/max17135-regulator.c
> that seems to be the starting point for kobo vendor kernel epd pmics.
> They seem to have taken the source and modified things. There we have
> regulators with empty ops for that step-up converted stuff, a separate
> vcom and a display regulator which really controls all of these step-up
> things.
>
> Regards,
> Andreas
Andreas Kemnade Dec. 1, 2021, 10:35 p.m. UTC | #5 
Hi,

[...]
> > > This is a vcom in the sense of voltage for compensation. We just
> > > currently don't support setting the vcom.
> > >
> > > I had a look at the Kobo code and this is similar to
> > > https://github.com/akemnade/linux/blob/kobo/epdc-pmic-5.15/drivers/regulator/sy7636-regulator.c#L614
> > >
> > > So I think that vcom is still the appropriate name for this.
> > >  
> > seems that you did not get me. If I understand the code behind it
> > correctly, it turns on all power rails (the +-15V stuff, VEE and so on)
> > with the defined delays, not just vcom because it sets
> > SY7636A_OPERATION_MODE_CRL_ONOFF. Controlling VCOM separately is possible
> > by using SY7636A_OPERATION_MODE_CRL_VCOMCTL in combintion with a
> > vcom_en gpio.
> >
> > I do not see a reason to turn on vcom only without the other higher
> > voltage rails, so the behaviour is not necessarily wrong but if I read
> > the binding documentation I would expect that just vcom is turned on.
> > That is the mismatch I am talking about.  
> 
> Ah! Ok I understand. I'll rename it to vdd then.
> 
Most important is IMHO some human-readable description in the bindings
document.

I am also just wondering whether this kind of logical
regulator which turns on several other regulators is actually accepted
or just slipped through review. I have no strong opinion here. I just
want to be able to clean up the tps65185 driver in the same way and not
having two similar pmics with different bindings and then a mess at the
consumer side. 

Regards,
Andreas
Alistair Francis Dec. 2, 2021, 11:45 a.m. UTC | #6 
On Thu, Dec 2, 2021 at 8:36 AM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> Hi,
>
> [...]
> > > > This is a vcom in the sense of voltage for compensation. We just
> > > > currently don't support setting the vcom.
> > > >
> > > > I had a look at the Kobo code and this is similar to
> > > > https://github.com/akemnade/linux/blob/kobo/epdc-pmic-5.15/drivers/regulator/sy7636-regulator.c#L614
> > > >
> > > > So I think that vcom is still the appropriate name for this.
> > > >
> > > seems that you did not get me. If I understand the code behind it
> > > correctly, it turns on all power rails (the +-15V stuff, VEE and so on)
> > > with the defined delays, not just vcom because it sets
> > > SY7636A_OPERATION_MODE_CRL_ONOFF. Controlling VCOM separately is possible
> > > by using SY7636A_OPERATION_MODE_CRL_VCOMCTL in combintion with a
> > > vcom_en gpio.
> > >
> > > I do not see a reason to turn on vcom only without the other higher
> > > voltage rails, so the behaviour is not necessarily wrong but if I read
> > > the binding documentation I would expect that just vcom is turned on.
> > > That is the mismatch I am talking about.
> >
> > Ah! Ok I understand. I'll rename it to vdd then.
> >
> Most important is IMHO some human-readable description in the bindings
> document.

That is what I ended up going with instead.

Alistair

>
> I am also just wondering whether this kind of logical
> regulator which turns on several other regulators is actually accepted
> or just slipped through review. I have no strong opinion here. I just
> want to be able to clean up the tps65185 driver in the same way and not
> having two similar pmics with different bindings and then a mess at the
> consumer side.
>
> Regards,
> Andreas
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
new file mode 100644
index 000000000000..0566f9498e2f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
@@ -0,0 +1,79 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/silergy,sy7636a.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: silergy sy7636a PMIC
+
+maintainers:
+  - Alistair Francis <alistair@alistair23.me>
+
+properties:
+  compatible:
+    const: silergy,sy7636a
+
+  reg:
+    description:
+      I2C device address.
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  '#thermal-sensor-cells':
+    const: 0
+
+  epd-pwr-good-gpios:
+    description:
+      Specifying the power good GPIOs.
+    maxItems: 1
+
+  regulators:
+    type: object
+
+    properties:
+      compatible:
+        const: silergy,sy7636a-regulator
+
+      vcom:
+        type: object
+        $ref: /schemas/regulator/regulator.yaml#
+        properties:
+          regulator-name:
+            const: vcom
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - '#thermal-sensor-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      pmic@62 {
+        compatible = "silergy,sy7636a";
+        reg = <0x62>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&pinctrl_epdpmic>;
+        #thermal-sensor-cells = <0>;
+
+        regulators {
+          reg_epdpmic: vcom {
+            regulator-name = "vcom";
+            regulator-boot-on;
+          };
+        };
+      };
+    };
+...