| Message ID | 20191003173401.16343-2-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce max12xx ADC support | expand |
On Thu, 3 Oct 2019 19:33:55 +0200 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Until now, only write operations were supported. Force two bytes read > operation when reading from this register (might be wrong when reading > the temperature, but will work with any other value). That's worrying as comments go. Just return an error on the temperature register if it's going to do the wrong thing. Thanks, Jonathan > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/iio/adc/max1027.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c > index 214883458582..6cdfe9ef73fc 100644 > --- a/drivers/iio/adc/max1027.c > +++ b/drivers/iio/adc/max1027.c > @@ -309,8 +309,11 @@ static int max1027_debugfs_reg_access(struct iio_dev *indio_dev, > struct max1027_state *st = iio_priv(indio_dev); > u8 *val = (u8 *)st->buffer; > > - if (readval != NULL) > - return -EINVAL; > + if (readval) { > + int ret = spi_read(st->spi, val, 2); > + *readval = be16_to_cpu(st->buffer[0]); > + return ret; > + } > > *val = (u8)writeval; > return spi_write(st->spi, val, 1);
Hi Jonathan, Jonathan Cameron <jic23@kernel.org> wrote on Sun, 6 Oct 2019 11:04:24 +0100: > On Thu, 3 Oct 2019 19:33:55 +0200 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > Until now, only write operations were supported. Force two bytes read > > operation when reading from this register (might be wrong when reading > > the temperature, but will work with any other value). > > That's worrying as comments go. Just return an error on the temperature > register if it's going to do the wrong thing. Actually the debugfs_reg_access hook is supposedly stateless. When reading registers I don't know what I am reading because the "source" is selected during the write operation, so I have no reliable way to know what I am reading. I set the read length to 2 bytes because most of the "atomic"reads are two bytes and it allows us to test various commands directly from userspace and read meaningful values. This is a limitation as: * Voltage 'atomic' reads are 2 bytes * Temperature 'atomic' reads are 2 bytes but never come alone (usually one voltage input of 2B will follow). * Any other 'condensed' input will be more than 2 bytes, ie. several voltage values in one go. In any case, doing a software reset of the chip will turn it back into a working state no matter what was requested/read. For me, 2-byte reads is a "good enough" solution that will work with almost all the simplest ('atomic') SPI operations, but if you think limiting to 2-bytes access is a problem (right now there is only write access, which is kind of useless on its own) then let's drop the patch. But I wanted to contribute it because it really helped me during the development. Thanks, Miquèl
On Mon, 7 Oct 2019 12:00:01 +0200 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Jonathan, > > Jonathan Cameron <jic23@kernel.org> wrote on Sun, 6 Oct 2019 11:04:24 > +0100: > > > On Thu, 3 Oct 2019 19:33:55 +0200 > > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > Until now, only write operations were supported. Force two bytes read > > > operation when reading from this register (might be wrong when reading > > > the temperature, but will work with any other value). > > > > That's worrying as comments go. Just return an error on the temperature > > register if it's going to do the wrong thing. > > Actually the debugfs_reg_access hook is supposedly stateless. When > reading registers I don't know what I am reading because the "source" is > selected during the write operation, so I have no reliable way to know > what I am reading. > > I set the read length to 2 bytes because most of the "atomic"reads are > two bytes and it allows us to test various commands directly from > userspace and read meaningful values. This is a limitation as: > * Voltage 'atomic' reads are 2 bytes > * Temperature 'atomic' reads are 2 bytes but never come alone (usually > one voltage input of 2B will follow). > * Any other 'condensed' input will be more than 2 bytes, ie. several > voltage values in one go. > > In any case, doing a software reset of the chip will turn it back > into a working state no matter what was requested/read. > > For me, 2-byte reads is a "good enough" solution that will work with > almost all the simplest ('atomic') SPI operations, but if you think > limiting to 2-bytes access is a problem (right now there is only write > access, which is kind of useless on its own) then let's drop the patch. > But I wanted to contribute it because it really helped me during the > development. This is fine as is. Comment was worrying so could perhaps have given more detail. Still it's a debug interface, people get to look at the datasheet if they are using this :) Jonathan > > > Thanks, > Miquèl
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c index 214883458582..6cdfe9ef73fc 100644 --- a/drivers/iio/adc/max1027.c +++ b/drivers/iio/adc/max1027.c @@ -309,8 +309,11 @@ static int max1027_debugfs_reg_access(struct iio_dev *indio_dev, struct max1027_state *st = iio_priv(indio_dev); u8 *val = (u8 *)st->buffer; - if (readval != NULL) - return -EINVAL; + if (readval) { + int ret = spi_read(st->spi, val, 2); + *readval = be16_to_cpu(st->buffer[0]); + return ret; + } *val = (u8)writeval; return spi_write(st->spi, val, 1);
Until now, only write operations were supported. Force two bytes read operation when reading from this register (might be wrong when reading the temperature, but will work with any other value). Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/iio/adc/max1027.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)