Linux IIO development
 help / color / mirror / Atom feed
* [PATCH] iio/adc/pac1934: fix channel disable configuration
@ 2025-08-11 13:09 Aleksandar Gerasimovski
  2025-08-12  8:36 ` Marius.Cristea
  2025-08-16 13:43 ` Jonathan Cameron
  0 siblings, 2 replies; 7+ messages in thread
From: Aleksandar Gerasimovski @ 2025-08-11 13:09 UTC (permalink / raw)
  To: linux-iio, marius.cristea, rene.straub; +Cc: Aleksandar Gerasimovski

There are two problems with the chip configuration in this driver:
- First, is that writing 12 bytes (ARRAY_SIZE(regs)) would anyhow
  lead to a config overflow due to HW auto increment implementation
  in the chip.
- Second, the i2c_smbus_write_block_data write ends up in writing
  unexpected value to the channel_dis register, this is because
  the smbus size that is 0x03 in this case gets written to the
  register. The PAC1931/2/3/4 data sheet does not really specify
  that block write is indeed supported.

This problem is probably not visible on PAC1934 version where all
channels are used as the chip is properly configured by luck,
but in our case whenusing PAC1931 this leads to nonfunctional device.

Signed-off-by: Rene Straub <rene.straub@belden.com>
Signed-off-by: Aleksandar Gerasimovski <aleksandar.gerasimovski@belden.com>
---
 drivers/iio/adc/pac1934.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/pac1934.c b/drivers/iio/adc/pac1934.c
index 09fe88eb3fb0..2e442e46f679 100644
--- a/drivers/iio/adc/pac1934.c
+++ b/drivers/iio/adc/pac1934.c
@@ -88,6 +88,7 @@
 #define PAC1934_VPOWER_3_ADDR			0x19
 #define PAC1934_VPOWER_4_ADDR			0x1A
 #define PAC1934_REFRESH_V_REG_ADDR		0x1F
+#define PAC1934_SLOW_REG_ADDR			0x20
 #define PAC1934_CTRL_STAT_REGS_ADDR		0x1C
 #define PAC1934_PID_REG_ADDR			0xFD
 #define PAC1934_MID_REG_ADDR			0xFE
@@ -1265,8 +1266,23 @@ static int pac1934_chip_configure(struct pac1934_chip_info *info)
 	/* no SLOW triggered REFRESH, clear POR */
 	regs[PAC1934_SLOW_REG_OFF] = 0;
 
-	ret =  i2c_smbus_write_block_data(client, PAC1934_CTRL_STAT_REGS_ADDR,
-					  ARRAY_SIZE(regs), (u8 *)regs);
+	/*
+	 * Write the three bytes sequentially, as the device does not support
+	 * block write.
+	 */
+	ret = i2c_smbus_write_byte_data(client, PAC1934_CTRL_STAT_REGS_ADDR,
+					regs[PAC1934_CHANNEL_DIS_REG_OFF]);
+	if (ret)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(client,
+					PAC1934_CTRL_STAT_REGS_ADDR + PAC1934_NEG_PWR_REG_OFF,
+					regs[PAC1934_NEG_PWR_REG_OFF]);
+	if (ret)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(client, PAC1934_SLOW_REG_ADDR,
+					regs[PAC1934_SLOW_REG_OFF]);
 	if (ret)
 		return ret;
 
-- 
2.34.1


**********************************************************************
DISCLAIMER:
Privileged and/or Confidential information may be contained in this message. If you are not the addressee of this message, you may not copy, use or deliver this message to anyone. In such event, you should destroy the message and kindly notify the sender by reply e-mail. It is understood that opinions or conclusions that do not relate to the official business of the company are neither given nor endorsed by the company. Thank You.

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] iio/adc/pac1934: fix channel disable configuration
  2025-08-11 13:09 [PATCH] iio/adc/pac1934: fix channel disable configuration Aleksandar Gerasimovski
@ 2025-08-12  8:36 ` Marius.Cristea
  2025-08-16 13:43 ` Jonathan Cameron
  1 sibling, 0 replies; 7+ messages in thread
From: Marius.Cristea @ 2025-08-12  8:36 UTC (permalink / raw)
  To: aleksandar.gerasimovski, rene.straub, linux-iio

Hi,

   Thank you for finding and fixing the bug.



Reviewed-by: Marius Cristea <marius.cristea@microchip.com>





On Mon, 2025-08-11 at 13:09 +0000, Aleksandar Gerasimovski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> There are two problems with the chip configuration in this driver:
> - First, is that writing 12 bytes (ARRAY_SIZE(regs)) would anyhow
>   lead to a config overflow due to HW auto increment implementation
>   in the chip.
> - Second, the i2c_smbus_write_block_data write ends up in writing
>   unexpected value to the channel_dis register, this is because
>   the smbus size that is 0x03 in this case gets written to the
>   register. The PAC1931/2/3/4 data sheet does not really specify
>   that block write is indeed supported.
> 
> This problem is probably not visible on PAC1934 version where all
> channels are used as the chip is properly configured by luck,
> but in our case whenusing PAC1931 this leads to nonfunctional device.
> 
> Signed-off-by: Rene Straub <rene.straub@belden.com>
> Signed-off-by: Aleksandar Gerasimovski
> <aleksandar.gerasimovski@belden.com>
> ---
>  drivers/iio/adc/pac1934.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/pac1934.c b/drivers/iio/adc/pac1934.c
> index 09fe88eb3fb0..2e442e46f679 100644
> --- a/drivers/iio/adc/pac1934.c
> +++ b/drivers/iio/adc/pac1934.c
> @@ -88,6 +88,7 @@
>  #define PAC1934_VPOWER_3_ADDR                  0x19
>  #define PAC1934_VPOWER_4_ADDR                  0x1A
>  #define PAC1934_REFRESH_V_REG_ADDR             0x1F
> +#define PAC1934_SLOW_REG_ADDR                  0x20
>  #define PAC1934_CTRL_STAT_REGS_ADDR            0x1C
>  #define PAC1934_PID_REG_ADDR                   0xFD
>  #define PAC1934_MID_REG_ADDR                   0xFE
> @@ -1265,8 +1266,23 @@ static int pac1934_chip_configure(struct
> pac1934_chip_info *info)
>         /* no SLOW triggered REFRESH, clear POR */
>         regs[PAC1934_SLOW_REG_OFF] = 0;
> 
> -       ret =  i2c_smbus_write_block_data(client,
> PAC1934_CTRL_STAT_REGS_ADDR,
> -                                         ARRAY_SIZE(regs), (u8
> *)regs);
> +       /*
> +        * Write the three bytes sequentially, as the device does not
> support
> +        * block write.
> +        */
> +       ret = i2c_smbus_write_byte_data(client,
> PAC1934_CTRL_STAT_REGS_ADDR,
> +                                      
> regs[PAC1934_CHANNEL_DIS_REG_OFF]);
> +       if (ret)
> +               return ret;
> +
> +       ret = i2c_smbus_write_byte_data(client,
> +                                       PAC1934_CTRL_STAT_REGS_ADDR +
> PAC1934_NEG_PWR_REG_OFF,
> +                                      
> regs[PAC1934_NEG_PWR_REG_OFF]);
> +       if (ret)
> +               return ret;
> +
> +       ret = i2c_smbus_write_byte_data(client,
> PAC1934_SLOW_REG_ADDR,
> +                                       regs[PAC1934_SLOW_REG_OFF]);
>         if (ret)
>                 return ret;
> 
> --
> 2.34.1
> 
> 
> *********************************************************************
> *
> DISCLAIMER:
> Privileged and/or Confidential information may be contained in this
> message. If you are not the addressee of this message, you may not
> copy, use or deliver this message to anyone. In such event, you
> should destroy the message and kindly notify the sender by reply e-
> mail. It is understood that opinions or conclusions that do not
> relate to the official business of the company are neither given nor
> endorsed by the company. Thank You.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] iio/adc/pac1934: fix channel disable configuration
  2025-08-11 13:09 [PATCH] iio/adc/pac1934: fix channel disable configuration Aleksandar Gerasimovski
  2025-08-12  8:36 ` Marius.Cristea
@ 2025-08-16 13:43 ` Jonathan Cameron
  2025-08-18 11:08   ` Aleksandar Gerasimovski
  1 sibling, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2025-08-16 13:43 UTC (permalink / raw)
  To: Aleksandar Gerasimovski; +Cc: linux-iio, marius.cristea, rene.straub

On Mon, 11 Aug 2025 13:09:04 +0000
Aleksandar Gerasimovski <aleksandar.gerasimovski@belden.com> wrote:

> There are two problems with the chip configuration in this driver:
> - First, is that writing 12 bytes (ARRAY_SIZE(regs)) would anyhow
>   lead to a config overflow due to HW auto increment implementation
>   in the chip.
> - Second, the i2c_smbus_write_block_data write ends up in writing
>   unexpected value to the channel_dis register, this is because
>   the smbus size that is 0x03 in this case gets written to the
>   register. The PAC1931/2/3/4 data sheet does not really specify
>   that block write is indeed supported.
> 
> This problem is probably not visible on PAC1934 version where all
> channels are used as the chip is properly configured by luck,
> but in our case whenusing PAC1931 this leads to nonfunctional device.
> 
> Signed-off-by: Rene Straub <rene.straub@belden.com>
> Signed-off-by: Aleksandar Gerasimovski <aleksandar.gerasimovski@belden.com>

Hi, Thanks for the patch.

Needs a fixes tag so we know how far to back port it.
You can just reply with one to this thread rather than sending a new
version.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/pac1934.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/pac1934.c b/drivers/iio/adc/pac1934.c
> index 09fe88eb3fb0..2e442e46f679 100644
> --- a/drivers/iio/adc/pac1934.c
> +++ b/drivers/iio/adc/pac1934.c
> @@ -88,6 +88,7 @@
>  #define PAC1934_VPOWER_3_ADDR			0x19
>  #define PAC1934_VPOWER_4_ADDR			0x1A
>  #define PAC1934_REFRESH_V_REG_ADDR		0x1F
> +#define PAC1934_SLOW_REG_ADDR			0x20
>  #define PAC1934_CTRL_STAT_REGS_ADDR		0x1C
>  #define PAC1934_PID_REG_ADDR			0xFD
>  #define PAC1934_MID_REG_ADDR			0xFE
> @@ -1265,8 +1266,23 @@ static int pac1934_chip_configure(struct pac1934_chip_info *info)
>  	/* no SLOW triggered REFRESH, clear POR */
>  	regs[PAC1934_SLOW_REG_OFF] = 0;
>  
> -	ret =  i2c_smbus_write_block_data(client, PAC1934_CTRL_STAT_REGS_ADDR,
> -					  ARRAY_SIZE(regs), (u8 *)regs);
> +	/*
> +	 * Write the three bytes sequentially, as the device does not support
> +	 * block write.
> +	 */
> +	ret = i2c_smbus_write_byte_data(client, PAC1934_CTRL_STAT_REGS_ADDR,
> +					regs[PAC1934_CHANNEL_DIS_REG_OFF]);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(client,
> +					PAC1934_CTRL_STAT_REGS_ADDR + PAC1934_NEG_PWR_REG_OFF,
> +					regs[PAC1934_NEG_PWR_REG_OFF]);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, PAC1934_SLOW_REG_ADDR,
> +					regs[PAC1934_SLOW_REG_OFF]);
>  	if (ret)
>  		return ret;
>  


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] iio/adc/pac1934: fix channel disable configuration
  2025-08-16 13:43 ` Jonathan Cameron
@ 2025-08-18 11:08   ` Aleksandar Gerasimovski
  2025-08-18 17:56     ` Jonathan Cameron
  0 siblings, 1 reply; 7+ messages in thread
From: Aleksandar Gerasimovski @ 2025-08-18 11:08 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio@vger.kernel.org, marius.cristea@microchip.com,
	Rene Straub

Hi Jonathan, and sorry for the previous non-plain text message:

Fixes: 0fb528c8255b (iio: adc: adding support for PAC193x)

Regards,
Aleksandar

From: Jonathan Cameron <jic23@kernel.org> 
Sent: Saturday, August 16, 2025 3:44 PM
To: Aleksandar Gerasimovski <Aleksandar.Gerasimovski@belden.com>
Cc: linux-iio@vger.kernel.org; marius.cristea@microchip.com; Rene Straub <Rene.Straub@belden.com>
Subject: Re: [PATCH] iio/adc/pac1934: fix channel disable configuration

On Mon, 11 Aug 2025 13: 09: 04 +0000 Aleksandar Gerasimovski <aleksandar. gerasimovski@ belden. com> wrote: > There are two problems with the chip configuration in this driver: > - First, is that writing 12 bytes (ARRAY_SIZE(regs)) would

On Mon, 11 Aug 2025 13:09:04 +0000
Aleksandar Gerasimovski <mailto:aleksandar.gerasimovski@belden.com> wrote:

> There are two problems with the chip configuration in this driver:
> - First, is that writing 12 bytes (ARRAY_SIZE(regs)) would anyhow
>   lead to a config overflow due to HW auto increment implementation
>   in the chip.
> - Second, the i2c_smbus_write_block_data write ends up in writing
>   unexpected value to the channel_dis register, this is because
>   the smbus size that is 0x03 in this case gets written to the
>   register. The PAC1931/2/3/4 data sheet does not really specify
>   that block write is indeed supported.
> 
> This problem is probably not visible on PAC1934 version where all
> channels are used as the chip is properly configured by luck,
> but in our case whenusing PAC1931 this leads to nonfunctional device.
> 
> Signed-off-by: Rene Straub <mailto:rene.straub@belden.com>
> Signed-off-by: Aleksandar Gerasimovski <mailto:aleksandar.gerasimovski@belden.com>

Hi, Thanks for the patch.

Needs a fixes tag so we know how far to back port it.
You can just reply with one to this thread rather than sending a new
version.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/pac1934.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/pac1934.c b/drivers/iio/adc/pac1934.c
> index 09fe88eb3fb0..2e442e46f679 100644
> --- a/drivers/iio/adc/pac1934.c
> +++ b/drivers/iio/adc/pac1934.c
> @@ -88,6 +88,7 @@
>  #define PAC1934_VPOWER_3_ADDR			0x19
>  #define PAC1934_VPOWER_4_ADDR			0x1A
>  #define PAC1934_REFRESH_V_REG_ADDR		0x1F
> +#define PAC1934_SLOW_REG_ADDR			0x20
>  #define PAC1934_CTRL_STAT_REGS_ADDR		0x1C
>  #define PAC1934_PID_REG_ADDR			0xFD
>  #define PAC1934_MID_REG_ADDR			0xFE
> @@ -1265,8 +1266,23 @@ static int pac1934_chip_configure(struct pac1934_chip_info *info)
>  	/* no SLOW triggered REFRESH, clear POR */
>  	regs[PAC1934_SLOW_REG_OFF] = 0;
>  
> -	ret =  i2c_smbus_write_block_data(client, PAC1934_CTRL_STAT_REGS_ADDR,
> -					  ARRAY_SIZE(regs), (u8 *)regs);
> +	/*
> +	 * Write the three bytes sequentially, as the device does not support
> +	 * block write.
> +	 */
> +	ret = i2c_smbus_write_byte_data(client, PAC1934_CTRL_STAT_REGS_ADDR,
> +					regs[PAC1934_CHANNEL_DIS_REG_OFF]);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(client,
> +					PAC1934_CTRL_STAT_REGS_ADDR + PAC1934_NEG_PWR_REG_OFF,
> +					regs[PAC1934_NEG_PWR_REG_OFF]);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, PAC1934_SLOW_REG_ADDR,
> +					regs[PAC1934_SLOW_REG_OFF]);
>  	if (ret)
>  		return ret;
>  

**********************************************************************
DISCLAIMER:
Privileged and/or Confidential information may be contained in this message. If you are not the addressee of this message, you may not copy, use or deliver this message to anyone. In such event, you should destroy the message and kindly notify the sender by reply e-mail. It is understood that opinions or conclusions that do not relate to the official business of the company are neither given nor endorsed by the company. Thank You.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] iio/adc/pac1934: fix channel disable configuration
  2025-08-18 11:08   ` Aleksandar Gerasimovski
@ 2025-08-18 17:56     ` Jonathan Cameron
  2025-08-19  8:57       ` Aleksandar Gerasimovski
  0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2025-08-18 17:56 UTC (permalink / raw)
  To: Aleksandar Gerasimovski
  Cc: linux-iio@vger.kernel.org, marius.cristea@microchip.com,
	Rene Straub

On Mon, 18 Aug 2025 11:08:33 +0000
Aleksandar Gerasimovski <Aleksandar.Gerasimovski@belden.com> wrote:

> Hi Jonathan, and sorry for the previous non-plain text message:
> 
> Fixes: 0fb528c8255b (iio: adc: adding support for PAC193x)
Thanks.  I noticed one more thing when I was about to queue this up..

> 
> Regards,
> Aleksandar
> 
> From: Jonathan Cameron <jic23@kernel.org> 
> Sent: Saturday, August 16, 2025 3:44 PM
> To: Aleksandar Gerasimovski <Aleksandar.Gerasimovski@belden.com>
> Cc: linux-iio@vger.kernel.org; marius.cristea@microchip.com; Rene Straub <Rene.Straub@belden.com>
> Subject: Re: [PATCH] iio/adc/pac1934: fix channel disable configuration
> 
> On Mon, 11 Aug 2025 13: 09: 04 +0000 Aleksandar Gerasimovski <aleksandar. gerasimovski@ belden. com> wrote: > There are two problems with the chip configuration in this driver: > - First, is that writing 12 bytes (ARRAY_SIZE(regs)) would
> 
> On Mon, 11 Aug 2025 13:09:04 +0000
> Aleksandar Gerasimovski <mailto:aleksandar.gerasimovski@belden.com> wrote:
> 
> > There are two problems with the chip configuration in this driver:
> > - First, is that writing 12 bytes (ARRAY_SIZE(regs)) would anyhow
> >   lead to a config overflow due to HW auto increment implementation
> >   in the chip.
> > - Second, the i2c_smbus_write_block_data write ends up in writing
> >   unexpected value to the channel_dis register, this is because
> >   the smbus size that is 0x03 in this case gets written to the
> >   register. The PAC1931/2/3/4 data sheet does not really specify
> >   that block write is indeed supported.
> > 
> > This problem is probably not visible on PAC1934 version where all
> > channels are used as the chip is properly configured by luck,
> > but in our case whenusing PAC1931 this leads to nonfunctional device.
> > 
> > Signed-off-by: Rene Straub <mailto:rene.straub@belden.com>
This SoB chain doesn't comply with the normal requirements.
Given the author is Aleksandar (no From in the commit, so it's the email sender)
my guess is either

a) Rene wrote the patch and the author of the commit accidentally got changed)
b) Rene and Aleksandar both wrote the patch in which case it is missing
Co-developed-by: Rene Straub <mailto:rene.straub@belden.com>
here

I can fix it up if you confirm if it was a or b or something else

Thanks,

Jonathan

> > Signed-off-by: Aleksandar Gerasimovski <mailto:aleksandar.gerasimovski@belden.com>  
> 
> Hi, Thanks for the patch.
> 
> Needs a fixes tag so we know how far to back port it.
> You can just reply with one to this thread rather than sending a new
> version.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/adc/pac1934.c | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/pac1934.c b/drivers/iio/adc/pac1934.c
> > index 09fe88eb3fb0..2e442e46f679 100644
> > --- a/drivers/iio/adc/pac1934.c
> > +++ b/drivers/iio/adc/pac1934.c
> > @@ -88,6 +88,7 @@
> >  #define PAC1934_VPOWER_3_ADDR			0x19
> >  #define PAC1934_VPOWER_4_ADDR			0x1A
> >  #define PAC1934_REFRESH_V_REG_ADDR		0x1F
> > +#define PAC1934_SLOW_REG_ADDR			0x20
> >  #define PAC1934_CTRL_STAT_REGS_ADDR		0x1C
> >  #define PAC1934_PID_REG_ADDR			0xFD
> >  #define PAC1934_MID_REG_ADDR			0xFE
> > @@ -1265,8 +1266,23 @@ static int pac1934_chip_configure(struct pac1934_chip_info *info)
> >  	/* no SLOW triggered REFRESH, clear POR */
> >  	regs[PAC1934_SLOW_REG_OFF] = 0;
> >  
> > -	ret =  i2c_smbus_write_block_data(client, PAC1934_CTRL_STAT_REGS_ADDR,
> > -					  ARRAY_SIZE(regs), (u8 *)regs);
> > +	/*
> > +	 * Write the three bytes sequentially, as the device does not support
> > +	 * block write.
> > +	 */
> > +	ret = i2c_smbus_write_byte_data(client, PAC1934_CTRL_STAT_REGS_ADDR,
> > +					regs[PAC1934_CHANNEL_DIS_REG_OFF]);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = i2c_smbus_write_byte_data(client,
> > +					PAC1934_CTRL_STAT_REGS_ADDR + PAC1934_NEG_PWR_REG_OFF,
> > +					regs[PAC1934_NEG_PWR_REG_OFF]);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = i2c_smbus_write_byte_data(client, PAC1934_SLOW_REG_ADDR,
> > +					regs[PAC1934_SLOW_REG_OFF]);
> >  	if (ret)
> >  		return ret;
> >    
> 
> **********************************************************************
> DISCLAIMER:
> Privileged and/or Confidential information may be contained in this message. If you are not the addressee of this message, you may not copy, use or deliver this message to anyone. In such event, you should destroy the message and kindly notify the sender by reply e-mail. It is understood that opinions or conclusions that do not relate to the official business of the company are neither given nor endorsed by the company. Thank You.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] iio/adc/pac1934: fix channel disable configuration
  2025-08-18 17:56     ` Jonathan Cameron
@ 2025-08-19  8:57       ` Aleksandar Gerasimovski
  2025-08-25  9:45         ` Jonathan Cameron
  0 siblings, 1 reply; 7+ messages in thread
From: Aleksandar Gerasimovski @ 2025-08-19  8:57 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio@vger.kernel.org, marius.cristea@microchip.com,
	Rene Straub

Hi Jonathan,

Both Rene and I was investigating the problem in first place, I come with the patch afterwards.
So, the sign would be then:

Suggested-by: Rene Straub <mailto:rene.straub@belden.com>
Signed-off-by: Aleksandar Gerasimovski <aleksandar.gerasimovski@belden.com>

Thanks for following up!

From: Jonathan Cameron <jic23@kernel.org> 
Sent: Monday, August 18, 2025 7:56 PM
To: Aleksandar Gerasimovski <Aleksandar.Gerasimovski@belden.com>
Cc: linux-iio@vger.kernel.org; marius.cristea@microchip.com; Rene Straub <Rene.Straub@belden.com>
Subject: Re: [PATCH] iio/adc/pac1934: fix channel disable configuration

On Mon, 18 Aug 2025 11: 08: 33 +0000 Aleksandar Gerasimovski <Aleksandar. Gerasimovski@ belden. com> wrote: > Hi Jonathan, and sorry for the previous non-plain text message: > > Fixes: 0fb528c8255b (iio: adc: adding support for PAC193x)

On Mon, 18 Aug 2025 11:08:33 +0000
Aleksandar Gerasimovski <mailto:Aleksandar.Gerasimovski@belden.com> wrote:

> Hi Jonathan, and sorry for the previous non-plain text message:
> 
> Fixes: 0fb528c8255b (iio: adc: adding support for PAC193x)
Thanks.  I noticed one more thing when I was about to queue this up..

> 
> Regards,
> Aleksandar
> 
> From: Jonathan Cameron <mailto:jic23@kernel.org> 
> Sent: Saturday, August 16, 2025 3:44 PM
> To: Aleksandar Gerasimovski <mailto:Aleksandar.Gerasimovski@belden.com>
> Cc: mailto:linux-iio@vger.kernel.org; mailto:marius.cristea@microchip.com; Rene Straub <mailto:Rene.Straub@belden.com>
> Subject: Re: [PATCH] iio/adc/pac1934: fix channel disable configuration
> 
> On Mon, 11 Aug 2025 13: 09: 04 +0000 Aleksandar Gerasimovski <aleksandar. gerasimovski@ belden. com> wrote: > There are two problems with the chip configuration in this driver: > - First, is that writing 12 bytes (ARRAY_SIZE(regs)) would
> 
> On Mon, 11 Aug 2025 13:09:04 +0000
> Aleksandar Gerasimovski <mailto:aleksandar.gerasimovski@belden.com> wrote:
> 
> > There are two problems with the chip configuration in this driver:
> > - First, is that writing 12 bytes (ARRAY_SIZE(regs)) would anyhow
> >   lead to a config overflow due to HW auto increment implementation
> >   in the chip.
> > - Second, the i2c_smbus_write_block_data write ends up in writing
> >   unexpected value to the channel_dis register, this is because
> >   the smbus size that is 0x03 in this case gets written to the
> >   register. The PAC1931/2/3/4 data sheet does not really specify
> >   that block write is indeed supported.
> > 
> > This problem is probably not visible on PAC1934 version where all
> > channels are used as the chip is properly configured by luck,
> > but in our case whenusing PAC1931 this leads to nonfunctional device.
> > 
> > Signed-off-by: Rene Straub <mailto:rene.straub@belden.com>
This SoB chain doesn't comply with the normal requirements.
Given the author is Aleksandar (no From in the commit, so it's the email sender)
my guess is either

a) Rene wrote the patch and the author of the commit accidentally got changed)
b) Rene and Aleksandar both wrote the patch in which case it is missing
Co-developed-by: Rene Straub <mailto:rene.straub@belden.com>
here

I can fix it up if you confirm if it was a or b or something else

Thanks,

Jonathan

> > Signed-off-by: Aleksandar Gerasimovski <mailto:aleksandar.gerasimovski@belden.com>  
> 
> Hi, Thanks for the patch.
> 
> Needs a fixes tag so we know how far to back port it.
> You can just reply with one to this thread rather than sending a new
> version.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/adc/pac1934.c | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/pac1934.c b/drivers/iio/adc/pac1934.c
> > index 09fe88eb3fb0..2e442e46f679 100644
> > --- a/drivers/iio/adc/pac1934.c
> > +++ b/drivers/iio/adc/pac1934.c
> > @@ -88,6 +88,7 @@
> >  #define PAC1934_VPOWER_3_ADDR			0x19
> >  #define PAC1934_VPOWER_4_ADDR			0x1A
> >  #define PAC1934_REFRESH_V_REG_ADDR		0x1F
> > +#define PAC1934_SLOW_REG_ADDR			0x20
> >  #define PAC1934_CTRL_STAT_REGS_ADDR		0x1C
> >  #define PAC1934_PID_REG_ADDR			0xFD
> >  #define PAC1934_MID_REG_ADDR			0xFE
> > @@ -1265,8 +1266,23 @@ static int pac1934_chip_configure(struct pac1934_chip_info *info)
> >  	/* no SLOW triggered REFRESH, clear POR */
> >  	regs[PAC1934_SLOW_REG_OFF] = 0;
> >  
> > -	ret =  i2c_smbus_write_block_data(client, PAC1934_CTRL_STAT_REGS_ADDR,
> > -					  ARRAY_SIZE(regs), (u8 *)regs);
> > +	/*
> > +	 * Write the three bytes sequentially, as the device does not support
> > +	 * block write.
> > +	 */
> > +	ret = i2c_smbus_write_byte_data(client, PAC1934_CTRL_STAT_REGS_ADDR,
> > +					regs[PAC1934_CHANNEL_DIS_REG_OFF]);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = i2c_smbus_write_byte_data(client,
> > +					PAC1934_CTRL_STAT_REGS_ADDR + PAC1934_NEG_PWR_REG_OFF,
> > +					regs[PAC1934_NEG_PWR_REG_OFF]);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = i2c_smbus_write_byte_data(client, PAC1934_SLOW_REG_ADDR,
> > +					regs[PAC1934_SLOW_REG_OFF]);
> >  	if (ret)
> >  		return ret;
> >    
> 
> **********************************************************************
> DISCLAIMER:
> Privileged and/or Confidential information may be contained in this message. If you are not the addressee of this message, you may not copy, use or deliver this message to anyone. In such event, you should destroy the message and kindly notify the sender by reply e-mail. It is understood that opinions or conclusions that do not relate to the official business of the company are neither given nor endorsed by the company. Thank You.

**********************************************************************
DISCLAIMER:
Privileged and/or Confidential information may be contained in this message. If you are not the addressee of this message, you may not copy, use or deliver this message to anyone. In such event, you should destroy the message and kindly notify the sender by reply e-mail. It is understood that opinions or conclusions that do not relate to the official business of the company are neither given nor endorsed by the company. Thank You.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] iio/adc/pac1934: fix channel disable configuration
  2025-08-19  8:57       ` Aleksandar Gerasimovski
@ 2025-08-25  9:45         ` Jonathan Cameron
  0 siblings, 0 replies; 7+ messages in thread
From: Jonathan Cameron @ 2025-08-25  9:45 UTC (permalink / raw)
  To: Aleksandar Gerasimovski
  Cc: linux-iio@vger.kernel.org, marius.cristea@microchip.com,
	Rene Straub

On Tue, 19 Aug 2025 08:57:40 +0000
Aleksandar Gerasimovski <Aleksandar.Gerasimovski@belden.com> wrote:

> Hi Jonathan,
> 
> Both Rene and I was investigating the problem in first place, I come with the patch afterwards.
> So, the sign would be then:
> 
> Suggested-by: Rene Straub <mailto:rene.straub@belden.com>
> Signed-off-by: Aleksandar Gerasimovski <aleksandar.gerasimovski@belden.com>
> 
> Thanks for following up!
> 
Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan

> From: Jonathan Cameron <jic23@kernel.org> 
> Sent: Monday, August 18, 2025 7:56 PM
> To: Aleksandar Gerasimovski <Aleksandar.Gerasimovski@belden.com>
> Cc: linux-iio@vger.kernel.org; marius.cristea@microchip.com; Rene Straub <Rene.Straub@belden.com>
> Subject: Re: [PATCH] iio/adc/pac1934: fix channel disable configuration
> 
> On Mon, 18 Aug 2025 11: 08: 33 +0000 Aleksandar Gerasimovski <Aleksandar. Gerasimovski@ belden. com> wrote: > Hi Jonathan, and sorry for the previous non-plain text message: > > Fixes: 0fb528c8255b (iio: adc: adding support for PAC193x)
> 
> On Mon, 18 Aug 2025 11:08:33 +0000
> Aleksandar Gerasimovski <mailto:Aleksandar.Gerasimovski@belden.com> wrote:
> 
> > Hi Jonathan, and sorry for the previous non-plain text message:
> > 
> > Fixes: 0fb528c8255b (iio: adc: adding support for PAC193x)  
> Thanks.  I noticed one more thing when I was about to queue this up..
> 
> > 
> > Regards,
> > Aleksandar
> > 
> > From: Jonathan Cameron <mailto:jic23@kernel.org> 
> > Sent: Saturday, August 16, 2025 3:44 PM
> > To: Aleksandar Gerasimovski <mailto:Aleksandar.Gerasimovski@belden.com>
> > Cc: mailto:linux-iio@vger.kernel.org; mailto:marius.cristea@microchip.com; Rene Straub <mailto:Rene.Straub@belden.com>
> > Subject: Re: [PATCH] iio/adc/pac1934: fix channel disable configuration
> > 
> > On Mon, 11 Aug 2025 13: 09: 04 +0000 Aleksandar Gerasimovski <aleksandar. gerasimovski@ belden. com> wrote: > There are two problems with the chip configuration in this driver: > - First, is that writing 12 bytes (ARRAY_SIZE(regs)) would
> > 
> > On Mon, 11 Aug 2025 13:09:04 +0000
> > Aleksandar Gerasimovski <mailto:aleksandar.gerasimovski@belden.com> wrote:
> >   
> > > There are two problems with the chip configuration in this driver:
> > > - First, is that writing 12 bytes (ARRAY_SIZE(regs)) would anyhow
> > >   lead to a config overflow due to HW auto increment implementation
> > >   in the chip.
> > > - Second, the i2c_smbus_write_block_data write ends up in writing
> > >   unexpected value to the channel_dis register, this is because
> > >   the smbus size that is 0x03 in this case gets written to the
> > >   register. The PAC1931/2/3/4 data sheet does not really specify
> > >   that block write is indeed supported.
> > > 
> > > This problem is probably not visible on PAC1934 version where all
> > > channels are used as the chip is properly configured by luck,
> > > but in our case whenusing PAC1931 this leads to nonfunctional device.
> > > 
> > > Signed-off-by: Rene Straub <mailto:rene.straub@belden.com>  
> This SoB chain doesn't comply with the normal requirements.
> Given the author is Aleksandar (no From in the commit, so it's the email sender)
> my guess is either
> 
> a) Rene wrote the patch and the author of the commit accidentally got changed)
> b) Rene and Aleksandar both wrote the patch in which case it is missing
> Co-developed-by: Rene Straub <mailto:rene.straub@belden.com>
> here
> 
> I can fix it up if you confirm if it was a or b or something else
> 
> Thanks,
> 
> Jonathan
> 
> > > Signed-off-by: Aleksandar Gerasimovski <mailto:aleksandar.gerasimovski@belden.com>    
> > 
> > Hi, Thanks for the patch.
> > 
> > Needs a fixes tag so we know how far to back port it.
> > You can just reply with one to this thread rather than sending a new
> > version.
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> > > ---
> > >  drivers/iio/adc/pac1934.c | 20 ++++++++++++++++++--
> > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/pac1934.c b/drivers/iio/adc/pac1934.c
> > > index 09fe88eb3fb0..2e442e46f679 100644
> > > --- a/drivers/iio/adc/pac1934.c
> > > +++ b/drivers/iio/adc/pac1934.c
> > > @@ -88,6 +88,7 @@
> > >  #define PAC1934_VPOWER_3_ADDR			0x19
> > >  #define PAC1934_VPOWER_4_ADDR			0x1A
> > >  #define PAC1934_REFRESH_V_REG_ADDR		0x1F
> > > +#define PAC1934_SLOW_REG_ADDR			0x20
> > >  #define PAC1934_CTRL_STAT_REGS_ADDR		0x1C
> > >  #define PAC1934_PID_REG_ADDR			0xFD
> > >  #define PAC1934_MID_REG_ADDR			0xFE
> > > @@ -1265,8 +1266,23 @@ static int pac1934_chip_configure(struct pac1934_chip_info *info)
> > >  	/* no SLOW triggered REFRESH, clear POR */
> > >  	regs[PAC1934_SLOW_REG_OFF] = 0;
> > >  
> > > -	ret =  i2c_smbus_write_block_data(client, PAC1934_CTRL_STAT_REGS_ADDR,
> > > -					  ARRAY_SIZE(regs), (u8 *)regs);
> > > +	/*
> > > +	 * Write the three bytes sequentially, as the device does not support
> > > +	 * block write.
> > > +	 */
> > > +	ret = i2c_smbus_write_byte_data(client, PAC1934_CTRL_STAT_REGS_ADDR,
> > > +					regs[PAC1934_CHANNEL_DIS_REG_OFF]);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = i2c_smbus_write_byte_data(client,
> > > +					PAC1934_CTRL_STAT_REGS_ADDR + PAC1934_NEG_PWR_REG_OFF,
> > > +					regs[PAC1934_NEG_PWR_REG_OFF]);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = i2c_smbus_write_byte_data(client, PAC1934_SLOW_REG_ADDR,
> > > +					regs[PAC1934_SLOW_REG_OFF]);
> > >  	if (ret)
> > >  		return ret;
> > >      
> > 
> > **********************************************************************
> > DISCLAIMER:
> > Privileged and/or Confidential information may be contained in this message. If you are not the addressee of this message, you may not copy, use or deliver this message to anyone. In such event, you should destroy the message and kindly notify the sender by reply e-mail. It is understood that opinions or conclusions that do not relate to the official business of the company are neither given nor endorsed by the company. Thank You.  
> 
> **********************************************************************
> DISCLAIMER:
> Privileged and/or Confidential information may be contained in this message. If you are not the addressee of this message, you may not copy, use or deliver this message to anyone. In such event, you should destroy the message and kindly notify the sender by reply e-mail. It is understood that opinions or conclusions that do not relate to the official business of the company are neither given nor endorsed by the company. Thank You.
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-08-25  9:45 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-11 13:09 [PATCH] iio/adc/pac1934: fix channel disable configuration Aleksandar Gerasimovski
2025-08-12  8:36 ` Marius.Cristea
2025-08-16 13:43 ` Jonathan Cameron
2025-08-18 11:08   ` Aleksandar Gerasimovski
2025-08-18 17:56     ` Jonathan Cameron
2025-08-19  8:57       ` Aleksandar Gerasimovski
2025-08-25  9:45         ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox