[PATCH] iio: ad5770r: make devicetree property reading consistent

[PATCH] iio: ad5770r: make devicetree property reading consistent

[Nuno Sá]

The bindings file for this driver is defining the property as 'reg' but
the driver was reading it with the 'num' name. This patches makes the
driver consistent with what is defined in the bindings.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/dac/ad5770r.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/dac/ad5770r.c b/drivers/iio/dac/ad5770r.c
index 8107f7bbbe3c..7e2fd32e993a 100644
--- a/drivers/iio/dac/ad5770r.c
+++ b/drivers/iio/dac/ad5770r.c
@@ -522,7 +522,7 @@ static int ad5770r_channel_config(struct ad5770r_state *st)
 		return -EINVAL;
 
 	device_for_each_child_node(&st->spi->dev, child) {
-		ret = fwnode_property_read_u32(child, "num", &num);
+		ret = fwnode_property_read_u32(child, "reg", &num);
 		if (ret)
 			goto err_child_out;
 		if (num >= AD5770R_MAX_CHANNELS) {
-- 
2.32.0

Re: [PATCH] iio: ad5770r: make devicetree property reading consistent

[Andy Shevchenko]

On Wed, Aug 11, 2021 at 10:46 AM Nuno Sá <nuno.sa@analog.com> wrote:
>
> The bindings file for this driver is defining the property as 'reg' but
> the driver was reading it with the 'num' name. This patches makes the

"This patches makes the..." --> "Make the..."

> driver consistent with what is defined in the bindings.

While it seems okay, it may be now a chicken-egg issue (somebody
created a DT with "num" property).

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] iio: ad5770r: make devicetree property reading consistent

[Sa, Nuno]

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Wednesday, August 11, 2021 6:04 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> <jic23@kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>
> Subject: Re: [PATCH] iio: ad5770r: make devicetree property reading
> consistent
> 
> On Wed, Aug 11, 2021 at 10:46 AM Nuno Sá <nuno.sa@analog.com>
> wrote:
> >
> > The bindings file for this driver is defining the property as 'reg' but
> > the driver was reading it with the 'num' name. This patches makes
> the
> 
> "This patches makes the..." --> "Make the..."
> 
> > driver consistent with what is defined in the bindings.
> 
> While it seems okay, it may be now a chicken-egg issue (somebody
> created a DT with "num" property).
> 

Arghh, I see. Well, maybe let's go the other way around and change the
bindings doc to 'num'?

- Nuno Sá

RE: [PATCH] iio: ad5770r: make devicetree property reading consistent

[Sa, Nuno]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Thursday, August 12, 2021 9:06 AM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> <jic23@kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>
> Subject: Re: [PATCH] iio: ad5770r: make devicetree property reading
> consistent
> 
> 
> 
> On Thursday, August 12, 2021, Sa, Nuno <Nuno.Sa@analog.com
> <mailto:Nuno.Sa@analog.com> > wrote:
> 
> 
> 	> From: Andy Shevchenko <andy.shevchenko@gmail.com
> <mailto:andy.shevchenko@gmail.com> >
> 	> Sent: Wednesday, August 11, 2021 6:04 PM
> 	> To: Sa, Nuno <Nuno.Sa@analog.com
> <mailto:Nuno.Sa@analog.com> >
> 	> Cc: linux-iio <linux-iio@vger.kernel.org <mailto:linux-
> iio@vger.kernel.org> >; Jonathan Cameron
> 	> <jic23@kernel.org <mailto:jic23@kernel.org> >; Hennerich,
> Michael
> 	> <Michael.Hennerich@analog.com
> <mailto:Michael.Hennerich@analog.com> >; Lars-Peter Clausen
> 	> <lars@metafoo.de <mailto:lars@metafoo.de> >
> 	> Subject: Re: [PATCH] iio: ad5770r: make devicetree property
> reading
> 	> consistent
> 	>
> 	> On Wed, Aug 11, 2021 at 10:46 AM Nuno Sá
> <nuno.sa@analog.com <mailto:nuno.sa@analog.com> >
> 	> wrote:
> 	> >
> 	> > The bindings file for this driver is defining the property as
> 'reg' but
> 	> > the driver was reading it with the 'num' name. This patches
> makes
> 	> the
> 	>
> 	> "This patches makes the..." --> "Make the..."
> 	>
> 	> > driver consistent with what is defined in the bindings.
> 	>
> 	> While it seems okay, it may be now a chicken-egg issue
> (somebody
> 	> created a DT with "num" property).
> 	>
> 
> 	Arghh, I see. Well, maybe let's go the other way around and
> change the
> 	bindings doc to 'num'?
> 
> 
> Not sure, like I said it’s a chicken-egg issue. Consult with Rob perhaps?

Hi Rob,

Could you give your input on this one?

Thanks!
- Nuno Sá 

Re: [PATCH] iio: ad5770r: make devicetree property reading consistent

[Rob Herring]

On Thu, Aug 12, 2021 at 3:14 AM Sa, Nuno <Nuno.Sa@analog.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Thursday, August 12, 2021 9:06 AM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > <jic23@kernel.org>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > <lars@metafoo.de>
> > Subject: Re: [PATCH] iio: ad5770r: make devicetree property reading
> > consistent
> >
> >
> >
> > On Thursday, August 12, 2021, Sa, Nuno <Nuno.Sa@analog.com
> > <mailto:Nuno.Sa@analog.com> > wrote:
> >
> >
> >       > From: Andy Shevchenko <andy.shevchenko@gmail.com
> > <mailto:andy.shevchenko@gmail.com> >
> >       > Sent: Wednesday, August 11, 2021 6:04 PM
> >       > To: Sa, Nuno <Nuno.Sa@analog.com
> > <mailto:Nuno.Sa@analog.com> >
> >       > Cc: linux-iio <linux-iio@vger.kernel.org <mailto:linux-
> > iio@vger.kernel.org> >; Jonathan Cameron
> >       > <jic23@kernel.org <mailto:jic23@kernel.org> >; Hennerich,
> > Michael
> >       > <Michael.Hennerich@analog.com
> > <mailto:Michael.Hennerich@analog.com> >; Lars-Peter Clausen
> >       > <lars@metafoo.de <mailto:lars@metafoo.de> >
> >       > Subject: Re: [PATCH] iio: ad5770r: make devicetree property
> > reading
> >       > consistent
> >       >
> >       > On Wed, Aug 11, 2021 at 10:46 AM Nuno Sá
> > <nuno.sa@analog.com <mailto:nuno.sa@analog.com> >
> >       > wrote:
> >       > >
> >       > > The bindings file for this driver is defining the property as
> > 'reg' but
> >       > > the driver was reading it with the 'num' name. This patches
> > makes
> >       > the
> >       >
> >       > "This patches makes the..." --> "Make the..."
> >       >
> >       > > driver consistent with what is defined in the bindings.
> >       >
> >       > While it seems okay, it may be now a chicken-egg issue
> > (somebody
> >       > created a DT with "num" property).
> >       >
> >
> >       Arghh, I see. Well, maybe let's go the other way around and
> > change the
> >       bindings doc to 'num'?
> >
> >
> > Not sure, like I said it’s a chicken-egg issue. Consult with Rob perhaps?
>
> Hi Rob,
>
> Could you give your input on this one?

There's no context, but I'm assuming this is in channel nodes. Keep
the binding 'reg' and make the driver support both if needed.
Considering the author of the binding also changed the binding from
num to reg shortly after adding the binding, I don't think 'num'
support is needed. If someone used 'num' and didn't run validation,
well, that's their problem.

Rob

RE: [PATCH] iio: ad5770r: make devicetree property reading consistent

[Sa, Nuno]

> -----Original Message-----
> From: Rob Herring <robh+dt@kernel.org>
> Sent: Thursday, August 12, 2021 5:11 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; linux-iio
> <linux-iio@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Lars-Peter
> Clausen <lars@metafoo.de>
> Subject: Re: [PATCH] iio: ad5770r: make devicetree property reading
> consistent
> 
> On Thu, Aug 12, 2021 at 3:14 AM Sa, Nuno <Nuno.Sa@analog.com>
> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Thursday, August 12, 2021 9:06 AM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > > <jic23@kernel.org>; Hennerich, Michael
> > > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > > <lars@metafoo.de>
> > > Subject: Re: [PATCH] iio: ad5770r: make devicetree property
> reading
> > > consistent
> > >
> > >
> > >
> > > On Thursday, August 12, 2021, Sa, Nuno <Nuno.Sa@analog.com
> > > <mailto:Nuno.Sa@analog.com> > wrote:
> > >
> > >
> > >       > From: Andy Shevchenko <andy.shevchenko@gmail.com
> > > <mailto:andy.shevchenko@gmail.com> >
> > >       > Sent: Wednesday, August 11, 2021 6:04 PM
> > >       > To: Sa, Nuno <Nuno.Sa@analog.com
> > > <mailto:Nuno.Sa@analog.com> >
> > >       > Cc: linux-iio <linux-iio@vger.kernel.org <mailto:linux-
> > > iio@vger.kernel.org> >; Jonathan Cameron
> > >       > <jic23@kernel.org <mailto:jic23@kernel.org> >; Hennerich,
> > > Michael
> > >       > <Michael.Hennerich@analog.com
> > > <mailto:Michael.Hennerich@analog.com> >; Lars-Peter Clausen
> > >       > <lars@metafoo.de <mailto:lars@metafoo.de> >
> > >       > Subject: Re: [PATCH] iio: ad5770r: make devicetree property
> > > reading
> > >       > consistent
> > >       >
> > >       > On Wed, Aug 11, 2021 at 10:46 AM Nuno Sá
> > > <nuno.sa@analog.com <mailto:nuno.sa@analog.com> >
> > >       > wrote:
> > >       > >
> > >       > > The bindings file for this driver is defining the property as
> > > 'reg' but
> > >       > > the driver was reading it with the 'num' name. This patches
> > > makes
> > >       > the
> > >       >
> > >       > "This patches makes the..." --> "Make the..."
> > >       >
> > >       > > driver consistent with what is defined in the bindings.
> > >       >
> > >       > While it seems okay, it may be now a chicken-egg issue
> > > (somebody
> > >       > created a DT with "num" property).
> > >       >
> > >
> > >       Arghh, I see. Well, maybe let's go the other way around and
> > > change the
> > >       bindings doc to 'num'?
> > >
> > >
> > > Not sure, like I said it’s a chicken-egg issue. Consult with Rob
> perhaps?
> >
> > Hi Rob,
> >
> > Could you give your input on this one?
> 
> There's no context, but I'm assuming this is in channel nodes. Keep

Sorry about that. Your assumption is correct, the binding is for a channel
node [1]. The driver just get's it as 'num' [2] which is not consistent.
Naively, I just though changing the driver to use reg would be enough
but Andy nicely raised the question of someone being already relying
on 'num'...

> the binding 'reg' and make the driver support both if needed.
> Considering the author of the binding also changed the binding from
> num to reg shortly after adding the binding, I don't think 'num'
> support is needed. If someone used 'num' and didn't run validation,
> well, that's their problem.
> 

So I guess the solution here is just to change the driver to support both
reg and num.

[1]: https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/iio/dac/adi,ad5770r.yaml#L67
[2]: https://elixir.bootlin.com/linux/latest/source/drivers/iio/dac/ad5770r.c#L525

- Nuno Sá

Re: [PATCH] iio: ad5770r: make devicetree property reading consistent

[Andy Shevchenko]

On Fri, Aug 13, 2021 at 10:47 AM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > From: Rob Herring <robh+dt@kernel.org>
> > Sent: Thursday, August 12, 2021 5:11 PM
> > On Thu, Aug 12, 2021 at 3:14 AM Sa, Nuno <Nuno.Sa@analog.com>
> > wrote:

...

> > > Could you give your input on this one?
> >
> > There's no context, but I'm assuming this is in channel nodes. Keep
>
> Sorry about that. Your assumption is correct, the binding is for a channel
> node [1]. The driver just get's it as 'num' [2] which is not consistent.
> Naively, I just though changing the driver to use reg would be enough
> but Andy nicely raised the question of someone being already relying
> on 'num'...
>
> > the binding 'reg' and make the driver support both if needed.
> > Considering the author of the binding also changed the binding from
> > num to reg shortly after adding the binding, I don't think 'num'
> > support is needed. If someone used 'num' and didn't run validation,
> > well, that's their problem.
> >
>
> So I guess the solution here is just to change the driver to support both
> reg and num.

As far as I got Rob's answer, if the binding never had the 'num',
dropping it from the driver is what we want now (actually your
original patch) and users, who are 'too much clever' :-) should have
had run validation for their DTs before production.

Taking this into account, I'm fine with the patch (but update a commit
message to summarize this discussion)
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] iio: ad5770r: make devicetree property reading consistent

[Sa, Nuno]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Friday, August 13, 2021 10:05 AM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Rob Herring <robh+dt@kernel.org>; linux-iio <linux-
> iio@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Lars-Peter
> Clausen <lars@metafoo.de>
> Subject: Re: [PATCH] iio: ad5770r: make devicetree property reading
> consistent
> 
> On Fri, Aug 13, 2021 at 10:47 AM Sa, Nuno <Nuno.Sa@analog.com>
> wrote:
> > > From: Rob Herring <robh+dt@kernel.org>
> > > Sent: Thursday, August 12, 2021 5:11 PM
> > > On Thu, Aug 12, 2021 at 3:14 AM Sa, Nuno <Nuno.Sa@analog.com>
> > > wrote:
> 
> ...
> 
> > > > Could you give your input on this one?
> > >
> > > There's no context, but I'm assuming this is in channel nodes. Keep
> >
> > Sorry about that. Your assumption is correct, the binding is for a
> channel
> > node [1]. The driver just get's it as 'num' [2] which is not consistent.
> > Naively, I just though changing the driver to use reg would be
> enough
> > but Andy nicely raised the question of someone being already relying
> > on 'num'...
> >
> > > the binding 'reg' and make the driver support both if needed.
> > > Considering the author of the binding also changed the binding
> from
> > > num to reg shortly after adding the binding, I don't think 'num'
> > > support is needed. If someone used 'num' and didn't run
> validation,
> > > well, that's their problem.
> > >
> >
> > So I guess the solution here is just to change the driver to support
> both
> > reg and num.
> 
> As far as I got Rob's answer, if the binding never had the 'num',
> dropping it from the driver is what we want now (actually your
> original patch) and users, who are 'too much clever' :-) should have
> had run validation for their DTs before production.
> 
> Taking this into account, I'm fine with the patch (but update a commit
> message to summarize this discussion)
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 

You're right... 
Jonathan, do you want a v2 with an updated commit message?

- Nuno Sá

Re: [PATCH] iio: ad5770r: make devicetree property reading consistent

[Jonathan Cameron]

On Fri, 13 Aug 2021 10:05:17 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > -----Original Message-----
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Friday, August 13, 2021 10:05 AM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: Rob Herring <robh+dt@kernel.org>; linux-iio <linux-  
> > iio@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;  
> > Hennerich, Michael <Michael.Hennerich@analog.com>; Lars-Peter
> > Clausen <lars@metafoo.de>
> > Subject: Re: [PATCH] iio: ad5770r: make devicetree property reading
> > consistent
> > 
> > On Fri, Aug 13, 2021 at 10:47 AM Sa, Nuno <Nuno.Sa@analog.com>
> > wrote:  
> > > > From: Rob Herring <robh+dt@kernel.org>
> > > > Sent: Thursday, August 12, 2021 5:11 PM
> > > > On Thu, Aug 12, 2021 at 3:14 AM Sa, Nuno <Nuno.Sa@analog.com>
> > > > wrote:  
> > 
> > ...
> >   
> > > > > Could you give your input on this one?  
> > > >
> > > > There's no context, but I'm assuming this is in channel nodes. Keep  
> > >
> > > Sorry about that. Your assumption is correct, the binding is for a  
> > channel  
> > > node [1]. The driver just get's it as 'num' [2] which is not consistent.
> > > Naively, I just though changing the driver to use reg would be  
> > enough  
> > > but Andy nicely raised the question of someone being already relying
> > > on 'num'...
> > >  
> > > > the binding 'reg' and make the driver support both if needed.
> > > > Considering the author of the binding also changed the binding  
> > from  
> > > > num to reg shortly after adding the binding, I don't think 'num'
> > > > support is needed. If someone used 'num' and didn't run  
> > validation,  
> > > > well, that's their problem.
> > > >  
> > >
> > > So I guess the solution here is just to change the driver to support  
> > both  
> > > reg and num.  
> > 
> > As far as I got Rob's answer, if the binding never had the 'num',
> > dropping it from the driver is what we want now (actually your
> > original patch) and users, who are 'too much clever' :-) should have
> > had run validation for their DTs before production.
> > 
> > Taking this into account, I'm fine with the patch (but update a commit
> > message to summarize this discussion)
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >   
> 
> You're right... 
> Jonathan, do you want a v2 with an updated commit message?

Please do. Also please add a fixes tag given we are treating it
as a fix.  If we discover someone is using the num variant then
we'll just have to support both values as a fix to the fix.
Not ideal, but as observed, hopefully people are validating the
DTs (which basically means no one is using this in production or
it would have been pointed out before).

Jonathan


> 
> - Nuno Sá

RE: [PATCH] iio: ad5770r: make devicetree property reading consistent

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, August 14, 2021 6:04 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; Rob Herring
> <robh+dt@kernel.org>; linux-iio <linux-iio@vger.kernel.org>;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Lars-Peter
> Clausen <lars@metafoo.de>
> Subject: Re: [PATCH] iio: ad5770r: make devicetree property reading
> consistent
> 
> [External]
> 
> On Fri, 13 Aug 2021 10:05:17 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Friday, August 13, 2021 10:05 AM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: Rob Herring <robh+dt@kernel.org>; linux-iio <linux-
> > > iio@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> > > Hennerich, Michael <Michael.Hennerich@analog.com>; Lars-Peter
> > > Clausen <lars@metafoo.de>
> > > Subject: Re: [PATCH] iio: ad5770r: make devicetree property
> reading
> > > consistent
> > >
> > > On Fri, Aug 13, 2021 at 10:47 AM Sa, Nuno <Nuno.Sa@analog.com>
> > > wrote:
> > > > > From: Rob Herring <robh+dt@kernel.org>
> > > > > Sent: Thursday, August 12, 2021 5:11 PM
> > > > > On Thu, Aug 12, 2021 at 3:14 AM Sa, Nuno
> <Nuno.Sa@analog.com>
> > > > > wrote:
> > >
> > > ...
> > >
> > > > > > Could you give your input on this one?
> > > > >
> > > > > There's no context, but I'm assuming this is in channel nodes.
> Keep
> > > >
> > > > Sorry about that. Your assumption is correct, the binding is for a
> > > channel
> > > > node [1]. The driver just get's it as 'num' [2] which is not
> consistent.
> > > > Naively, I just though changing the driver to use reg would be
> > > enough
> > > > but Andy nicely raised the question of someone being already
> relying
> > > > on 'num'...
> > > >
> > > > > the binding 'reg' and make the driver support both if needed.
> > > > > Considering the author of the binding also changed the binding
> > > from
> > > > > num to reg shortly after adding the binding, I don't think 'num'
> > > > > support is needed. If someone used 'num' and didn't run
> > > validation,
> > > > > well, that's their problem.
> > > > >
> > > >
> > > > So I guess the solution here is just to change the driver to support
> > > both
> > > > reg and num.
> > >
> > > As far as I got Rob's answer, if the binding never had the 'num',
> > > dropping it from the driver is what we want now (actually your
> > > original patch) and users, who are 'too much clever' :-) should have
> > > had run validation for their DTs before production.
> > >
> > > Taking this into account, I'm fine with the patch (but update a
> commit
> > > message to summarize this discussion)
> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > >
> >
> > You're right...
> > Jonathan, do you want a v2 with an updated commit message?
> 
> Please do. Also please add a fixes tag given we are treating it
> as a fix.  If we discover someone is using the num variant then
> we'll just have to support both values as a fix to the fix.
> Not ideal, but as observed, hopefully people are validating the
> DTs (which basically means no one is using this in production or
> it would have been pointed out before).
> 

Well, It seems we need to go through the support both 'num' and 'reg'
route... I did some git blaming and it turns out 'num' was actually supported
in the bindings [1]. After some time it was replaced by 'reg' [2] leaving the
driver unchanged... I guess we have a significant window of time here
where someone could deploy a *validated* devicetree using 'num'...
If no objections, on v2 I will just try to get 'reg' and if not present, fallback
to 'num' before erroring out.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ea52c21268e68cfdc1aabe686b154d73d8bf4d7e
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2cf3818f18b26992ff20a730df46e08e2485fd67

- Nuno Sá

Re: [PATCH] iio: ad5770r: make devicetree property reading consistent

[Andy Shevchenko]

On Mon, Aug 16, 2021 at 10:54 AM Sa, Nuno <Nuno.Sa@analog.com> wrote:

...

> Well, It seems we need to go through the support both 'num' and 'reg'
> route... I did some git blaming and it turns out 'num' was actually supported
> in the bindings [1]. After some time it was replaced by 'reg' [2] leaving the
> driver unchanged... I guess we have a significant window of time here
> where someone could deploy a *validated* devicetree using 'num'...
> If no objections, on v2 I will just try to get 'reg' and if not present, fallback
> to 'num' before erroring out.

Ouch, thanks for digging this out!

So, it means the following:
1) add "num" as _obsolete_ property to DT bindings
2) add support of "reg" to the driver

> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ea52c21268e68cfdc1aabe686b154d73d8bf4d7e
> [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2cf3818f18b26992ff20a730df46e08e2485fd67

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] iio: ad5770r: make devicetree property reading consistent

[Sa, Nuno]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Monday, August 16, 2021 10:10 AM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; Rob Herring
> <robh+dt@kernel.org>; linux-iio <linux-iio@vger.kernel.org>;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Lars-Peter
> Clausen <lars@metafoo.de>
> Subject: Re: [PATCH] iio: ad5770r: make devicetree property reading
> consistent
> 
> On Mon, Aug 16, 2021 at 10:54 AM Sa, Nuno <Nuno.Sa@analog.com>
> wrote:
> 
> ...
> 
> > Well, It seems we need to go through the support both 'num' and
> 'reg'
> > route... I did some git blaming and it turns out 'num' was actually
> supported
> > in the bindings [1]. After some time it was replaced by 'reg' [2]
> leaving the
> > driver unchanged... I guess we have a significant window of time
> here
> > where someone could deploy a *validated* devicetree using 'num'...
> > If no objections, on v2 I will just try to get 'reg' and if not present,
> fallback
> > to 'num' before erroring out.
> 
> Ouch, thanks for digging this out!
> 
> So, it means the following:
> 1) add "num" as _obsolete_ property to DT bindings
> 2) add support of "reg" to the driver
> 
> > [1]:
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/k
> ernel/git/torvalds/linux.git/commit/?id=ea52c21268e68cfdc1aabe686b
> 154d73d8bf4d7e__;!!A3Ni8CS0y2Y!pXYlkekser2PGbqrJZWpCXQ0oxp4
> OUNdOpRl03X9jRhgrj-jPaE_WB7hsAWEAA$
> > [2]:
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/k
> ernel/git/torvalds/linux.git/commit/?id=2cf3818f18b26992ff20a730df4
> 6e08e2485fd67__;!!A3Ni8CS0y2Y!pXYlkekser2PGbqrJZWpCXQ0oxp4O
> UNdOpRl03X9jRhgrj-jPaE_WB7e_v1D4g$
> 
> --
> With Best Regards,

Hi Rob,

As Andy suggested I'm trying to add the 'num' property back to the bindings
with a 'deprecated: true' value for the property. I was trying to get something
like (the main idea is to have either ref or num defined):

channel@0:
...
  properties:
    - oneof:
        - reg:
            ...
        - num:
            /* not sure if it makes sense to give the same values (description and possible values)
                 as for 'reg' */
            deprecated: true

However, I'm failing to get pass through validations. Is there a way to make this a oneOf as I'm
trying to? I'm feeling that these channel nodes should have a required key where we could use
oneOf 

Moreover, it seems that the validation enforces the 'reg' or 'ranges' for channel nodes
like this (maybe the fix [1] was actually because of this) so I wonder about my statement of
anyone having "validated" devicetrees with the old 'num' property...

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2cf3818f18b26992ff20a730df46e08e2485fd67
- Nuno Sá

Re: [PATCH] iio: ad5770r: make devicetree property reading consistent

[Rob Herring]

On Mon, Aug 16, 2021 at 2:54 AM Sa, Nuno <Nuno.Sa@analog.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Saturday, August 14, 2021 6:04 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; Rob Herring
> > <robh+dt@kernel.org>; linux-iio <linux-iio@vger.kernel.org>;
> > Hennerich, Michael <Michael.Hennerich@analog.com>; Lars-Peter
> > Clausen <lars@metafoo.de>
> > Subject: Re: [PATCH] iio: ad5770r: make devicetree property reading
> > consistent
> >
> > [External]
> >
> > On Fri, 13 Aug 2021 10:05:17 +0000
> > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> >
> > > > -----Original Message-----
> > > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > Sent: Friday, August 13, 2021 10:05 AM
> > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Cc: Rob Herring <robh+dt@kernel.org>; linux-iio <linux-
> > > > iio@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> > > > Hennerich, Michael <Michael.Hennerich@analog.com>; Lars-Peter
> > > > Clausen <lars@metafoo.de>
> > > > Subject: Re: [PATCH] iio: ad5770r: make devicetree property
> > reading
> > > > consistent
> > > >
> > > > On Fri, Aug 13, 2021 at 10:47 AM Sa, Nuno <Nuno.Sa@analog.com>
> > > > wrote:
> > > > > > From: Rob Herring <robh+dt@kernel.org>
> > > > > > Sent: Thursday, August 12, 2021 5:11 PM
> > > > > > On Thu, Aug 12, 2021 at 3:14 AM Sa, Nuno
> > <Nuno.Sa@analog.com>
> > > > > > wrote:
> > > >
> > > > ...
> > > >
> > > > > > > Could you give your input on this one?
> > > > > >
> > > > > > There's no context, but I'm assuming this is in channel nodes.
> > Keep
> > > > >
> > > > > Sorry about that. Your assumption is correct, the binding is for a
> > > > channel
> > > > > node [1]. The driver just get's it as 'num' [2] which is not
> > consistent.
> > > > > Naively, I just though changing the driver to use reg would be
> > > > enough
> > > > > but Andy nicely raised the question of someone being already
> > relying
> > > > > on 'num'...
> > > > >
> > > > > > the binding 'reg' and make the driver support both if needed.
> > > > > > Considering the author of the binding also changed the binding
> > > > from
> > > > > > num to reg shortly after adding the binding, I don't think 'num'
> > > > > > support is needed. If someone used 'num' and didn't run
> > > > validation,
> > > > > > well, that's their problem.
> > > > > >
> > > > >
> > > > > So I guess the solution here is just to change the driver to support
> > > > both
> > > > > reg and num.
> > > >
> > > > As far as I got Rob's answer, if the binding never had the 'num',
> > > > dropping it from the driver is what we want now (actually your
> > > > original patch) and users, who are 'too much clever' :-) should have
> > > > had run validation for their DTs before production.
> > > >
> > > > Taking this into account, I'm fine with the patch (but update a
> > commit
> > > > message to summarize this discussion)
> > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > >
> > >
> > > You're right...
> > > Jonathan, do you want a v2 with an updated commit message?
> >
> > Please do. Also please add a fixes tag given we are treating it
> > as a fix.  If we discover someone is using the num variant then
> > we'll just have to support both values as a fix to the fix.
> > Not ideal, but as observed, hopefully people are validating the
> > DTs (which basically means no one is using this in production or
> > it would have been pointed out before).
> >
>
> Well, It seems we need to go through the support both 'num' and 'reg'
> route... I did some git blaming and it turns out 'num' was actually supported
> in the bindings [1]. After some time it was replaced by 'reg' [2] leaving the
> driver unchanged... I guess we have a significant window of time here
> where someone could deploy a *validated* devicetree using 'num'...

No there wasn't. Both commits landed in v5.7.

> If no objections, on v2 I will just try to get 'reg' and if not present, fallback
> to 'num' before erroring out.

Unless a user turns up and complains, then I say drop 'num'.

Rob

RE: [PATCH] iio: ad5770r: make devicetree property reading consistent

[Sa, Nuno]

> -----Original Message-----
> From: Rob Herring <robh+dt@kernel.org>
> Sent: Monday, August 16, 2021 2:19 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; linux-iio <linux-iio@vger.kernel.org>;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Lars-Peter
> Clausen <lars@metafoo.de>
> Subject: Re: [PATCH] iio: ad5770r: make devicetree property reading
> consistent
> 
> On Mon, Aug 16, 2021 at 2:54 AM Sa, Nuno <Nuno.Sa@analog.com>
> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Saturday, August 14, 2021 6:04 PM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; Rob
> Herring
> > > <robh+dt@kernel.org>; linux-iio <linux-iio@vger.kernel.org>;
> > > Hennerich, Michael <Michael.Hennerich@analog.com>; Lars-Peter
> > > Clausen <lars@metafoo.de>
> > > Subject: Re: [PATCH] iio: ad5770r: make devicetree property
> reading
> > > consistent
> > >
> > > [External]
> > >
> > > On Fri, 13 Aug 2021 10:05:17 +0000
> > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > > Sent: Friday, August 13, 2021 10:05 AM
> > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > Cc: Rob Herring <robh+dt@kernel.org>; linux-iio <linux-
> > > > > iio@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> > > > > Hennerich, Michael <Michael.Hennerich@analog.com>; Lars-
> Peter
> > > > > Clausen <lars@metafoo.de>
> > > > > Subject: Re: [PATCH] iio: ad5770r: make devicetree property
> > > reading
> > > > > consistent
> > > > >
> > > > > On Fri, Aug 13, 2021 at 10:47 AM Sa, Nuno
> <Nuno.Sa@analog.com>
> > > > > wrote:
> > > > > > > From: Rob Herring <robh+dt@kernel.org>
> > > > > > > Sent: Thursday, August 12, 2021 5:11 PM
> > > > > > > On Thu, Aug 12, 2021 at 3:14 AM Sa, Nuno
> > > <Nuno.Sa@analog.com>
> > > > > > > wrote:
> > > > >
> > > > > ...
> > > > >
> > > > > > > > Could you give your input on this one?
> > > > > > >
> > > > > > > There's no context, but I'm assuming this is in channel
> nodes.
> > > Keep
> > > > > >
> > > > > > Sorry about that. Your assumption is correct, the binding is for
> a
> > > > > channel
> > > > > > node [1]. The driver just get's it as 'num' [2] which is not
> > > consistent.
> > > > > > Naively, I just though changing the driver to use reg would be
> > > > > enough
> > > > > > but Andy nicely raised the question of someone being already
> > > relying
> > > > > > on 'num'...
> > > > > >
> > > > > > > the binding 'reg' and make the driver support both if
> needed.
> > > > > > > Considering the author of the binding also changed the
> binding
> > > > > from
> > > > > > > num to reg shortly after adding the binding, I don't think
> 'num'
> > > > > > > support is needed. If someone used 'num' and didn't run
> > > > > validation,
> > > > > > > well, that's their problem.
> > > > > > >
> > > > > >
> > > > > > So I guess the solution here is just to change the driver to
> support
> > > > > both
> > > > > > reg and num.
> > > > >
> > > > > As far as I got Rob's answer, if the binding never had the 'num',
> > > > > dropping it from the driver is what we want now (actually your
> > > > > original patch) and users, who are 'too much clever' :-) should
> have
> > > > > had run validation for their DTs before production.
> > > > >
> > > > > Taking this into account, I'm fine with the patch (but update a
> > > commit
> > > > > message to summarize this discussion)
> > > > > Reviewed-by: Andy Shevchenko
> <andy.shevchenko@gmail.com>
> > > > >
> > > >
> > > > You're right...
> > > > Jonathan, do you want a v2 with an updated commit message?
> > >
> > > Please do. Also please add a fixes tag given we are treating it
> > > as a fix.  If we discover someone is using the num variant then
> > > we'll just have to support both values as a fix to the fix.
> > > Not ideal, but as observed, hopefully people are validating the
> > > DTs (which basically means no one is using this in production or
> > > it would have been pointed out before).
> > >
> >
> > Well, It seems we need to go through the support both 'num' and
> 'reg'
> > route... I did some git blaming and it turns out 'num' was actually
> supported
> > in the bindings [1]. After some time it was replaced by 'reg' [2]
> leaving the
> > driver unchanged... I guess we have a significant window of time
> here
> > where someone could deploy a *validated* devicetree using 'num'...
> 
> No there wasn't. Both commits landed in v5.7.

Ahh I see. I just looked to dates without thinking in release cycles...

> > If no objections, on v2 I will just try to get 'reg' and if not present,
> fallback
> > to 'num' before erroring out.
> 
> Unless a user turns up and complains, then I say drop 'num'.
> 

Ok, thanks! That's easier for me...

- Nuno Sá