diff mbox series

[6/8] iio: dac: ad3552r-hs: exit for error on wrong chip id

Message ID 20241216-wip-bl-ad3552r-axi-v0-iio-testing-carlos-v1-6-856ff71fc930@baylibre.com (mailing list archive)
State Changes Requested
Headers show
Series iio: ad3552r-hs: add support for ad3541/42r | expand

Commit Message

Angelo Dureghello Dec. 16, 2024, 8:36 p.m. UTC 
From: Angelo Dureghello <adureghello@baylibre.com>

Exit for error on wrong chip id, otherwise driver continues
with wrong assumptions.

Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 drivers/iio/dac/ad3552r-hs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Jonathan Cameron Dec. 19, 2024, 4:54 p.m. UTC | #1 
On Mon, 16 Dec 2024 21:36:26 +0100
Angelo Dureghello <adureghello@baylibre.com> wrote:

> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> Exit for error on wrong chip id, otherwise driver continues
> with wrong assumptions.
Why? Chip ID does not define all future compatible parts, just the
ones we know about today.

The reason not failing is that the moment we do exit on a mismatch
we can never support fallback device tree compatible IDs.  Is there
no chance that ADI will release a backwards compatible part in the
future that we'd like to work with old kernels?

Any mismatch in DT vs hardware present is considered a firmware
bug, not a kernel problem.
We used to reject missmatched IDs but after a long discussion with
DT maintainers it became clear that broke their model.

Thanks,

Jonathan

> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
>  drivers/iio/dac/ad3552r-hs.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> index 8974df625670..e613eee7fc11 100644
> --- a/drivers/iio/dac/ad3552r-hs.c
> +++ b/drivers/iio/dac/ad3552r-hs.c
> @@ -326,8 +326,9 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
>  
>  	id |= val << 8;
>  	if (id != st->model_data->chip_id)
> -		dev_info(st->dev, "Chip ID error. Expected 0x%x, Read 0x%x\n",
> -			 AD3552R_ID, id);
> +		return dev_err_probe(st->dev, -ENODEV,
> +				     "chip id error, expected 0x%x, got 0x%x\n",
> +				     st->model_data->chip_id, id);
>  
>  	/* Clear reset error flag, see ad3552r manual, rev B table 38. */
>  	ret = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_ERR_STATUS,
>
Conor Dooley Dec. 20, 2024, 9:55 a.m. UTC | #2 
On Thu, Dec 19, 2024 at 04:54:46PM +0000, Jonathan Cameron wrote:
> On Mon, 16 Dec 2024 21:36:26 +0100
> Angelo Dureghello <adureghello@baylibre.com> wrote:
> 
> > From: Angelo Dureghello <adureghello@baylibre.com>
> > 
> > Exit for error on wrong chip id, otherwise driver continues
> > with wrong assumptions.
> Why? Chip ID does not define all future compatible parts, just the
> ones we know about today.
> 
> The reason not failing is that the moment we do exit on a mismatch
> we can never support fallback device tree compatible IDs.  Is there
> no chance that ADI will release a backwards compatible part in the
> future that we'd like to work with old kernels?
> 
> Any mismatch in DT vs hardware present is considered a firmware
> bug, not a kernel problem.
> We used to reject missmatched IDs but after a long discussion with
> DT maintainers it became clear that broke their model.

I'd probably still say the warning of the info message is still too
harsh, and that it should be something like "Chip ID mismatch, operating
detected 0x%x as a 0x%x" ;)

> Thanks,
> 
> Jonathan
> 
> > 
> > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > ---
> >  drivers/iio/dac/ad3552r-hs.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> > index 8974df625670..e613eee7fc11 100644
> > --- a/drivers/iio/dac/ad3552r-hs.c
> > +++ b/drivers/iio/dac/ad3552r-hs.c
> > @@ -326,8 +326,9 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
> >  
> >  	id |= val << 8;
> >  	if (id != st->model_data->chip_id)
> > -		dev_info(st->dev, "Chip ID error. Expected 0x%x, Read 0x%x\n",
> > -			 AD3552R_ID, id);
> > +		return dev_err_probe(st->dev, -ENODEV,
> > +				     "chip id error, expected 0x%x, got 0x%x\n",
> > +				     st->model_data->chip_id, id);
> >  
> >  	/* Clear reset error flag, see ad3552r manual, rev B table 38. */
> >  	ret = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_ERR_STATUS,
> > 
>
Angelo Dureghello Dec. 20, 2024, 2:25 p.m. UTC | #3 
On 19.12.2024 16:54, Jonathan Cameron wrote:
> On Mon, 16 Dec 2024 21:36:26 +0100
> Angelo Dureghello <adureghello@baylibre.com> wrote:
> 
> > From: Angelo Dureghello <adureghello@baylibre.com>
> > 
> > Exit for error on wrong chip id, otherwise driver continues
> > with wrong assumptions.
> Why? Chip ID does not define all future compatible parts, just the
> ones we know about today.
> 
> The reason not failing is that the moment we do exit on a mismatch
> we can never support fallback device tree compatible IDs.  Is there
> no chance that ADI will release a backwards compatible part in the
> future that we'd like to work with old kernels?
>
> Any mismatch in DT vs hardware present is considered a firmware
> bug, not a kernel problem.
> We used to reject missmatched IDs but after a long discussion with
> DT maintainers it became clear that broke their model.
> 
Ok, i will apply to what decided so.  

> Thanks,
> 
> Jonathan
> 
> > 
> > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > ---
> >  drivers/iio/dac/ad3552r-hs.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> > index 8974df625670..e613eee7fc11 100644
> > --- a/drivers/iio/dac/ad3552r-hs.c
> > +++ b/drivers/iio/dac/ad3552r-hs.c
> > @@ -326,8 +326,9 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
> >  
> >  	id |= val << 8;
> >  	if (id != st->model_data->chip_id)
> > -		dev_info(st->dev, "Chip ID error. Expected 0x%x, Read 0x%x\n",
> > -			 AD3552R_ID, id);
> > +		return dev_err_probe(st->dev, -ENODEV,
> > +				     "chip id error, expected 0x%x, got 0x%x\n",
> > +				     st->model_data->chip_id, id);
> >  
> >  	/* Clear reset error flag, see ad3552r manual, rev B table 38. */
> >  	ret = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_ERR_STATUS,
> > 
>
diff mbox series

Patch

diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
index 8974df625670..e613eee7fc11 100644
--- a/drivers/iio/dac/ad3552r-hs.c
+++ b/drivers/iio/dac/ad3552r-hs.c
@@ -326,8 +326,9 @@  static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
 
 	id |= val << 8;
 	if (id != st->model_data->chip_id)
-		dev_info(st->dev, "Chip ID error. Expected 0x%x, Read 0x%x\n",
-			 AD3552R_ID, id);
+		return dev_err_probe(st->dev, -ENODEV,
+				     "chip id error, expected 0x%x, got 0x%x\n",
+				     st->model_data->chip_id, id);
 
 	/* Clear reset error flag, see ad3552r manual, rev B table 38. */
 	ret = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_ERR_STATUS,