@@ -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
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.
quoted hunk
Signed-off-by: Naresh Solanki <redacted>
---
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(+)
@@ -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.
On Wed, Feb 14, 2024 at 02:55:03PM +0530, Naresh Solanki wrote:
quoted
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.
quoted
Signed-off-by: Naresh Solanki <redacted>
---
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(+)
@@ -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:
quoted
On Wed, Feb 14, 2024 at 02:55:03PM +0530, Naresh Solanki wrote:
quoted
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.
quoted
Signed-off-by: Naresh Solanki <redacted>
---
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(+)
@@ -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.
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:
[ ... ]
quoted
quoted
quoted
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 Wed, Feb 14, 2024 at 05:17:04PM -0800, Guenter Roeck wrote:
quoted
On 2/14/24 11:55, Conor Dooley wrote:
[ ... ]
quoted
quoted
quoted
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.
quoted
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:
quoted
On Wed, Feb 14, 2024 at 05:17:04PM -0800, Guenter Roeck wrote:
quoted
On 2/14/24 11:55, Conor Dooley wrote:
[ ... ]
quoted
quoted
quoted
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".
quoted
quoted
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 Sat, Feb 17, 2024 at 07:57:43AM -0800, Guenter Roeck wrote:
quoted
On 2/15/24 03:48, Conor Dooley wrote:
quoted
On Wed, Feb 14, 2024 at 05:17:04PM -0800, Guenter Roeck wrote:
quoted
On 2/14/24 11:55, Conor Dooley wrote:
[ ... ]
quoted
quoted
quoted
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.
quoted
quoted
quoted
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