| Message ID | 20240222-ltc2983-misc-improv-v1-5-cf7d4457e98c@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: temperature: ltc2983: small improvements | expand |
On Thu, Feb 22, 2024 at 01:55:56PM +0100, Nuno Sa wrote: > Add a property for the VDD power supply regulator. > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > --- > Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > index dbb85135fd66..8aae867a770a 100644 > --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > @@ -57,6 +57,8 @@ properties: > interrupts: > maxItems: 1 > > + vdd-supply: true Although technically an ABI break, should we make this supply required? It is, at the end of the day, required by the hardware for operation. > + > adi,mux-delay-config-us: > description: | > Extra delay prior to each conversion, in addition to the internal 1ms > > -- > 2.43.2 >
On Thu, 2024-02-22 at 15:40 +0000, Conor Dooley wrote: > On Thu, Feb 22, 2024 at 01:55:56PM +0100, Nuno Sa wrote: > > Add a property for the VDD power supply regulator. > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > --- > > Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > index dbb85135fd66..8aae867a770a 100644 > > --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > @@ -57,6 +57,8 @@ properties: > > interrupts: > > maxItems: 1 > > > > + vdd-supply: true > > Although technically an ABI break, should we make this supply required? > It is, at the end of the day, required by the hardware for operation. > I thought about it but then realized it could break some existing users which is never a nice thing. I recently (in another series - the IIO backend) went through some trouble to actually not break ABI. Meaning, I had to do some not so neat hacking in the driver because Rob was more comfortable with not breaking ABI in DT. So, I assumed he would not like for me to break it in here. - Nuno Sá >
On Thu, Feb 22, 2024 at 05:41:03PM +0100, Nuno Sá wrote: > On Thu, 2024-02-22 at 15:40 +0000, Conor Dooley wrote: > > On Thu, Feb 22, 2024 at 01:55:56PM +0100, Nuno Sa wrote: > > > Add a property for the VDD power supply regulator. > > > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > > --- > > > Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git > > > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > > index dbb85135fd66..8aae867a770a 100644 > > > --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > > +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > > @@ -57,6 +57,8 @@ properties: > > > interrupts: > > > maxItems: 1 > > > > > > + vdd-supply: true > > > > Although technically an ABI break, should we make this supply required? > > It is, at the end of the day, required by the hardware for operation. > > > > I thought about it but then realized it could break some existing users which is > never a nice thing. Could you explain what scenario it actually breaks a system (not produces warnings with dtbs_check)? If anything actually broke something, it would be the driver change that actually assumed that the regulator was present and refused to probe otherwise, right? In Linux at least, the regulator core will provide a dummy regulator if one doesn't exist - otherwise patch 6/6 would actually contain a DT ABI breakage, since it does: ret = devm_regulator_get_enable(&spi->dev, "vdd"); if (ret) return ret; IMO making a new property required is only a meaningful break of the ABI when drivers reject probe when it is missing, but I must admit I don't know if other operating systems handle missing regulators as nicely as Linux does. > I recently (in another series - the IIO backend) went through some trouble to > actually not break ABI. Meaning, I had to do some not so neat hacking in the > driver because Rob was more comfortable with not breaking ABI in DT. So, I > assumed he would not like for me to break it in here.
On Thu, 22 Feb 2024 17:54:28 +0000 Conor Dooley <conor@kernel.org> wrote: > On Thu, Feb 22, 2024 at 05:41:03PM +0100, Nuno Sá wrote: > > On Thu, 2024-02-22 at 15:40 +0000, Conor Dooley wrote: > > > On Thu, Feb 22, 2024 at 01:55:56PM +0100, Nuno Sa wrote: > > > > Add a property for the VDD power supply regulator. > > > > > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > > > --- > > > > Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > > > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > > > index dbb85135fd66..8aae867a770a 100644 > > > > --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > > > +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > > > @@ -57,6 +57,8 @@ properties: > > > > interrupts: > > > > maxItems: 1 > > > > > > > > + vdd-supply: true > > > > > > Although technically an ABI break, should we make this supply required? > > > It is, at the end of the day, required by the hardware for operation. > > > > > > > I thought about it but then realized it could break some existing users which is > > never a nice thing. > > Could you explain what scenario it actually breaks a system (not > produces warnings with dtbs_check)? > > If anything actually broke something, it would be the driver change that > actually assumed that the regulator was present and refused to probe > otherwise, right? In Linux at least, the regulator core will provide a > dummy regulator if one doesn't exist - otherwise patch 6/6 would > actually contain a DT ABI breakage, since it does: > > ret = devm_regulator_get_enable(&spi->dev, "vdd"); > if (ret) > return ret; > > IMO making a new property required is only a meaningful break of the ABI > when drivers reject probe when it is missing, but I must admit I don't > know if other operating systems handle missing regulators as nicely as > Linux does. Agreed - adding a requirement on a supply to the dt-binding shouldn't be a problem because of the dummy regulators. Jonathan > > > I recently (in another series - the IIO backend) went through some trouble to > > actually not break ABI. Meaning, I had to do some not so neat hacking in the > > driver because Rob was more comfortable with not breaking ABI in DT. So, I > > assumed he would not like for me to break it in here. >
On Thu, 2024-02-22 at 17:54 +0000, Conor Dooley wrote: > On Thu, Feb 22, 2024 at 05:41:03PM +0100, Nuno Sá wrote: > > On Thu, 2024-02-22 at 15:40 +0000, Conor Dooley wrote: > > > On Thu, Feb 22, 2024 at 01:55:56PM +0100, Nuno Sa wrote: > > > > Add a property for the VDD power supply regulator. > > > > > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > > > --- > > > > Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 2 > > > > ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > > > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > > > index dbb85135fd66..8aae867a770a 100644 > > > > --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > > > +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > > > @@ -57,6 +57,8 @@ properties: > > > > interrupts: > > > > maxItems: 1 > > > > > > > > + vdd-supply: true > > > > > > Although technically an ABI break, should we make this supply required? > > > It is, at the end of the day, required by the hardware for operation. > > > > > > > I thought about it but then realized it could break some existing users > > which is > > never a nice thing. > > Could you explain what scenario it actually breaks a system (not > produces warnings with dtbs_check)? Oh, I guess I could not explain myself :). I did not meant breaking the system (I'm aware of the dummy regulator) but I meant exactly what you mention above about dtbs_check. Like, if someone already validated a devicetree against the current bindings, that same devicetree will fail to validate now right? And I had the idea that we should not allow that... If not the case, I'm perfectly fine in making the supply required. - Nuno Sá >
On Fri, Feb 23, 2024 at 09:17:16AM +0100, Nuno Sá wrote: > On Thu, 2024-02-22 at 17:54 +0000, Conor Dooley wrote: > > On Thu, Feb 22, 2024 at 05:41:03PM +0100, Nuno Sá wrote: > > > On Thu, 2024-02-22 at 15:40 +0000, Conor Dooley wrote: > > > > On Thu, Feb 22, 2024 at 01:55:56PM +0100, Nuno Sa wrote: > > > > > Add a property for the VDD power supply regulator. > > > > > > > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > > > > --- > > > > > Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 2 > > > > > ++ > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > diff --git > > > > > a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > > > > b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > > > > index dbb85135fd66..8aae867a770a 100644 > > > > > --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > > > > +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml > > > > > @@ -57,6 +57,8 @@ properties: > > > > > interrupts: > > > > > maxItems: 1 > > > > > > > > > > + vdd-supply: true > > > > > > > > Although technically an ABI break, should we make this supply required? > > > > It is, at the end of the day, required by the hardware for operation. > > > > > > > > > > I thought about it but then realized it could break some existing users > > > which is > > > never a nice thing. > > > > Could you explain what scenario it actually breaks a system (not > > produces warnings with dtbs_check)? > > Oh, I guess I could not explain myself :). I did not meant breaking the system > (I'm aware of the dummy regulator) but I meant exactly what you mention above > about dtbs_check. Like, if someone already validated a devicetree against the > current bindings, that same devicetree will fail to validate now right? And I > had the idea that we should not allow that... If not the case, I'm perfectly > fine in making the supply required. I think that's fine, the system will still work which is the important part of the ABI. Cheers, Conor.
diff --git a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml index dbb85135fd66..8aae867a770a 100644 --- a/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml +++ b/Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml @@ -57,6 +57,8 @@ properties: interrupts: maxItems: 1 + vdd-supply: true + adi,mux-delay-config-us: description: | Extra delay prior to each conversion, in addition to the internal 1ms
Add a property for the VDD power supply regulator. Signed-off-by: Nuno Sa <nuno.sa@analog.com> --- Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml | 2 ++ 1 file changed, 2 insertions(+)