[PATCH v2 1/2] dt-bindings: trivial-devices: add ltp8800

Cedric Encarnacion posted 2 patches 2 weeks, 4 days ago
[PATCH v2 1/2] dt-bindings: trivial-devices: add ltp8800
Posted by Cedric Encarnacion 2 weeks, 4 days ago
Add Analog Devices LTP8800-1A, LTP8800-2, and LTP8800-4A DC/DC μModule
regulator.

Signed-off-by: Cedric Encarnacion <cedricjustine.encarnacion@analog.com>
---
 Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
 MAINTAINERS                                            | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 90a7c0a3dc48..72877d00b8dd 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -43,6 +43,8 @@ properties:
           - adi,adp5589
             # Analog Devices LT7182S Dual Channel 6A, 20V PolyPhase Step-Down Silent Switcher
           - adi,lt7182s
+            # Analog Devices LTP8800-1A/-2/-4A 150A/135A/200A, 54V DC/DC μModule regulator
+          - adi,ltp8800
             # AMS iAQ-Core VOC Sensor
           - ams,iaq-core
             # Temperature monitoring of Astera Labs PT5161L PCIe retimer
diff --git a/MAINTAINERS b/MAINTAINERS
index 7c357800519a..6ca691500fb7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13555,6 +13555,11 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/light/liteon,ltr390.yaml
 F:	drivers/iio/light/ltr390.c
 
+LTP8800 HARDWARE MONITOR DRIVER
+M:	Cedric Encarnacion <cedricjustine.encarnacion@analog.com>
+L:	linux-hwmon@vger.kernel.org
+S:	Supported
+
 LYNX 28G SERDES PHY DRIVER
 M:	Ioana Ciornei <ioana.ciornei@nxp.com>
 L:	netdev@vger.kernel.org
-- 
2.39.5

Re: [PATCH v2 1/2] dt-bindings: trivial-devices: add ltp8800
Posted by Guenter Roeck 2 weeks, 4 days ago
On 11/5/24 19:09, Cedric Encarnacion wrote:
> Add Analog Devices LTP8800-1A, LTP8800-2, and LTP8800-4A DC/DC μModule
> regulator.
> 
> Signed-off-by: Cedric Encarnacion <cedricjustine.encarnacion@analog.com>
> ---
>   Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
>   MAINTAINERS                                            | 5 +++++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> index 90a7c0a3dc48..72877d00b8dd 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> @@ -43,6 +43,8 @@ properties:
>             - adi,adp5589
>               # Analog Devices LT7182S Dual Channel 6A, 20V PolyPhase Step-Down Silent Switcher
>             - adi,lt7182s
> +            # Analog Devices LTP8800-1A/-2/-4A 150A/135A/200A, 54V DC/DC μModule regulator
> +          - adi,ltp8800
>               # AMS iAQ-Core VOC Sensor
>             - ams,iaq-core
>               # Temperature monitoring of Astera Labs PT5161L PCIe retimer
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7c357800519a..6ca691500fb7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13555,6 +13555,11 @@ S:	Maintained
>   F:	Documentation/devicetree/bindings/iio/light/liteon,ltr390.yaml
>   F:	drivers/iio/light/ltr390.c
>   
> +LTP8800 HARDWARE MONITOR DRIVER
> +M:	Cedric Encarnacion <cedricjustine.encarnacion@analog.com>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Supported
> +

This entry doesn't make sense in this patch.

Guenter

>   LYNX 28G SERDES PHY DRIVER
>   M:	Ioana Ciornei <ioana.ciornei@nxp.com>
>   L:	netdev@vger.kernel.org

Re: [PATCH v2 1/2] dt-bindings: trivial-devices: add ltp8800
Posted by Conor Dooley 2 weeks, 3 days ago
On Tue, Nov 05, 2024 at 08:34:01PM -0800, Guenter Roeck wrote:
> On 11/5/24 19:09, Cedric Encarnacion wrote:
> > Add Analog Devices LTP8800-1A, LTP8800-2, and LTP8800-4A DC/DC μModule
> > regulator.

A single compatible for 3 devices is highly suspect. What is
different between these devices?

> > 
> > Signed-off-by: Cedric Encarnacion <cedricjustine.encarnacion@analog.com>
> > ---
> >   Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
> >   MAINTAINERS                                            | 5 +++++
> >   2 files changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> > index 90a7c0a3dc48..72877d00b8dd 100644
> > --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> > +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> > @@ -43,6 +43,8 @@ properties:
> >             - adi,adp5589
> >               # Analog Devices LT7182S Dual Channel 6A, 20V PolyPhase Step-Down Silent Switcher
> >             - adi,lt7182s
> > +            # Analog Devices LTP8800-1A/-2/-4A 150A/135A/200A, 54V DC/DC μModule regulator
> > +          - adi,ltp8800
> >               # AMS iAQ-Core VOC Sensor
> >             - ams,iaq-core
> >               # Temperature monitoring of Astera Labs PT5161L PCIe retimer
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 7c357800519a..6ca691500fb7 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -13555,6 +13555,11 @@ S:	Maintained
> >   F:	Documentation/devicetree/bindings/iio/light/liteon,ltr390.yaml
> >   F:	drivers/iio/light/ltr390.c
> > +LTP8800 HARDWARE MONITOR DRIVER
> > +M:	Cedric Encarnacion <cedricjustine.encarnacion@analog.com>
> > +L:	linux-hwmon@vger.kernel.org
> > +S:	Supported
> > +
> 
> This entry doesn't make sense in this patch.
> 
> Guenter
> 
> >   LYNX 28G SERDES PHY DRIVER
> >   M:	Ioana Ciornei <ioana.ciornei@nxp.com>
> >   L:	netdev@vger.kernel.org
> 
Re: [PATCH v2 1/2] dt-bindings: trivial-devices: add ltp8800
Posted by Guenter Roeck 2 weeks, 3 days ago
On 11/6/24 08:06, Conor Dooley wrote:
> On Tue, Nov 05, 2024 at 08:34:01PM -0800, Guenter Roeck wrote:
>> On 11/5/24 19:09, Cedric Encarnacion wrote:
>>> Add Analog Devices LTP8800-1A, LTP8800-2, and LTP8800-4A DC/DC μModule
>>> regulator.
> 
> A single compatible for 3 devices is highly suspect. What is
> different between these devices?
> 

The maximum supported current is different.

-2:  135A
-1A: 150A
-4A: 200A

Programming is exactly the same, which is why I had asked the submitter to use
a single compatible property. Sorry for that if it is inappropriate.

Is there some guidance explaining when to use a single vs. multiple compatible
properties for different chip variants ?

Note that there are also LTP8803-1A which supports 160A, and LTP8802A-1B
which supports 140A. Maybe there are more, but those are the ones I can find in
public. I don't know if there is a difference from programming perspective compared
to the LTP8800 chip variants; the datasheets are too vague to be sure. It would be
useful to know if those chips should get separate compatible entries if programming
is the same.

Thanks,
Guenter

Re: [PATCH v2 1/2] dt-bindings: trivial-devices: add ltp8800
Posted by Conor Dooley 2 weeks, 3 days ago
On Wed, Nov 06, 2024 at 08:35:33AM -0800, Guenter Roeck wrote:
> On 11/6/24 08:06, Conor Dooley wrote:
> > On Tue, Nov 05, 2024 at 08:34:01PM -0800, Guenter Roeck wrote:
> > > On 11/5/24 19:09, Cedric Encarnacion wrote:
> > > > Add Analog Devices LTP8800-1A, LTP8800-2, and LTP8800-4A DC/DC μModule
> > > > regulator.
> > 
> > A single compatible for 3 devices is highly suspect. What is
> > different between these devices?
> > 
> 
> The maximum supported current is different.
> 
> -2:  135A
> -1A: 150A
> -4A: 200A
> 
> Programming is exactly the same, which is why I had asked the submitter to use
> a single compatible property. Sorry for that if it is inappropriate.
> 
> Is there some guidance explaining when to use a single vs. multiple compatible
> properties for different chip variants ?

TBH, I'm biased and a bit paranoid, so I'd probably give them all
compatibles and set one of them as a fallback. If the programming model
is actually identical, then it's probably fair to use a single
compatible (provided the commit message explains exactly why it's safe
to do) unless the different output conditions require using different
regulator output constraints that different compatibles would be
required to enforce.

> Note that there are also LTP8803-1A which supports 160A, and LTP8802A-1B
> which supports 140A. Maybe there are more, but those are the ones I can find in
> public. I don't know if there is a difference from programming perspective compared
> to the LTP8800 chip variants; the datasheets are too vague to be sure. It would be
> useful to know if those chips should get separate compatible entries if programming
> is the same.
Re: [PATCH v2 1/2] dt-bindings: trivial-devices: add ltp8800
Posted by Guenter Roeck 2 weeks, 3 days ago
On 11/6/24 08:46, Conor Dooley wrote:
> On Wed, Nov 06, 2024 at 08:35:33AM -0800, Guenter Roeck wrote:
>> On 11/6/24 08:06, Conor Dooley wrote:
>>> On Tue, Nov 05, 2024 at 08:34:01PM -0800, Guenter Roeck wrote:
>>>> On 11/5/24 19:09, Cedric Encarnacion wrote:
>>>>> Add Analog Devices LTP8800-1A, LTP8800-2, and LTP8800-4A DC/DC μModule
>>>>> regulator.
>>>
>>> A single compatible for 3 devices is highly suspect. What is
>>> different between these devices?
>>>
>>
>> The maximum supported current is different.
>>
>> -2:  135A
>> -1A: 150A
>> -4A: 200A
>>
>> Programming is exactly the same, which is why I had asked the submitter to use
>> a single compatible property. Sorry for that if it is inappropriate.
>>
>> Is there some guidance explaining when to use a single vs. multiple compatible
>> properties for different chip variants ?
> 
> TBH, I'm biased and a bit paranoid, so I'd probably give them all
> compatibles and set one of them as a fallback. If the programming model

Sometimes compatibles have been rejected because a new chip variant is fully
compatible with an existing one. Now you are doing the opposite. A document
providing reliable guidance one way or the other would really be useful.

Thanks,
Guenter

Re: [PATCH v2 1/2] dt-bindings: trivial-devices: add ltp8800
Posted by Conor Dooley 2 weeks, 3 days ago
On Wed, Nov 06, 2024 at 04:06:02PM +0000, Conor Dooley wrote:
> On Tue, Nov 05, 2024 at 08:34:01PM -0800, Guenter Roeck wrote:
> > On 11/5/24 19:09, Cedric Encarnacion wrote:
> > > Add Analog Devices LTP8800-1A, LTP8800-2, and LTP8800-4A DC/DC μModule
> > > regulator.
> 
> A single compatible for 3 devices is highly suspect. What is
> different between these devices?

Additionally, looking at one of the datasheets, this has several inputs
that could be controlled by a GPIO, a clock input and several supply
inputs. It also has a regulator output. I don't think it is suitable for
trivial-devices.yaml.

> 
> > > 
> > > Signed-off-by: Cedric Encarnacion <cedricjustine.encarnacion@analog.com>
> > > ---
> > >   Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
> > >   MAINTAINERS                                            | 5 +++++
> > >   2 files changed, 7 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> > > index 90a7c0a3dc48..72877d00b8dd 100644
> > > --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> > > +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> > > @@ -43,6 +43,8 @@ properties:
> > >             - adi,adp5589
> > >               # Analog Devices LT7182S Dual Channel 6A, 20V PolyPhase Step-Down Silent Switcher
> > >             - adi,lt7182s
> > > +            # Analog Devices LTP8800-1A/-2/-4A 150A/135A/200A, 54V DC/DC μModule regulator
> > > +          - adi,ltp8800
> > >               # AMS iAQ-Core VOC Sensor
> > >             - ams,iaq-core
> > >               # Temperature monitoring of Astera Labs PT5161L PCIe retimer
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 7c357800519a..6ca691500fb7 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -13555,6 +13555,11 @@ S:	Maintained
> > >   F:	Documentation/devicetree/bindings/iio/light/liteon,ltr390.yaml
> > >   F:	drivers/iio/light/ltr390.c
> > > +LTP8800 HARDWARE MONITOR DRIVER
> > > +M:	Cedric Encarnacion <cedricjustine.encarnacion@analog.com>
> > > +L:	linux-hwmon@vger.kernel.org
> > > +S:	Supported
> > > +
> > 
> > This entry doesn't make sense in this patch.
> > 
> > Guenter
> > 
> > >   LYNX 28G SERDES PHY DRIVER
> > >   M:	Ioana Ciornei <ioana.ciornei@nxp.com>
> > >   L:	netdev@vger.kernel.org
> > 


Re: [PATCH v2 1/2] dt-bindings: trivial-devices: add ltp8800
Posted by Guenter Roeck 2 weeks, 3 days ago
On 11/6/24 08:11, Conor Dooley wrote:
> On Wed, Nov 06, 2024 at 04:06:02PM +0000, Conor Dooley wrote:
>> On Tue, Nov 05, 2024 at 08:34:01PM -0800, Guenter Roeck wrote:
>>> On 11/5/24 19:09, Cedric Encarnacion wrote:
>>>> Add Analog Devices LTP8800-1A, LTP8800-2, and LTP8800-4A DC/DC μModule
>>>> regulator.
>>
>> A single compatible for 3 devices is highly suspect. What is
>> different between these devices?
> 
> Additionally, looking at one of the datasheets, this has several inputs
> that could be controlled by a GPIO, a clock input and several supply
> inputs. It also has a regulator output. I don't think it is suitable for
> trivial-devices.yaml.
> 

All PMBus devices are by definition regulators with input and output voltages.
After all, PMBus stands for "Power Management Bus". Some of them are listed
in trivial devices, some are not. Is that a general guidance, or in other
words should I (we) automatically reject patches adding PMBus devices
to the trivial devices file ?

Thanks,
Guenter

Re: [PATCH v2 1/2] dt-bindings: trivial-devices: add ltp8800
Posted by Conor Dooley 2 weeks, 3 days ago
On Wed, Nov 06, 2024 at 08:43:54AM -0800, Guenter Roeck wrote:
> On 11/6/24 08:11, Conor Dooley wrote:
> > On Wed, Nov 06, 2024 at 04:06:02PM +0000, Conor Dooley wrote:
> > > On Tue, Nov 05, 2024 at 08:34:01PM -0800, Guenter Roeck wrote:
> > > > On 11/5/24 19:09, Cedric Encarnacion wrote:
> > > > > Add Analog Devices LTP8800-1A, LTP8800-2, and LTP8800-4A DC/DC μModule
> > > > > regulator.
> > > 
> > > A single compatible for 3 devices is highly suspect. What is
> > > different between these devices?
> > 
> > Additionally, looking at one of the datasheets, this has several inputs
> > that could be controlled by a GPIO, a clock input and several supply
> > inputs. It also has a regulator output. I don't think it is suitable for
> > trivial-devices.yaml.
> > 
> 
> All PMBus devices are by definition regulators with input and output voltages.
> After all, PMBus stands for "Power Management Bus". Some of them are listed
> in trivial devices, some are not. Is that a general guidance, or in other
> words should I (we) automatically reject patches adding PMBus devices
> to the trivial devices file ?

Personally I like what Jonathan does for iio devices, where he requires
input supplies to be documented, which in turns means they can't go into
trivial-devices.yaml. I wanted to add an input supply option to
trivial-devices.yaml but ?Rob? was not a fan.
In this case it would need a dedicated binding to document the regulator
child node and permit things like regulator-always-on or for any
consumers of the regulator to exist. I suppose that probably applies to
all pmbus bindings?
In this case, there seems to be an input "sync" clock that may need to
be enabled, which is another nail in the coffin for
trivial-devices.yaml.
Re: [PATCH v2 1/2] dt-bindings: trivial-devices: add ltp8800
Posted by Guenter Roeck 2 weeks, 3 days ago
On 11/6/24 08:54, Conor Dooley wrote:
> On Wed, Nov 06, 2024 at 08:43:54AM -0800, Guenter Roeck wrote:
>> On 11/6/24 08:11, Conor Dooley wrote:
>>> On Wed, Nov 06, 2024 at 04:06:02PM +0000, Conor Dooley wrote:
>>>> On Tue, Nov 05, 2024 at 08:34:01PM -0800, Guenter Roeck wrote:
>>>>> On 11/5/24 19:09, Cedric Encarnacion wrote:
>>>>>> Add Analog Devices LTP8800-1A, LTP8800-2, and LTP8800-4A DC/DC μModule
>>>>>> regulator.
>>>>
>>>> A single compatible for 3 devices is highly suspect. What is
>>>> different between these devices?
>>>
>>> Additionally, looking at one of the datasheets, this has several inputs
>>> that could be controlled by a GPIO, a clock input and several supply
>>> inputs. It also has a regulator output. I don't think it is suitable for
>>> trivial-devices.yaml.
>>>
>>
>> All PMBus devices are by definition regulators with input and output voltages.
>> After all, PMBus stands for "Power Management Bus". Some of them are listed
>> in trivial devices, some are not. Is that a general guidance, or in other
>> words should I (we) automatically reject patches adding PMBus devices
>> to the trivial devices file ?
> 
> Personally I like what Jonathan does for iio devices, where he requires
> input supplies to be documented, which in turns means they can't go into
> trivial-devices.yaml. I wanted to add an input supply option to
> trivial-devices.yaml but ?Rob? was not a fan.

I may be missing something, but doesn't every chip have an input supply ?
granted, PMBus chips often have more than one, but still ...

> In this case it would need a dedicated binding to document the regulator
> child node and permit things like regulator-always-on or for any
> consumers of the regulator to exist. I suppose that probably applies to
> all pmbus bindings?

Yes. There may be a few exceptions, for example if a fan controller is
modeled as PMBus device, but that is rare. From a driver perspective,
exposing regulator nodes is optional, though.

> In this case, there seems to be an input "sync" clock that may need to
> be enabled, which is another nail in the coffin for
> trivial-devices.yaml.

I really don't know if it is a good idea to expose such data. That clock can
be connected to ground. It is only necessary in power-sharing configurations,
and requires all chips to use the same clock. I'd assume it to be a fixed clock
in pretty much all circumstances. The frequency needs to be configured into
the chip, but that needs to be done during board manufacturing because it
determines the switching frequency. Writing wrong data into the chip may
render the board unusable or even destroy it (I destroyed several PMBus chips
myself while playing with such parameters on evaluation boards). Maybe there
is some use case where changing the configuration is necessary, but I am not
in favor of exposing it due to the risk involved.

Thanks,
Guenter

Re: [PATCH v2 1/2] dt-bindings: trivial-devices: add ltp8800
Posted by Conor Dooley 2 weeks, 3 days ago
On Wed, Nov 06, 2024 at 10:19:19AM -0800, Guenter Roeck wrote:
> On 11/6/24 08:54, Conor Dooley wrote:
> > On Wed, Nov 06, 2024 at 08:43:54AM -0800, Guenter Roeck wrote:
> > > On 11/6/24 08:11, Conor Dooley wrote:
> > > > On Wed, Nov 06, 2024 at 04:06:02PM +0000, Conor Dooley wrote:
> > > > > On Tue, Nov 05, 2024 at 08:34:01PM -0800, Guenter Roeck wrote:
> > > > > > On 11/5/24 19:09, Cedric Encarnacion wrote:
> > > > > > > Add Analog Devices LTP8800-1A, LTP8800-2, and LTP8800-4A DC/DC μModule
> > > > > > > regulator.
> > > > > 
> > > > > A single compatible for 3 devices is highly suspect. What is
> > > > > different between these devices?
> > > > 
> > > > Additionally, looking at one of the datasheets, this has several inputs
> > > > that could be controlled by a GPIO, a clock input and several supply
> > > > inputs. It also has a regulator output. I don't think it is suitable for
> > > > trivial-devices.yaml.
> > > > 
> > > 
> > > All PMBus devices are by definition regulators with input and output voltages.
> > > After all, PMBus stands for "Power Management Bus". Some of them are listed
> > > in trivial devices, some are not. Is that a general guidance, or in other
> > > words should I (we) automatically reject patches adding PMBus devices
> > > to the trivial devices file ?
> > 
> > Personally I like what Jonathan does for iio devices, where he requires
> > input supplies to be documented, which in turns means they can't go into
> > trivial-devices.yaml. I wanted to add an input supply option to
> > trivial-devices.yaml but ?Rob? was not a fan.
> 
> I may be missing something, but doesn't every chip have an input supply ?
> granted, PMBus chips often have more than one, but still ...

Yeah, that's why I wanted to permit a supply in trivial-devices, because
I bet 99% of devices in there have a supply. IIRC the problem was that
there wasn't a good "generic" name for one. I don't think it was a "you
cannot do this" but a "you need to come up with a name for that supply
that works generically" and I couldn't.

> > In this case it would need a dedicated binding to document the regulator
> > child node and permit things like regulator-always-on or for any
> > consumers of the regulator to exist. I suppose that probably applies to
> > all pmbus bindings?
> 
> Yes. There may be a few exceptions, for example if a fan controller is
> modeled as PMBus device, but that is rare. From a driver perspective,
> exposing regulator nodes is optional, though.
> 
> > In this case, there seems to be an input "sync" clock that may need to
> > be enabled, which is another nail in the coffin for
> > trivial-devices.yaml.
> 
> I really don't know if it is a good idea to expose such data. That clock can
> be connected to ground. It is only necessary in power-sharing configurations,
> and requires all chips to use the same clock. I'd assume it to be a fixed clock
> in pretty much all circumstances. The frequency needs to be configured into
> the chip, but that needs to be done during board manufacturing because it
> determines the switching frequency. Writing wrong data into the chip may
> render the board unusable or even destroy it (I destroyed several PMBus chips
> myself while playing with such parameters on evaluation boards). Maybe there
> is some use case where changing the configuration is necessary, but I am not
> in favor of exposing it due to the risk involved.

I figured it'd be fixed, but that doesn't mean it can't have an enable
(or a supply of its own).