| Message ID | 20210811074827.21889-1-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iio: ad5770r: make devicetree property reading consistent | expand |
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).
> 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á
> -----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á
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
> -----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á
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>
> -----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á
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á
> -----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á
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
> -----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á
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
> -----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á
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) {
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(-)