diff mbox series

[v2,06/16] iio: adc: ad7768-1: set MOSI idle state to prevent accidental reset

Message ID b99ec75a9f8a47df72a0e8cf126d7e9312ce59c1.1737985435.git.Jonathan.Santos@analog.com (mailing list archive)
State Changes Requested
Headers show
Series Add features, improvements, and fixes | expand

Commit Message

Jonathan Santos Jan. 27, 2025, 3:12 p.m. UTC
Datasheet recommends Setting the MOSI idle state to high in order to
prevent accidental reset of the device when SCLK is free running.
This happens when the controller clocks out a 1 followed by 63 zeros
while the CS is held low.

Check if SPI controller supports SPI_MOSI_IDLE_HIGH flag and set it.

Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support")
Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
v2 Changes:
* Only setup SPI_MOSI_IDLE_HIGH flag if the controller supports it, otherwise the driver
  continues the same. I realized that using bits_per_word does not avoid the problem that
  MOSI idle state is trying to solve. If the controller drives the MOSI low between bytes
  during a transfer, nothing happens.
---
 drivers/iio/adc/ad7768-1.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Jonathan Cameron Feb. 1, 2025, 3:31 p.m. UTC | #1
On Mon, 27 Jan 2025 12:12:29 -0300
Jonathan Santos <Jonathan.Santos@analog.com> wrote:

> Datasheet recommends Setting the MOSI idle state to high in order to
> prevent accidental reset of the device when SCLK is free running.
> This happens when the controller clocks out a 1 followed by 63 zeros
> while the CS is held low.
> 
> Check if SPI controller supports SPI_MOSI_IDLE_HIGH flag and set it.
> 
> Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support")
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>

Ideally drag fix to start of patch set to make it obvious it can be
picked up without the rest.  I'm not grabbing it yet as doesn't
feel like it has enough review yet.

> ---
> v2 Changes:
> * Only setup SPI_MOSI_IDLE_HIGH flag if the controller supports it, otherwise the driver
>   continues the same. I realized that using bits_per_word does not avoid the problem that
>   MOSI idle state is trying to solve. If the controller drives the MOSI low between bytes
>   during a transfer, nothing happens.
> ---
>  drivers/iio/adc/ad7768-1.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index c3cf04311c40..95ba89435652 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -574,6 +574,22 @@ static int ad7768_probe(struct spi_device *spi)
>  		return -ENOMEM;
>  
>  	st = iio_priv(indio_dev);
> +	/*
> +	 * Datasheet recommends SDI line to be kept high when
> +	 * data is not being clocked out of the controller

Very short line wrap.  Go up to 80 chars.  If this picks up
some review, I can fix that whilst applying. If not do it for
your v3.

> +	 * and the spi clock is free running, to prevent
> +	 * accidental reset.
> +	 * Since many controllers do not support the
> +	 * SPI_MOSI_IDLE_HIGH flag yet, only request the MOSI
> +	 * idle state to enable if the controller supports it.
> +	 */
> +	if (spi->controller->mode_bits & SPI_MOSI_IDLE_HIGH) {
> +		spi->mode |= SPI_MOSI_IDLE_HIGH;
> +		ret = spi_setup(spi);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	st->spi = spi;
>  
>  	st->vref = devm_regulator_get(&spi->dev, "vref");
Jonathan Santos Feb. 3, 2025, 11:34 a.m. UTC | #2
On 02/01, Jonathan Cameron wrote:
> On Mon, 27 Jan 2025 12:12:29 -0300
> Jonathan Santos <Jonathan.Santos@analog.com> wrote:
> 
> > Datasheet recommends Setting the MOSI idle state to high in order to
> > prevent accidental reset of the device when SCLK is free running.
> > This happens when the controller clocks out a 1 followed by 63 zeros
> > while the CS is held low.
> > 
> > Check if SPI controller supports SPI_MOSI_IDLE_HIGH flag and set it.
> > 
> > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support")
> > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> 
> Ideally drag fix to start of patch set to make it obvious it can be
> picked up without the rest.  I'm not grabbing it yet as doesn't
> feel like it has enough review yet.
> 
Sure, will do.
> > ---
> > v2 Changes:
> > * Only setup SPI_MOSI_IDLE_HIGH flag if the controller supports it, otherwise the driver
> >   continues the same. I realized that using bits_per_word does not avoid the problem that
> >   MOSI idle state is trying to solve. If the controller drives the MOSI low between bytes
> >   during a transfer, nothing happens.
> > ---
> >  drivers/iio/adc/ad7768-1.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> > index c3cf04311c40..95ba89435652 100644
> > --- a/drivers/iio/adc/ad7768-1.c
> > +++ b/drivers/iio/adc/ad7768-1.c
> > @@ -574,6 +574,22 @@ static int ad7768_probe(struct spi_device *spi)
> >  		return -ENOMEM;
> >  
> >  	st = iio_priv(indio_dev);
> > +	/*
> > +	 * Datasheet recommends SDI line to be kept high when
> > +	 * data is not being clocked out of the controller
> 
> Very short line wrap.  Go up to 80 chars.  If this picks up
> some review, I can fix that whilst applying. If not do it for
> your v3.
> 
Right, i will do it in the v3.
> > +	 * and the spi clock is free running, to prevent
> > +	 * accidental reset.
> > +	 * Since many controllers do not support the
> > +	 * SPI_MOSI_IDLE_HIGH flag yet, only request the MOSI
> > +	 * idle state to enable if the controller supports it.
> > +	 */
> > +	if (spi->controller->mode_bits & SPI_MOSI_IDLE_HIGH) {
> > +		spi->mode |= SPI_MOSI_IDLE_HIGH;
> > +		ret = spi_setup(spi);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> >  	st->spi = spi;
> >  
> >  	st->vref = devm_regulator_get(&spi->dev, "vref");
>
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index c3cf04311c40..95ba89435652 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -574,6 +574,22 @@  static int ad7768_probe(struct spi_device *spi)
 		return -ENOMEM;
 
 	st = iio_priv(indio_dev);
+	/*
+	 * Datasheet recommends SDI line to be kept high when
+	 * data is not being clocked out of the controller
+	 * and the spi clock is free running, to prevent
+	 * accidental reset.
+	 * Since many controllers do not support the
+	 * SPI_MOSI_IDLE_HIGH flag yet, only request the MOSI
+	 * idle state to enable if the controller supports it.
+	 */
+	if (spi->controller->mode_bits & SPI_MOSI_IDLE_HIGH) {
+		spi->mode |= SPI_MOSI_IDLE_HIGH;
+		ret = spi_setup(spi);
+		if (ret < 0)
+			return ret;
+	}
+
 	st->spi = spi;
 
 	st->vref = devm_regulator_get(&spi->dev, "vref");