| Message ID | 714ff48341753de0509208e3c553b19c1c43e480.1736201898.git.Jonathan.Santos@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7768-1: Add features, improvements, and fixes | expand |
On 1/7/25 9:25 AM, Jonathan Santos wrote: > All supported parts require that the MOSI line stays high > while in idle. > > Configure SPI controller to set MOSI idle state to high. > > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support") > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > --- > drivers/iio/adc/ad7768-1.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c > index c3cf04311c40..463a28d09c2e 100644 > --- a/drivers/iio/adc/ad7768-1.c > +++ b/drivers/iio/adc/ad7768-1.c > @@ -574,6 +574,15 @@ static int ad7768_probe(struct spi_device *spi) > return -ENOMEM; > > st = iio_priv(indio_dev); > + /* > + * The ADC SDI line must be kept high when > + * data is not being clocked out of the controller. > + * Request the SPI controller to make 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"); Very few SPI controllers currently have the SPI_MOSI_IDLE_HIGH capability flag set in Linux right now (whether they actually support it or not), so this could break existing users. The datasheet says: When reading back data with CS held low, it is recommended that SDI idle high to prevent an accidental reset of the device where SCLK is free running (see the Reset section). And the reset section says: When CS is held low, it is possible to provide a reset by clocking in a 1 followed by 63 zeros on SDI, which is the SPI resume command reset function used to exit power-down mode. Since the largest xfer we do is 3 bytes before deasserting CS, I don't think we have any risk of accidentally resetting right now. If we ever do implement a data read of more than 64 bits without toggling CS, then we could just set the TX data to be all 0xFF and have the same effect without requiring the SPI controller to support SPI_MOSI_IDLE_HIGH.
On 01/07, David Lechner wrote: > On 1/7/25 9:25 AM, Jonathan Santos wrote: > > All supported parts require that the MOSI line stays high > > while in idle. > > > > Configure SPI controller to set MOSI idle state to high. > > > > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support") > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > > --- ... > > @@ -574,6 +574,15 @@ static int ad7768_probe(struct spi_device *spi) > > return -ENOMEM; > > > > st = iio_priv(indio_dev); > > + /* > > + * The ADC SDI line must be kept high when > > + * data is not being clocked out of the controller. > > + * Request the SPI controller to make 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"); > > Very few SPI controllers currently have the SPI_MOSI_IDLE_HIGH capability flag > set in Linux right now (whether they actually support it or not), so this could > break existing users. Good point. Maybe only dev_warn() if SPI_MOSI_IDLE_HIGH is not supported? > ... > > If we ever do implement a data read of more than 64 bits without toggling CS, > then we could just set the TX data to be all 0xFF and have the same effect > without requiring the SPI controller to support SPI_MOSI_IDLE_HIGH. One point of having SPI_MOSI_IDLE_HIGH is that the controller may bring MOSI low between data words of a transfer. I think all transfer words are going to be either 16 or 24 with the new patches setting bits_per_word in all transfers but that might still not be enough if eventually the controller is unable to support those word sizes. Plus you would have the complication of filling the tx_buf for all transfers. For the part that instigated the development of SPI_MOSI_IDLE_HIGH, the MOSI line also had to be high in between transfers. The diagrams at AD7768-1 datasheet page 51 suggest the same would be needed for this chip too.
On Fri, 10 Jan 2025 18:56:26 -0300 Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > On 01/07, David Lechner wrote: > > On 1/7/25 9:25 AM, Jonathan Santos wrote: > > > All supported parts require that the MOSI line stays high > > > while in idle. > > > > > > Configure SPI controller to set MOSI idle state to high. > > > > > > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support") > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > > > --- > ... > > > @@ -574,6 +574,15 @@ static int ad7768_probe(struct spi_device *spi) > > > return -ENOMEM; > > > > > > st = iio_priv(indio_dev); > > > + /* > > > + * The ADC SDI line must be kept high when > > > + * data is not being clocked out of the controller. > > > + * Request the SPI controller to make 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"); > > > > Very few SPI controllers currently have the SPI_MOSI_IDLE_HIGH capability flag > > set in Linux right now (whether they actually support it or not), so this could > > break existing users. > > Good point. Maybe only dev_warn() if SPI_MOSI_IDLE_HIGH is not supported? > > > > ... > > > > If we ever do implement a data read of more than 64 bits without toggling CS, > > then we could just set the TX data to be all 0xFF and have the same effect > > without requiring the SPI controller to support SPI_MOSI_IDLE_HIGH. > > One point of having SPI_MOSI_IDLE_HIGH is that the controller may bring MOSI low > between data words of a transfer. I think all transfer words are going to be > either 16 or 24 with the new patches setting bits_per_word in all transfers but > that might still not be enough if eventually the controller is unable to support > those word sizes. Can we make the use of SPI_MOSI_IDLE_HIGH only apply if controller doesn't support what is required to do the transfers in one go? > Plus you would have the complication of filling the tx_buf for > all transfers. Wrap that up in a regmap, or read and write functions and that should be easy enough. > > For the part that instigated the development of SPI_MOSI_IDLE_HIGH, the MOSI line > also had to be high in between transfers. The diagrams at AD7768-1 datasheet > page 51 suggest the same would be needed for this chip too. Whilst the datasheet indeed draws lines for that, i doubt it notices except on clock transitions and between transfers the clock won't do anything. If we confirm that the device does notice, then I don't mind limiting the controllers to those with that can ensure it doesn't get set wrong. Jonathan
On 01/12, Jonathan Cameron wrote: > On Fri, 10 Jan 2025 18:56:26 -0300 > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > > > On 01/07, David Lechner wrote: > > > On 1/7/25 9:25 AM, Jonathan Santos wrote: > > > > All supported parts require that the MOSI line stays high > > > > while in idle. > > > > > > > > Configure SPI controller to set MOSI idle state to high. > > > > > > > > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support") > > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > > > > --- > > ... > > > > @@ -574,6 +574,15 @@ static int ad7768_probe(struct spi_device *spi) > > > > return -ENOMEM; > > > > > > > > st = iio_priv(indio_dev); > > > > + /* > > > > + * The ADC SDI line must be kept high when > > > > + * data is not being clocked out of the controller. > > > > + * Request the SPI controller to make 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"); > > > > > > Very few SPI controllers currently have the SPI_MOSI_IDLE_HIGH capability flag > > > set in Linux right now (whether they actually support it or not), so this could > > > break existing users. > > > > Good point. Maybe only dev_warn() if SPI_MOSI_IDLE_HIGH is not supported? > > > > > > > ... > > > > > > If we ever do implement a data read of more than 64 bits without toggling CS, > > > then we could just set the TX data to be all 0xFF and have the same effect > > > without requiring the SPI controller to support SPI_MOSI_IDLE_HIGH. > > > > One point of having SPI_MOSI_IDLE_HIGH is that the controller may bring MOSI low > > between data words of a transfer. I think all transfer words are going to be > > either 16 or 24 with the new patches setting bits_per_word in all transfers but > > that might still not be enough if eventually the controller is unable to support > > those word sizes. > > Can we make the use of SPI_MOSI_IDLE_HIGH only apply if controller doesn't support > what is required to do the transfers in one go? I think so, but that would require tweaking spi controller drivers since we don't know at spi_setup() what transfers will ask for their bits_per_word. Not excited with this idea but may try something if that makes it easier to support these unusual SPI devices. > > > Plus you would have the complication of filling the tx_buf for > > all transfers. > > Wrap that up in a regmap, or read and write functions and that should be easy enough. > > > > > For the part that instigated the development of SPI_MOSI_IDLE_HIGH, the MOSI line > > also had to be high in between transfers. The diagrams at AD7768-1 datasheet > > page 51 suggest the same would be needed for this chip too. > > Whilst the datasheet indeed draws lines for that, i doubt it notices except on > clock transitions and between transfers the clock won't do anything. > If we confirm that the device does notice, then I don't mind limiting the controllers > to those with that can ensure it doesn't get set wrong. > > Jonathan > >
On Mon, 13 Jan 2025 09:19:42 -0300 Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > On 01/12, Jonathan Cameron wrote: > > On Fri, 10 Jan 2025 18:56:26 -0300 > > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > > > > > On 01/07, David Lechner wrote: > > > > On 1/7/25 9:25 AM, Jonathan Santos wrote: > > > > > All supported parts require that the MOSI line stays high > > > > > while in idle. > > > > > > > > > > Configure SPI controller to set MOSI idle state to high. > > > > > > > > > > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support") > > > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > > > > > --- > > > ... > > > > > @@ -574,6 +574,15 @@ static int ad7768_probe(struct spi_device *spi) > > > > > return -ENOMEM; > > > > > > > > > > st = iio_priv(indio_dev); > > > > > + /* > > > > > + * The ADC SDI line must be kept high when > > > > > + * data is not being clocked out of the controller. > > > > > + * Request the SPI controller to make 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"); > > > > > > > > Very few SPI controllers currently have the SPI_MOSI_IDLE_HIGH capability flag > > > > set in Linux right now (whether they actually support it or not), so this could > > > > break existing users. > > > > > > Good point. Maybe only dev_warn() if SPI_MOSI_IDLE_HIGH is not supported? > > > > > > > > > > ... > > > > > > > > If we ever do implement a data read of more than 64 bits without toggling CS, > > > > then we could just set the TX data to be all 0xFF and have the same effect > > > > without requiring the SPI controller to support SPI_MOSI_IDLE_HIGH. > > > > > > One point of having SPI_MOSI_IDLE_HIGH is that the controller may bring MOSI low > > > between data words of a transfer. I think all transfer words are going to be > > > either 16 or 24 with the new patches setting bits_per_word in all transfers but > > > that might still not be enough if eventually the controller is unable to support > > > those word sizes. > > > > Can we make the use of SPI_MOSI_IDLE_HIGH only apply if controller doesn't support > > what is required to do the transfers in one go? > > I think so, but that would require tweaking spi controller drivers since we > don't know at spi_setup() what transfers will ask for their bits_per_word. > Not excited with this idea but may try something if that makes it easier to > support these unusual SPI devices. I'm confused. Here it is a client driver question I think. That driver knows what it is asking for. It can query if that word length is supported, if not query if SPI_MOSI_IDLE_HIGH is possible and if neither fail to probe with suitable error message. Jonathan > > > > > > Plus you would have the complication of filling the tx_buf for > > > all transfers. > > > > Wrap that up in a regmap, or read and write functions and that should be easy enough. > > > > > > > > For the part that instigated the development of SPI_MOSI_IDLE_HIGH, the MOSI line > > > also had to be high in between transfers. The diagrams at AD7768-1 datasheet > > > page 51 suggest the same would be needed for this chip too. > > > > Whilst the datasheet indeed draws lines for that, i doubt it notices except on > > clock transitions and between transfers the clock won't do anything. > > If we confirm that the device does notice, then I don't mind limiting the controllers > > to those with that can ensure it doesn't get set wrong. > > > > Jonathan > > > >
On 01/18, Jonathan Cameron wrote: > On Mon, 13 Jan 2025 09:19:42 -0300 > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > > > On 01/12, Jonathan Cameron wrote: > > > On Fri, 10 Jan 2025 18:56:26 -0300 > > > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > > > > > > > On 01/07, David Lechner wrote: > > > > > On 1/7/25 9:25 AM, Jonathan Santos wrote: > > > > > > All supported parts require that the MOSI line stays high > > > > > > while in idle. > > > > > > > > > > > > Configure SPI controller to set MOSI idle state to high. > > > > > > > > > > > > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support") > > > > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > > > > > > --- > > > > ... > > > > > > @@ -574,6 +574,15 @@ static int ad7768_probe(struct spi_device *spi) > > > > > > return -ENOMEM; > > > > > > > > > > > > st = iio_priv(indio_dev); > > > > > > + /* > > > > > > + * The ADC SDI line must be kept high when > > > > > > + * data is not being clocked out of the controller. > > > > > > + * Request the SPI controller to make 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"); > > > > > > > > > > Very few SPI controllers currently have the SPI_MOSI_IDLE_HIGH capability flag > > > > > set in Linux right now (whether they actually support it or not), so this could > > > > > break existing users. > > > > > > > > Good point. Maybe only dev_warn() if SPI_MOSI_IDLE_HIGH is not supported? > > > > > > > > > > > > > ... > > > > > > > > > > If we ever do implement a data read of more than 64 bits without toggling CS, > > > > > then we could just set the TX data to be all 0xFF and have the same effect > > > > > without requiring the SPI controller to support SPI_MOSI_IDLE_HIGH. > > > > > > > > One point of having SPI_MOSI_IDLE_HIGH is that the controller may bring MOSI low > > > > between data words of a transfer. I think all transfer words are going to be > > > > either 16 or 24 with the new patches setting bits_per_word in all transfers but > > > > that might still not be enough if eventually the controller is unable to support > > > > those word sizes. > > > > > > Can we make the use of SPI_MOSI_IDLE_HIGH only apply if controller doesn't support > > > what is required to do the transfers in one go? > > > > I think so, but that would require tweaking spi controller drivers since we > > don't know at spi_setup() what transfers will ask for their bits_per_word. > > Not excited with this idea but may try something if that makes it easier to > > support these unusual SPI devices. > > I'm confused. Here it is a client driver question I think. That driver knows what > it is asking for. It can query if that word length is supported, if not query > if SPI_MOSI_IDLE_HIGH is possible and if neither fail to probe with suitable > error message. > > Jonathan Ah yes, I think that would be a better way to go. I thought your previous question was about making SPI_MOSI_IDLE_HIGH support within the SPI subsystem only apply if the controller couldn't support all of the bits_per_word an ADC driver would want for it's transfers. Sorry for the confusion. I'm still a bit skeptical about whether the device really works without SPI_MOSI_IDLE_HIGH. Though, if setting proper bits_per_word is enough then that's great because it will allow the device to work with a wider range of controllers. Marcelo > > > > > > > > > > > Plus you would have the complication of filling the tx_buf for > > > > all transfers. > > > > > > Wrap that up in a regmap, or read and write functions and that should be easy enough. > > > > > > > > > > > For the part that instigated the development of SPI_MOSI_IDLE_HIGH, the MOSI line > > > > also had to be high in between transfers. The diagrams at AD7768-1 datasheet > > > > page 51 suggest the same would be needed for this chip too. > > > > > > Whilst the datasheet indeed draws lines for that, i doubt it notices except on > > > clock transitions and between transfers the clock won't do anything. > > > If we confirm that the device does notice, then I don't mind limiting the controllers > > > to those with that can ensure it doesn't get set wrong. > > > > > > Jonathan > > > > > > >
diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c index c3cf04311c40..463a28d09c2e 100644 --- a/drivers/iio/adc/ad7768-1.c +++ b/drivers/iio/adc/ad7768-1.c @@ -574,6 +574,15 @@ static int ad7768_probe(struct spi_device *spi) return -ENOMEM; st = iio_priv(indio_dev); + /* + * The ADC SDI line must be kept high when + * data is not being clocked out of the controller. + * Request the SPI controller to make 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");
All supported parts require that the MOSI line stays high while in idle. Configure SPI controller to set MOSI idle state to high. Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support") Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> --- drivers/iio/adc/ad7768-1.c | 9 +++++++++ 1 file changed, 9 insertions(+)