| Message ID | 20240214092504.1237402-1-naresh.solanki@9elements.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] dt-bindings: hwmon: tda38640: Add interrupt & regulator properties | expand |
On Wed, Feb 14, 2024 at 02:55:03PM +0530, Naresh Solanki wrote: > Add properties for interrupt & regulator. > Also update example. I feel like a broken record. Your patches need to explain _why_ you're doing what you're doing. I can read the diff and see this, but I do not know what the justification for it is. /30 seconds later I really am a broken record, to quote from v1: | Feeling like a broken record, given I am leaving the same comments on | multiple patches. The commit message needs to explain why you're doing | something. I can read the diff and see what you did! https://lore.kernel.org/all/20240126-fleshed-subdued-36bae813e2ba@spud/ The patch itself does look better than the v1, with one minor comment below. Thanks, Conor. > Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com> > > --- > Changes in v2: > 1. Remove TEST=.. > 2. Update regulator subnode property as vout0 > 3. Restore commented line in example > 4. blank line after interrupts property in example. > --- > .../hwmon/pmbus/infineon,tda38640.yaml | 28 +++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml > index ded1c115764b..a93b3f86ee87 100644 > --- a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml > +++ b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml > @@ -30,6 +30,23 @@ properties: > unconnected(has internal pull-down). > type: boolean > > + interrupts: > + maxItems: 1 > + > + regulators: > + type: object > + description: > + list of regulators provided by this controller. > + > + properties: > + vout0: Why "vout0" if there's only one output? Is it called that in the documentation? I had a quick check but only saw it called "vout". Are there other related devices that would have multiple regulators that might end up sharing the binding? Thanks, Conor. > + $ref: /schemas/regulator/regulator.yaml# > + type: object > + > + unevaluatedProperties: false > + > + additionalProperties: false > + > required: > - compatible > - reg > @@ -38,6 +55,7 @@ additionalProperties: false > > examples: > - | > + #include <dt-bindings/interrupt-controller/irq.h> > i2c { > #address-cells = <1>; > #size-cells = <0>; > @@ -45,5 +63,15 @@ examples: > tda38640@40 { > compatible = "infineon,tda38640"; > reg = <0x40>; > + > + interrupt-parent = <&smb_pex_cpu0_event>; > + interrupts = <10 IRQ_TYPE_LEVEL_LOW>; > + > + regulators { > + pvnn_main_cpu0: vout0 { > + regulator-name = "pvnn_main_cpu0"; > + regulator-enable-ramp-delay = <200>; > + }; > + }; > }; > }; > > base-commit: 7e90b5c295ec1e47c8ad865429f046970c549a66 > -- > 2.42.0 >
On 2/14/24 09:51, Conor Dooley wrote: > On Wed, Feb 14, 2024 at 02:55:03PM +0530, Naresh Solanki wrote: >> Add properties for interrupt & regulator. >> Also update example. > > I feel like a broken record. Your patches need to explain _why_ you're > doing what you're doing. I can read the diff and see this, but I do not > know what the justification for it is. > > /30 seconds later > I really am a broken record, to quote from v1: > | Feeling like a broken record, given I am leaving the same comments on > | multiple patches. The commit message needs to explain why you're doing > | something. I can read the diff and see what you did! > > https://lore.kernel.org/all/20240126-fleshed-subdued-36bae813e2ba@spud/ > > The patch itself does look better than the v1, with one minor comment > below. > > Thanks, > Conor. > >> Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com> >> >> --- >> Changes in v2: >> 1. Remove TEST=.. >> 2. Update regulator subnode property as vout0 >> 3. Restore commented line in example >> 4. blank line after interrupts property in example. >> --- >> .../hwmon/pmbus/infineon,tda38640.yaml | 28 +++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml >> index ded1c115764b..a93b3f86ee87 100644 >> --- a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml >> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml >> @@ -30,6 +30,23 @@ properties: >> unconnected(has internal pull-down). >> type: boolean >> >> + interrupts: >> + maxItems: 1 >> + >> + regulators: >> + type: object >> + description: >> + list of regulators provided by this controller. >> + >> + properties: >> + vout0: > > Why "vout0" if there's only one output? Is it called that in the > documentation? I had a quick check but only saw it called "vout". > Are there other related devices that would have multiple regulators > that might end up sharing the binding? > Primarily because that is what the PMBus core generates for the driver because no one including me was aware that this is unacceptable for single-output drivers. We now have commit 88b5970e92d0 ("hwmon: (pmbus/core) Add helper macro to define single pmbus regulator"). I guess we can update the tda38640 driver to use the new macro to register vout instead of vout0. Guenter
On Wed, Feb 14, 2024 at 10:48:52AM -0800, Guenter Roeck wrote: > On 2/14/24 09:51, Conor Dooley wrote: > > On Wed, Feb 14, 2024 at 02:55:03PM +0530, Naresh Solanki wrote: > > > Add properties for interrupt & regulator. > > > Also update example. > > > > I feel like a broken record. Your patches need to explain _why_ you're > > doing what you're doing. I can read the diff and see this, but I do not > > know what the justification for it is. > > > > /30 seconds later > > I really am a broken record, to quote from v1: > > | Feeling like a broken record, given I am leaving the same comments on > > | multiple patches. The commit message needs to explain why you're doing > > | something. I can read the diff and see what you did! > > > > https://lore.kernel.org/all/20240126-fleshed-subdued-36bae813e2ba@spud/ > > > > The patch itself does look better than the v1, with one minor comment > > below. > > > > Thanks, > > Conor. > > > > > Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com> > > > > > > --- > > > Changes in v2: > > > 1. Remove TEST=.. > > > 2. Update regulator subnode property as vout0 > > > 3. Restore commented line in example > > > 4. blank line after interrupts property in example. > > > --- > > > .../hwmon/pmbus/infineon,tda38640.yaml | 28 +++++++++++++++++++ > > > 1 file changed, 28 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml > > > index ded1c115764b..a93b3f86ee87 100644 > > > --- a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml > > > +++ b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml > > > @@ -30,6 +30,23 @@ properties: > > > unconnected(has internal pull-down). > > > type: boolean > > > + interrupts: > > > + maxItems: 1 > > > + > > > + regulators: > > > + type: object > > > + description: > > > + list of regulators provided by this controller. > > > + > > > + properties: > > > + vout0: > > > > Why "vout0" if there's only one output? Is it called that in the > > documentation? I had a quick check but only saw it called "vout". > > Are there other related devices that would have multiple regulators > > that might end up sharing the binding? > > > > Primarily because that is what the PMBus core generates for the driver > because no one including me was aware that this is unacceptable > for single-output drivers. Is it unacceptable? If you're implying that I am saying it is, that's not what I was doing here - I'm just wondering why it was chosen. Numbering when there's only one seems odd, so I was just looking for the rationale. > We now have commit 88b5970e92d0 ("hwmon: > (pmbus/core) Add helper macro to define single pmbus regulator"). > I guess we can update the tda38640 driver to use the new macro > to register vout instead of vout0.
On 2/14/24 11:55, Conor Dooley wrote: [ ... ] >>> Why "vout0" if there's only one output? Is it called that in the >>> documentation? I had a quick check but only saw it called "vout". >>> Are there other related devices that would have multiple regulators >>> that might end up sharing the binding? >>> >> >> Primarily because that is what the PMBus core generates for the driver >> because no one including me was aware that this is unacceptable >> for single-output drivers. > > Is it unacceptable? If you're implying that I am saying it is, that's > not what I was doing here - I'm just wondering why it was chosen. > Numbering when there's only one seems odd, so I was just looking for the > rationale. > Given the tendency of corporate speak (aka "this was a good attempt" for a complete screwup), and since this did come up before, I did interpret it along that line. My apologies if that was not the idea. Still, I really don't know how to resolve this for existing PMBus drivers which do register "vout0" even if there is only a single output regulator. Guenter
On Wed, Feb 14, 2024 at 05:17:04PM -0800, Guenter Roeck wrote: > On 2/14/24 11:55, Conor Dooley wrote: > [ ... ] > > > > Why "vout0" if there's only one output? Is it called that in the > > > > documentation? I had a quick check but only saw it called "vout". > > > > Are there other related devices that would have multiple regulators > > > > that might end up sharing the binding? > > > > > > > > > > Primarily because that is what the PMBus core generates for the driver > > > because no one including me was aware that this is unacceptable > > > for single-output drivers. > > > > Is it unacceptable? If you're implying that I am saying it is, that's > > not what I was doing here - I'm just wondering why it was chosen. > > Numbering when there's only one seems odd, so I was just looking for the > > rationale. > > > > Given the tendency of corporate speak (aka "this was a good attempt" for > a complete screwup), and since this did come up before, I did interpret > it along that line. My apologies if that was not the idea. I'm not gonna go and decree that "vout0" is unacceptable, if it was called that in documentation that I had missed or was convention, I was just gonna say "okay, that sounds reasonable to me". > Still, I really don't know how to resolve this for existing PMBus drivers > which do register "vout0" even if there is only a single output regulator. I had a quick look at that series, none of the devices that I checked out there seem to have documented regulators at all. Some of the devices were only documented in trivial-devices.yaml. Relying on the naming of undocumented child nodes is a bug in those drivers & I guess nobody cares about dtbs_check complaints for those platforms. The example that was linked in the other thread doesn't even use a valid compatible :( https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed/aspeed-bmc-delta-ahe50dc.dts?id=8d3dea210042f54b952b481838c1e7dfc4ec751d#n21 I guess it uses the i2c device ids to probe on that platform, or have I missed something there? Cheers, Conor.
On 2/15/24 03:48, Conor Dooley wrote: > On Wed, Feb 14, 2024 at 05:17:04PM -0800, Guenter Roeck wrote: >> On 2/14/24 11:55, Conor Dooley wrote: >> [ ... ] >>>>> Why "vout0" if there's only one output? Is it called that in the >>>>> documentation? I had a quick check but only saw it called "vout". >>>>> Are there other related devices that would have multiple regulators >>>>> that might end up sharing the binding? >>>>> >>>> >>>> Primarily because that is what the PMBus core generates for the driver >>>> because no one including me was aware that this is unacceptable >>>> for single-output drivers. >>> >>> Is it unacceptable? If you're implying that I am saying it is, that's >>> not what I was doing here - I'm just wondering why it was chosen. >>> Numbering when there's only one seems odd, so I was just looking for the >>> rationale. >>> >> >> Given the tendency of corporate speak (aka "this was a good attempt" for >> a complete screwup), and since this did come up before, I did interpret >> it along that line. My apologies if that was not the idea. > > I'm not gonna go and decree that "vout0" is unacceptable, if it was > called that in documentation that I had missed or was convention, I was > just gonna say "okay, that sounds reasonable to me". > "convention" only if lack of awareness how regulators are supposed to be named is a convention. >> Still, I really don't know how to resolve this for existing PMBus drivers >> which do register "vout0" even if there is only a single output regulator. > > I had a quick look at that series, none of the devices that I checked > out there seem to have documented regulators at all. Some of the devices > were only documented in trivial-devices.yaml. Relying on the naming of > undocumented child nodes is a bug in those drivers & I guess nobody cares > about dtbs_check complaints for those platforms. The example that was > linked in the other thread doesn't even use a valid compatible :( > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed/aspeed-bmc-delta-ahe50dc.dts?id=8d3dea210042f54b952b481838c1e7dfc4ec751d#n21 > I guess it uses the i2c device ids to probe on that platform, or have > I missed something there? > I think that is correct. If I recall correctly, the I2C subsystem no longer searches for compatible drivers by only looking at the device id in the compatible node, so I guess one has to list "lm25066" instead of "ti,lm25066" as compatible to get a match in the i2c subsystem. That is of course completely wrong. Guenter
On Sat, Feb 17, 2024 at 07:57:43AM -0800, Guenter Roeck wrote: > On 2/15/24 03:48, Conor Dooley wrote: > > On Wed, Feb 14, 2024 at 05:17:04PM -0800, Guenter Roeck wrote: > > > On 2/14/24 11:55, Conor Dooley wrote: > > > [ ... ] > > > > > > Why "vout0" if there's only one output? Is it called that in the > > > > > > documentation? I had a quick check but only saw it called "vout". > > > > > > Are there other related devices that would have multiple regulators > > > > > > that might end up sharing the binding? > > > > > > > > > > > > > > > > Primarily because that is what the PMBus core generates for the driver > > > > > because no one including me was aware that this is unacceptable > > > > > for single-output drivers. > > > > > > > > Is it unacceptable? If you're implying that I am saying it is, that's > > > > not what I was doing here - I'm just wondering why it was chosen. > > > > Numbering when there's only one seems odd, so I was just looking for the > > > > rationale. > > > > > > > > > > Given the tendency of corporate speak (aka "this was a good attempt" for > > > a complete screwup), and since this did come up before, I did interpret > > > it along that line. My apologies if that was not the idea. > > > > I'm not gonna go and decree that "vout0" is unacceptable, if it was > > called that in documentation that I had missed or was convention, I was > > just gonna say "okay, that sounds reasonable to me". > > > > "convention" only if lack of awareness how regulators are supposed to be named > is a convention. They're "supposed" to be named whatever the binding says they are named, but as we've discovered none of these devices actually have bindings that allow regulators in the first place. I think they should be called whatever they're called in the documentation for the device, which in this case was "vout". > > > Still, I really don't know how to resolve this for existing PMBus drivers > > > which do register "vout0" even if there is only a single output regulator. > > > > I had a quick look at that series, none of the devices that I checked > > out there seem to have documented regulators at all. Some of the devices > > were only documented in trivial-devices.yaml. Relying on the naming of > > undocumented child nodes is a bug in those drivers & I guess nobody cares > > about dtbs_check complaints for those platforms. The example that was > > linked in the other thread doesn't even use a valid compatible :( > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed/aspeed-bmc-delta-ahe50dc.dts?id=8d3dea210042f54b952b481838c1e7dfc4ec751d#n21 > > I guess it uses the i2c device ids to probe on that platform, or have > > I missed something there? > > > > I think that is correct. If I recall correctly, the I2C subsystem no longer > searches for compatible drivers by only looking at the device id in the > compatible node, so I guess one has to list "lm25066" instead of "ti,lm25066" > as compatible to get a match in the i2c subsystem. That is of course > completely wrong. If the driver is probing based on i2c_device_id matching, is it correct to use DT to probe the regulators? (I don't know, that's not some sort of rhetorical question).
On 2/17/24 12:30, Conor Dooley wrote: > On Sat, Feb 17, 2024 at 07:57:43AM -0800, Guenter Roeck wrote: >> On 2/15/24 03:48, Conor Dooley wrote: >>> On Wed, Feb 14, 2024 at 05:17:04PM -0800, Guenter Roeck wrote: >>>> On 2/14/24 11:55, Conor Dooley wrote: >>>> [ ... ] >>>>>>> Why "vout0" if there's only one output? Is it called that in the >>>>>>> documentation? I had a quick check but only saw it called "vout". >>>>>>> Are there other related devices that would have multiple regulators >>>>>>> that might end up sharing the binding? >>>>>>> >>>>>> >>>>>> Primarily because that is what the PMBus core generates for the driver >>>>>> because no one including me was aware that this is unacceptable >>>>>> for single-output drivers. >>>>> >>>>> Is it unacceptable? If you're implying that I am saying it is, that's >>>>> not what I was doing here - I'm just wondering why it was chosen. >>>>> Numbering when there's only one seems odd, so I was just looking for the >>>>> rationale. >>>>> >>>> >>>> Given the tendency of corporate speak (aka "this was a good attempt" for >>>> a complete screwup), and since this did come up before, I did interpret >>>> it along that line. My apologies if that was not the idea. >>> >>> I'm not gonna go and decree that "vout0" is unacceptable, if it was >>> called that in documentation that I had missed or was convention, I was >>> just gonna say "okay, that sounds reasonable to me". >>> >> >> "convention" only if lack of awareness how regulators are supposed to be named >> is a convention. > > They're "supposed" to be named whatever the binding says they are named, > but as we've discovered none of these devices actually have bindings > that allow regulators in the first place. I think they should be called > whatever they're called in the documentation for the device, which in > this case was "vout". > I would agree. I'd even submit a yaml file extension for the affected drivers to address that, but I realize that I am notoriously bad with doing that, so I won't even try. >>>> Still, I really don't know how to resolve this for existing PMBus drivers >>>> which do register "vout0" even if there is only a single output regulator. >>> >>> I had a quick look at that series, none of the devices that I checked >>> out there seem to have documented regulators at all. Some of the devices >>> were only documented in trivial-devices.yaml. Relying on the naming of >>> undocumented child nodes is a bug in those drivers & I guess nobody cares >>> about dtbs_check complaints for those platforms. The example that was >>> linked in the other thread doesn't even use a valid compatible :( >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed/aspeed-bmc-delta-ahe50dc.dts?id=8d3dea210042f54b952b481838c1e7dfc4ec751d#n21 >>> I guess it uses the i2c device ids to probe on that platform, or have >>> I missed something there? >>> >> >> I think that is correct. If I recall correctly, the I2C subsystem no longer >> searches for compatible drivers by only looking at the device id in the >> compatible node, so I guess one has to list "lm25066" instead of "ti,lm25066" >> as compatible to get a match in the i2c subsystem. That is of course >> completely wrong. > > If the driver is probing based on i2c_device_id matching, is it correct to > use DT to probe the regulators? (I don't know, that's not some sort of > rhetorical question). Looking into the lm25066 driver, it actually does support the "ti,lm25066" compatible. Given that, I don't know why the dts file doesn't use it, especially since the dts file was created after the compatible entry was added to the lm25066 driver. Guenter
diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml index ded1c115764b..a93b3f86ee87 100644 --- a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml +++ b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml @@ -30,6 +30,23 @@ properties: unconnected(has internal pull-down). type: boolean + interrupts: + maxItems: 1 + + regulators: + type: object + description: + list of regulators provided by this controller. + + properties: + vout0: + $ref: /schemas/regulator/regulator.yaml# + type: object + + unevaluatedProperties: false + + additionalProperties: false + required: - compatible - reg @@ -38,6 +55,7 @@ additionalProperties: false examples: - | + #include <dt-bindings/interrupt-controller/irq.h> i2c { #address-cells = <1>; #size-cells = <0>; @@ -45,5 +63,15 @@ examples: tda38640@40 { compatible = "infineon,tda38640"; reg = <0x40>; + + interrupt-parent = <&smb_pex_cpu0_event>; + interrupts = <10 IRQ_TYPE_LEVEL_LOW>; + + regulators { + pvnn_main_cpu0: vout0 { + regulator-name = "pvnn_main_cpu0"; + regulator-enable-ramp-delay = <200>; + }; + }; }; };
Add properties for interrupt & regulator. Also update example. Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com> --- Changes in v2: 1. Remove TEST=.. 2. Update regulator subnode property as vout0 3. Restore commented line in example 4. blank line after interrupts property in example. --- .../hwmon/pmbus/infineon,tda38640.yaml | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) base-commit: 7e90b5c295ec1e47c8ad865429f046970c549a66