diff mbox series

[v3,36/47] mfd: ti_am335x_tscadc: Support the correctly spelled DT property

Message ID 20210915155908.476767-37-miquel.raynal@bootlin.com (mailing list archive)
State Not Applicable
Headers show
Series TI AM437X ADC1 | expand

Commit Message

Miquel Raynal Sept. 15, 2021, 3:58 p.m. UTC
There was in the past a typo in the coordinate readouts property. The
bindings have been updated, the touchscreen driver as well and now
supports both. However, the MFD driver that is in charge of verifying
the validity of the property only checks the bogus one. Add support for
the correctly spelled DT property.

Fixes: c9aeb249bf72 ("Input: ti_am335x_tsc - fix spelling mistake in TSC/ADC DT binding")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Lee Jones Sept. 22, 2021, 3:57 p.m. UTC | #1
On Wed, 15 Sep 2021, Miquel Raynal wrote:

> There was in the past a typo in the coordinate readouts property. The
> bindings have been updated, the touchscreen driver as well and now
> supports both. However, the MFD driver that is in charge of verifying
> the validity of the property only checks the bogus one. Add support for
> the correctly spelled DT property.
> 
> Fixes: c9aeb249bf72 ("Input: ti_am335x_tsc - fix spelling mistake in TSC/ADC DT binding")
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/mfd/ti_am335x_tscadc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> index 155a8ed879b3..aa46ed669016 100644
> --- a/drivers/mfd/ti_am335x_tscadc.c
> +++ b/drivers/mfd/ti_am335x_tscadc.c
> @@ -144,8 +144,14 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  	if (tscadc->data->has_tsc) {
>  		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
>  		of_property_read_u32(node, "ti,wires", &tscmag_wires);
> -		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
> +		err = of_property_read_u32(node, "ti,coordinate-readouts",
> +					   &readouts);
> +		if (err < 0)
> +			of_property_read_u32(node, "ti,coordiante-readouts",
> +					     &readouts);
> +

How long are you proposing that we support this churn?

>  		of_node_put(node);
> +
>  		if (tscmag_wires)
>  			use_tsc = true;
>  	} else {
Miquel Raynal Sept. 23, 2021, 8:19 a.m. UTC | #2
Hi Lee,

lee.jones@linaro.org wrote on Wed, 22 Sep 2021 16:57:10 +0100:

> On Wed, 15 Sep 2021, Miquel Raynal wrote:
> 
> > There was in the past a typo in the coordinate readouts property. The
> > bindings have been updated, the touchscreen driver as well and now
> > supports both. However, the MFD driver that is in charge of verifying
> > the validity of the property only checks the bogus one. Add support for
> > the correctly spelled DT property.
> > 
> > Fixes: c9aeb249bf72 ("Input: ti_am335x_tsc - fix spelling mistake in TSC/ADC DT binding")
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  drivers/mfd/ti_am335x_tscadc.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> > index 155a8ed879b3..aa46ed669016 100644
> > --- a/drivers/mfd/ti_am335x_tscadc.c
> > +++ b/drivers/mfd/ti_am335x_tscadc.c
> > @@ -144,8 +144,14 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> >  	if (tscadc->data->has_tsc) {
> >  		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
> >  		of_property_read_u32(node, "ti,wires", &tscmag_wires);
> > -		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
> > +		err = of_property_read_u32(node, "ti,coordinate-readouts",
> > +					   &readouts);
> > +		if (err < 0)
> > +			of_property_read_u32(node, "ti,coordiante-readouts",
> > +					     &readouts);
> > +  
> 
> How long are you proposing that we support this churn?

Well, I am not proposing anything, I am just "fixing" the driver so
that it fits the bindings :) Given the fact that at the end of this
series there is a patch that changes the "coordiante" typo to
"coordinate" in a device tree source file, I believe it is still too
soon...


Thanks,
Miquèl
Lee Jones Sept. 23, 2021, 9:11 a.m. UTC | #3
On Thu, 23 Sep 2021, Miquel Raynal wrote:

> Hi Lee,
> 
> lee.jones@linaro.org wrote on Wed, 22 Sep 2021 16:57:10 +0100:
> 
> > On Wed, 15 Sep 2021, Miquel Raynal wrote:
> > 
> > > There was in the past a typo in the coordinate readouts property. The
> > > bindings have been updated, the touchscreen driver as well and now
> > > supports both. However, the MFD driver that is in charge of verifying
> > > the validity of the property only checks the bogus one. Add support for
> > > the correctly spelled DT property.
> > > 
> > > Fixes: c9aeb249bf72 ("Input: ti_am335x_tsc - fix spelling mistake in TSC/ADC DT binding")
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > ---
> > >  drivers/mfd/ti_am335x_tscadc.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> > > index 155a8ed879b3..aa46ed669016 100644
> > > --- a/drivers/mfd/ti_am335x_tscadc.c
> > > +++ b/drivers/mfd/ti_am335x_tscadc.c
> > > @@ -144,8 +144,14 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> > >  	if (tscadc->data->has_tsc) {
> > >  		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
> > >  		of_property_read_u32(node, "ti,wires", &tscmag_wires);
> > > -		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
> > > +		err = of_property_read_u32(node, "ti,coordinate-readouts",
> > > +					   &readouts);
> > > +		if (err < 0)
> > > +			of_property_read_u32(node, "ti,coordiante-readouts",
> > > +					     &readouts);
> > > +  
> > 
> > How long are you proposing that we support this churn?
> 
> Well, I am not proposing anything, I am just "fixing" the driver so
> that it fits the bindings :) Given the fact that at the end of this
> series there is a patch that changes the "coordiante" typo to
> "coordinate" in a device tree source file, I believe it is still too
> soon...

If this is something you're changing in this set, please reconsider.

I'd rather have a slightly misspelled documented property than being
forced to support 2 for any length of time.
Miquel Raynal Sept. 23, 2021, 9:33 a.m. UTC | #4
Hi Lee,

lee.jones@linaro.org wrote on Thu, 23 Sep 2021 10:11:13 +0100:

> On Thu, 23 Sep 2021, Miquel Raynal wrote:
> 
> > Hi Lee,
> > 
> > lee.jones@linaro.org wrote on Wed, 22 Sep 2021 16:57:10 +0100:
> >   
> > > On Wed, 15 Sep 2021, Miquel Raynal wrote:
> > >   
> > > > There was in the past a typo in the coordinate readouts property. The
> > > > bindings have been updated, the touchscreen driver as well and now
> > > > supports both. However, the MFD driver that is in charge of verifying
> > > > the validity of the property only checks the bogus one. Add support for
> > > > the correctly spelled DT property.
> > > > 
> > > > Fixes: c9aeb249bf72 ("Input: ti_am335x_tsc - fix spelling mistake in TSC/ADC DT binding")
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > ---
> > > >  drivers/mfd/ti_am335x_tscadc.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> > > > index 155a8ed879b3..aa46ed669016 100644
> > > > --- a/drivers/mfd/ti_am335x_tscadc.c
> > > > +++ b/drivers/mfd/ti_am335x_tscadc.c
> > > > @@ -144,8 +144,14 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> > > >  	if (tscadc->data->has_tsc) {
> > > >  		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
> > > >  		of_property_read_u32(node, "ti,wires", &tscmag_wires);
> > > > -		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
> > > > +		err = of_property_read_u32(node, "ti,coordinate-readouts",
> > > > +					   &readouts);
> > > > +		if (err < 0)
> > > > +			of_property_read_u32(node, "ti,coordiante-readouts",
> > > > +					     &readouts);
> > > > +    
> > > 
> > > How long are you proposing that we support this churn?  
> > 
> > Well, I am not proposing anything, I am just "fixing" the driver so
> > that it fits the bindings :) Given the fact that at the end of this
> > series there is a patch that changes the "coordiante" typo to
> > "coordinate" in a device tree source file, I believe it is still too
> > soon...  
> 
> If this is something you're changing in this set, please reconsider.
> 
> I'd rather have a slightly misspelled documented property than being
> forced to support 2 for any length of time.

I am not suggesting anything here:
- "coordiante" was introduced back in 2014.
- "coordinate" was introduced shortly later.
- "coordiante" was considered part of the "stable API" back in 2014 and
  even though the right spelling got introduced very shortly after, DT
  stability rules wanted us to support it forever.
- The touchscreen driver has immediately be fixed to support both but
  not the MFD driver, and this does not make any sense!

We *should* either support only one property (1) or support both in the
two drivers (2), but supporting the two in one driver and supporting
only one in the other one does not make any sense (given the fact that
these two drives are tied together, the touchscreen driver does not
exist without the MFD driver). That is what I am fixing here.

Is #1 valid? Theoretically it's the best scenario. In practice it is
not (yet) possible because the two versions are still used in the
mainline device trees:
$ git grep coordiante-readouts -- arch/arm/boot/dts/ | wc -l
1
$ git grep coordinate-readouts -- arch/arm/boot/dts/ | wc -l
5

So in this series I am fixing the MFD driver to be sure it handles
correctly the correctly spelled DT property which is used by 5 boards
since at least 6 years and I am also updating the remaining DT to use
the correctly spelled property as well.

I suppose we could propose to drop support for the "coordiante"
version of that property in a few years but if we decide to do it right
now we're definitely gonna break users.

Thanks,
Miquèl
Lee Jones Sept. 27, 2021, 8:09 a.m. UTC | #5
On Thu, 23 Sep 2021, Miquel Raynal wrote:

> Hi Lee,
> 
> lee.jones@linaro.org wrote on Thu, 23 Sep 2021 10:11:13 +0100:
> 
> > On Thu, 23 Sep 2021, Miquel Raynal wrote:
> > 
> > > Hi Lee,
> > > 
> > > lee.jones@linaro.org wrote on Wed, 22 Sep 2021 16:57:10 +0100:
> > >   
> > > > On Wed, 15 Sep 2021, Miquel Raynal wrote:
> > > >   
> > > > > There was in the past a typo in the coordinate readouts property. The
> > > > > bindings have been updated, the touchscreen driver as well and now
> > > > > supports both. However, the MFD driver that is in charge of verifying
> > > > > the validity of the property only checks the bogus one. Add support for
> > > > > the correctly spelled DT property.
> > > > > 
> > > > > Fixes: c9aeb249bf72 ("Input: ti_am335x_tsc - fix spelling mistake in TSC/ADC DT binding")
> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > ---
> > > > >  drivers/mfd/ti_am335x_tscadc.c | 8 +++++++-
> > > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> > > > > index 155a8ed879b3..aa46ed669016 100644
> > > > > --- a/drivers/mfd/ti_am335x_tscadc.c
> > > > > +++ b/drivers/mfd/ti_am335x_tscadc.c
> > > > > @@ -144,8 +144,14 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> > > > >  	if (tscadc->data->has_tsc) {
> > > > >  		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
> > > > >  		of_property_read_u32(node, "ti,wires", &tscmag_wires);
> > > > > -		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
> > > > > +		err = of_property_read_u32(node, "ti,coordinate-readouts",
> > > > > +					   &readouts);
> > > > > +		if (err < 0)
> > > > > +			of_property_read_u32(node, "ti,coordiante-readouts",
> > > > > +					     &readouts);
> > > > > +    
> > > > 
> > > > How long are you proposing that we support this churn?  
> > > 
> > > Well, I am not proposing anything, I am just "fixing" the driver so
> > > that it fits the bindings :) Given the fact that at the end of this
> > > series there is a patch that changes the "coordiante" typo to
> > > "coordinate" in a device tree source file, I believe it is still too
> > > soon...  
> > 
> > If this is something you're changing in this set, please reconsider.
> > 
> > I'd rather have a slightly misspelled documented property than being
> > forced to support 2 for any length of time.
> 
> I am not suggesting anything here:
> - "coordiante" was introduced back in 2014.
> - "coordinate" was introduced shortly later.
> - "coordiante" was considered part of the "stable API" back in 2014 and
>   even though the right spelling got introduced very shortly after, DT
>   stability rules wanted us to support it forever.
> - The touchscreen driver has immediately be fixed to support both but
>   not the MFD driver, and this does not make any sense!
> 
> We *should* either support only one property (1) or support both in the
> two drivers (2), but supporting the two in one driver and supporting
> only one in the other one does not make any sense (given the fact that
> these two drives are tied together, the touchscreen driver does not
> exist without the MFD driver). That is what I am fixing here.
> 
> Is #1 valid? Theoretically it's the best scenario. In practice it is
> not (yet) possible because the two versions are still used in the
> mainline device trees:
> $ git grep coordiante-readouts -- arch/arm/boot/dts/ | wc -l
> 1
> $ git grep coordinate-readouts -- arch/arm/boot/dts/ | wc -l
> 5
> 
> So in this series I am fixing the MFD driver to be sure it handles
> correctly the correctly spelled DT property which is used by 5 boards
> since at least 6 years and I am also updating the remaining DT to use
> the correctly spelled property as well.
> 
> I suppose we could propose to drop support for the "coordiante"
> version of that property in a few years but if we decide to do it right
> now we're definitely gonna break users.

It's a shame that the misspelled one is still in use.  If they were
all changed over at the start, there would be a strong case for
dropping support for it.
Miquel Raynal Sept. 27, 2021, 8:18 a.m. UTC | #6
Hi Lee,

lee.jones@linaro.org wrote on Mon, 27 Sep 2021 09:09:32 +0100:

> On Thu, 23 Sep 2021, Miquel Raynal wrote:
> 
> > Hi Lee,
> > 
> > lee.jones@linaro.org wrote on Thu, 23 Sep 2021 10:11:13 +0100:
> >   
> > > On Thu, 23 Sep 2021, Miquel Raynal wrote:
> > >   
> > > > Hi Lee,
> > > > 
> > > > lee.jones@linaro.org wrote on Wed, 22 Sep 2021 16:57:10 +0100:
> > > >     
> > > > > On Wed, 15 Sep 2021, Miquel Raynal wrote:
> > > > >     
> > > > > > There was in the past a typo in the coordinate readouts property. The
> > > > > > bindings have been updated, the touchscreen driver as well and now
> > > > > > supports both. However, the MFD driver that is in charge of verifying
> > > > > > the validity of the property only checks the bogus one. Add support for
> > > > > > the correctly spelled DT property.
> > > > > > 
> > > > > > Fixes: c9aeb249bf72 ("Input: ti_am335x_tsc - fix spelling mistake in TSC/ADC DT binding")
> > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > ---
> > > > > >  drivers/mfd/ti_am335x_tscadc.c | 8 +++++++-
> > > > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> > > > > > index 155a8ed879b3..aa46ed669016 100644
> > > > > > --- a/drivers/mfd/ti_am335x_tscadc.c
> > > > > > +++ b/drivers/mfd/ti_am335x_tscadc.c
> > > > > > @@ -144,8 +144,14 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> > > > > >  	if (tscadc->data->has_tsc) {
> > > > > >  		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
> > > > > >  		of_property_read_u32(node, "ti,wires", &tscmag_wires);
> > > > > > -		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
> > > > > > +		err = of_property_read_u32(node, "ti,coordinate-readouts",
> > > > > > +					   &readouts);
> > > > > > +		if (err < 0)
> > > > > > +			of_property_read_u32(node, "ti,coordiante-readouts",
> > > > > > +					     &readouts);
> > > > > > +      
> > > > > 
> > > > > How long are you proposing that we support this churn?    
> > > > 
> > > > Well, I am not proposing anything, I am just "fixing" the driver so
> > > > that it fits the bindings :) Given the fact that at the end of this
> > > > series there is a patch that changes the "coordiante" typo to
> > > > "coordinate" in a device tree source file, I believe it is still too
> > > > soon...    
> > > 
> > > If this is something you're changing in this set, please reconsider.
> > > 
> > > I'd rather have a slightly misspelled documented property than being
> > > forced to support 2 for any length of time.  
> > 
> > I am not suggesting anything here:
> > - "coordiante" was introduced back in 2014.
> > - "coordinate" was introduced shortly later.
> > - "coordiante" was considered part of the "stable API" back in 2014 and
> >   even though the right spelling got introduced very shortly after, DT
> >   stability rules wanted us to support it forever.
> > - The touchscreen driver has immediately be fixed to support both but
> >   not the MFD driver, and this does not make any sense!
> > 
> > We *should* either support only one property (1) or support both in the
> > two drivers (2), but supporting the two in one driver and supporting
> > only one in the other one does not make any sense (given the fact that
> > these two drives are tied together, the touchscreen driver does not
> > exist without the MFD driver). That is what I am fixing here.
> > 
> > Is #1 valid? Theoretically it's the best scenario. In practice it is
> > not (yet) possible because the two versions are still used in the
> > mainline device trees:
> > $ git grep coordiante-readouts -- arch/arm/boot/dts/ | wc -l
> > 1
> > $ git grep coordinate-readouts -- arch/arm/boot/dts/ | wc -l
> > 5
> > 
> > So in this series I am fixing the MFD driver to be sure it handles
> > correctly the correctly spelled DT property which is used by 5 boards
> > since at least 6 years and I am also updating the remaining DT to use
> > the correctly spelled property as well.
> > 
> > I suppose we could propose to drop support for the "coordiante"
> > version of that property in a few years but if we decide to do it right
> > now we're definitely gonna break users.  
> 
> It's a shame that the misspelled one is still in use.  If they were
> all changed over at the start, there would be a strong case for
> dropping support for it.

It is, clearly. I believe all DT got changed in one go, but a
contributor at the same time proposed a new DT with the legacy
property "copy-pasted" from the "existing" device trees and this went
off the radar...

Anyway, I'll send a v4 soon with all your suggestions taken into
account.

Thanks,
Miquèl
diff mbox series

Patch

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 155a8ed879b3..aa46ed669016 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -144,8 +144,14 @@  static	int ti_tscadc_probe(struct platform_device *pdev)
 	if (tscadc->data->has_tsc) {
 		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
 		of_property_read_u32(node, "ti,wires", &tscmag_wires);
-		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
+		err = of_property_read_u32(node, "ti,coordinate-readouts",
+					   &readouts);
+		if (err < 0)
+			of_property_read_u32(node, "ti,coordiante-readouts",
+					     &readouts);
+
 		of_node_put(node);
+
 		if (tscmag_wires)
 			use_tsc = true;
 	} else {