[PATCH 0/2] ad5593r fix read protocol

[PATCH 0/2] ad5593r fix read protocol

[Nuno Sá]

This patchset fixes the read protocol since it needs a STOP condition
between address write and data read.

The second change is trivial and only adds an i2c functionality check.

Michael Hennerich (1):
  iio: dac: ad5593r: Fix i2c read protocol requirements

Nuno Sá (1):
  iio: dac: ad5593r: add check for i2c functionality

 drivers/iio/dac/ad5593r.c | 48 +++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 20 deletions(-)

-- 
2.37.3

[PATCH 1/2] iio: dac: ad5593r: Fix i2c read protocol requirements

[Nuno Sá]

From: Michael Hennerich <michael.hennerich@analog.com>

For reliable operation across the full range of supported
interface rates, the AD5593R needs a STOP condition between
address write, and data read (like show in the datasheet Figure 40)
so in turn i2c_smbus_read_word_swapped cannot be used.

While at it, a simple helper was added to make the code simpler.

Fixes: 56ca9db862bf ("iio: dac: Add support for the AD5592R/AD5593R ADCs/DACs")
Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/dac/ad5593r.c | 44 +++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/dac/ad5593r.c b/drivers/iio/dac/ad5593r.c
index 34e1319a9712..43ee4efb250c 100644
--- a/drivers/iio/dac/ad5593r.c
+++ b/drivers/iio/dac/ad5593r.c
@@ -20,6 +20,24 @@
 #define AD5593R_MODE_GPIO_READBACK	(6 << 4)
 #define AD5593R_MODE_REG_READBACK	(7 << 4)
 
+static int ad5593r_read_word(struct i2c_client *i2c, u8 reg, u16 *value)
+{
+	int ret;
+	u8 buf[2];
+
+	ret = i2c_smbus_write_byte(i2c, reg);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_master_recv(i2c, buf, sizeof(buf));
+	if (ret < 0)
+		return ret;
+
+	*value = get_unaligned_be16(buf);
+
+	return 0;
+}
+
 static int ad5593r_write_dac(struct ad5592r_state *st, unsigned chan, u16 value)
 {
 	struct i2c_client *i2c = to_i2c_client(st->dev);
@@ -38,13 +56,7 @@ static int ad5593r_read_adc(struct ad5592r_state *st, unsigned chan, u16 *value)
 	if (val < 0)
 		return (int) val;
 
-	val = i2c_smbus_read_word_swapped(i2c, AD5593R_MODE_ADC_READBACK);
-	if (val < 0)
-		return (int) val;
-
-	*value = (u16) val;
-
-	return 0;
+	return ad5593r_read_word(i2c, AD5593R_MODE_ADC_READBACK, value);
 }
 
 static int ad5593r_reg_write(struct ad5592r_state *st, u8 reg, u16 value)
@@ -58,29 +70,21 @@ static int ad5593r_reg_write(struct ad5592r_state *st, u8 reg, u16 value)
 static int ad5593r_reg_read(struct ad5592r_state *st, u8 reg, u16 *value)
 {
 	struct i2c_client *i2c = to_i2c_client(st->dev);
-	s32 val;
-
-	val = i2c_smbus_read_word_swapped(i2c, AD5593R_MODE_REG_READBACK | reg);
-	if (val < 0)
-		return (int) val;
-
-	*value = (u16) val;
 
-	return 0;
+	return ad5593r_read_word(i2c, AD5593R_MODE_REG_READBACK | reg, value);
 }
 
 static int ad5593r_gpio_read(struct ad5592r_state *st, u8 *value)
 {
 	struct i2c_client *i2c = to_i2c_client(st->dev);
-	s32 val;
+	u16 val;
+	int ret;
 
-	val = i2c_smbus_read_word_swapped(i2c, AD5593R_MODE_GPIO_READBACK);
-	if (val < 0)
-		return (int) val;
+	ret = ad5593r_read_word(i2c, AD5593R_MODE_GPIO_READBACK, &val);
 
 	*value = (u8) val;
 
-	return 0;
+	return ret;
 }
 
 static const struct ad5592r_rw_ops ad5593r_rw_ops = {
-- 
2.37.3

[PATCH 2/2] iio: dac: ad5593r: add check for i2c functionality

[Nuno Sá]

Make sure that the needed i2c functionality is supported during probe.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/iio/dac/ad5593r.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/iio/dac/ad5593r.c b/drivers/iio/dac/ad5593r.c
index 43ee4efb250c..f09433e298db 100644
--- a/drivers/iio/dac/ad5593r.c
+++ b/drivers/iio/dac/ad5593r.c
@@ -98,6 +98,10 @@ static const struct ad5592r_rw_ops ad5593r_rw_ops = {
 static int ad5593r_i2c_probe(struct i2c_client *i2c,
 		const struct i2c_device_id *id)
 {
+	if (!i2c_check_functionality(i2c->adapter,
+				     I2C_FUNC_SMBUS_BYTE | I2C_FUNC_I2C))
+		return -EOPNOTSUPP;
+
 	return ad5592r_probe(&i2c->dev, id->name, &ad5593r_rw_ops);
 }
 
-- 
2.37.3

Re: [PATCH 0/2] ad5593r fix read protocol

[Jonathan Cameron]

On Tue, 13 Sep 2022 09:34:11 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> This patchset fixes the read protocol since it needs a STOP condition
> between address write and data read.
> 
> The second change is trivial and only adds an i2c functionality check.

Given we are late in the cycle, I've queued this up for the next merge
window, with a stable tag for the first paatch so it'll get backported
after the merge window.

Thanks,

Jonathan


> 
> Michael Hennerich (1):
>   iio: dac: ad5593r: Fix i2c read protocol requirements
> 
> Nuno Sá (1):
>   iio: dac: ad5593r: add check for i2c functionality
> 
>  drivers/iio/dac/ad5593r.c | 48 +++++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 20 deletions(-)
> 

Re: [PATCH 0/2] ad5593r fix read protocol

[Nuno Sá]

On Thu, 2022-09-15 at 15:38 +0100, Jonathan Cameron wrote:
> On Tue, 13 Sep 2022 09:34:11 +0200
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > This patchset fixes the read protocol since it needs a STOP
> > condition
> > between address write and data read.
> > 
> > The second change is trivial and only adds an i2c functionality
> > check.
> 
> Given we are late in the cycle, I've queued this up for the next
> merge
> window, with a stable tag for the first paatch so it'll get
> backported
> after the merge window.
> 
> 

Alright. BTW, not sure If I already asked this but do you have any
preference with regards to CCing stable? Should I have done it when
submitting or do you prefer to handle it yourself?

- Nuno Sá

Re: [PATCH 0/2] ad5593r fix read protocol

[Jonathan Cameron]

On Fri, 16 Sep 2022 08:05:29 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Thu, 2022-09-15 at 15:38 +0100, Jonathan Cameron wrote:
> > On Tue, 13 Sep 2022 09:34:11 +0200
> > Nuno Sá <nuno.sa@analog.com> wrote:
> >   
> > > This patchset fixes the read protocol since it needs a STOP
> > > condition
> > > between address write and data read.
> > > 
> > > The second change is trivial and only adds an i2c functionality
> > > check.  
> > 
> > Given we are late in the cycle, I've queued this up for the next
> > merge
> > window, with a stable tag for the first paatch so it'll get
> > backported
> > after the merge window.
> > 
> >   
> 
> Alright. BTW, not sure If I already asked this but do you have any
> preference with regards to CCing stable? Should I have done it when
> submitting or do you prefer to handle it yourself?

Generally I prefer submitters to not tag for stable and let me make that
decision.  Often I'll decide to not tag because I'm a little worried
about a fix and want it to be in mainline a little while before we
backport.  I don't mind people sending explicit backport requests
though once it's soaked a bit.

Mind you, these days the scripts that check for possible fixes
often pick these up before I've gotten to sending a backport
request. Sometimes I send a note when that happens to ask for
it to soak longer, but mostly the delay is enough that I'm happy
the patch got enough soaking before that happens.

Occasionally I just forget to tag with stable. If that happens
then I'm fine with a request to pick it up being sent out once
it is upstream!

Jonathan

> 
> - Nuno Sá
> 
>