All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5] mfd: da9063: Support SMBus and I2C mode
@ 2021-03-15 16:09 Mark Jonas
  2021-03-15 18:31 ` Wolfram Sang
  2021-03-16  7:21 ` Lee Jones
  0 siblings, 2 replies; 6+ messages in thread
From: Mark Jonas @ 2021-03-15 16:09 UTC (permalink / raw)
  To: Support Opensource, Lee Jones
  Cc: linux-kernel, linux-i2c, Adam.Thomson.Opensource,
	stwiss.opensource, marek.vasut, tingquan.ruan, hubert.streidl,
	Wolfram Sang, Mark Jonas

From: Hubert Streidl <hubert.streidl@de.bosch.com>

By default the PMIC DA9063 2-wire interface is SMBus compliant. This
means the PMIC will automatically reset the interface when the clock
signal ceases for more than the SMBus timeout of 35 ms.

If the I2C driver / device is not capable of creating atomic I2C
transactions, a context change can cause a ceasing of the clock signal.
This can happen if for example a real-time thread is scheduled. Then
the DA9063 in SMBus mode will reset the 2-wire interface. Subsequently
a write message could end up in the wrong register. This could cause
unpredictable system behavior.

The DA9063 PMIC also supports an I2C compliant mode for the 2-wire
interface. This mode does not reset the interface when the clock
signal ceases. Thus the problem depicted above does not occur.

This patch tests for the bus functionality "I2C_FUNC_I2C". It can
reasonably be assumed that the bus cannot obey SMBus timings if
this functionality is set. SMBus commands most probably are emulated
in this case which is prone to the latency issue described above.

This patch enables the I2C bus mode if I2C_FUNC_I2C is set or
otherwise keeps the default SMBus mode.

Signed-off-by: Hubert Streidl <hubert.streidl@de.bosch.com>
Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
---
 drivers/mfd/da9063-i2c.c             | 10 ++++++++++
 include/linux/mfd/da9063/registers.h |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
index 3781d0bb7786..e8a022e697c5 100644
--- a/drivers/mfd/da9063-i2c.c
+++ b/drivers/mfd/da9063-i2c.c
@@ -442,6 +442,16 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
+	/* If SMBus is not available and only I2C is possible, enter I2C mode */
+	if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) {
+		ret = regmap_clear_bits(da9063->regmap, DA9063_REG_CONFIG_J,
+			  DA9063_TWOWIRE_TO);
+		if (ret < 0) {
+			dev_err(da9063->dev, "Failed to set Two-Wire Bus Mode.\n");
+			return -EIO;
+		}
+	}
+
 	return da9063_device_init(da9063, i2c->irq);
 }
 
diff --git a/include/linux/mfd/da9063/registers.h b/include/linux/mfd/da9063/registers.h
index 1dbabf1b3cb8..6e0f66a2e727 100644
--- a/include/linux/mfd/da9063/registers.h
+++ b/include/linux/mfd/da9063/registers.h
@@ -1037,6 +1037,9 @@
 #define		DA9063_NONKEY_PIN_AUTODOWN	0x02
 #define		DA9063_NONKEY_PIN_AUTOFLPRT	0x03
 
+/* DA9063_REG_CONFIG_J (addr=0x10F) */
+#define DA9063_TWOWIRE_TO			0x40
+
 /* DA9063_REG_MON_REG_5 (addr=0x116) */
 #define DA9063_MON_A8_IDX_MASK			0x07
 #define		DA9063_MON_A8_IDX_NONE		0x00
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v5] mfd: da9063: Support SMBus and I2C mode
  2021-03-15 16:09 [PATCH v5] mfd: da9063: Support SMBus and I2C mode Mark Jonas
@ 2021-03-15 18:31 ` Wolfram Sang
  2021-03-16  7:21 ` Lee Jones
  1 sibling, 0 replies; 6+ messages in thread
From: Wolfram Sang @ 2021-03-15 18:31 UTC (permalink / raw)
  To: Mark Jonas
  Cc: Support Opensource, Lee Jones, linux-kernel, linux-i2c,
	Adam.Thomson.Opensource, stwiss.opensource, marek.vasut,
	tingquan.ruan, hubert.streidl

[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]

On Mon, Mar 15, 2021 at 05:09:03PM +0100, Mark Jonas wrote:
> From: Hubert Streidl <hubert.streidl@de.bosch.com>
> 
> By default the PMIC DA9063 2-wire interface is SMBus compliant. This
> means the PMIC will automatically reset the interface when the clock
> signal ceases for more than the SMBus timeout of 35 ms.
> 
> If the I2C driver / device is not capable of creating atomic I2C
> transactions, a context change can cause a ceasing of the clock signal.
> This can happen if for example a real-time thread is scheduled. Then
> the DA9063 in SMBus mode will reset the 2-wire interface. Subsequently
> a write message could end up in the wrong register. This could cause
> unpredictable system behavior.
> 
> The DA9063 PMIC also supports an I2C compliant mode for the 2-wire
> interface. This mode does not reset the interface when the clock
> signal ceases. Thus the problem depicted above does not occur.
> 
> This patch tests for the bus functionality "I2C_FUNC_I2C". It can
> reasonably be assumed that the bus cannot obey SMBus timings if
> this functionality is set. SMBus commands most probably are emulated
> in this case which is prone to the latency issue described above.
> 
> This patch enables the I2C bus mode if I2C_FUNC_I2C is set or
> otherwise keeps the default SMBus mode.
> 
> Signed-off-by: Hubert Streidl <hubert.streidl@de.bosch.com>
> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>

From an I2C point of view, this is the correct approach:

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v5] mfd: da9063: Support SMBus and I2C mode
  2021-03-15 16:09 [PATCH v5] mfd: da9063: Support SMBus and I2C mode Mark Jonas
  2021-03-15 18:31 ` Wolfram Sang
@ 2021-03-16  7:21 ` Lee Jones
  2021-03-16  8:23   ` Jonas Mark (BT-FIR/ENG1-Grb)
  1 sibling, 1 reply; 6+ messages in thread
From: Lee Jones @ 2021-03-16  7:21 UTC (permalink / raw)
  To: Mark Jonas
  Cc: Support Opensource, linux-kernel, linux-i2c,
	Adam.Thomson.Opensource, stwiss.opensource, marek.vasut,
	tingquan.ruan, hubert.streidl, Wolfram Sang

On Mon, 15 Mar 2021, Mark Jonas wrote:

> From: Hubert Streidl <hubert.streidl@de.bosch.com>
> 
> By default the PMIC DA9063 2-wire interface is SMBus compliant. This
> means the PMIC will automatically reset the interface when the clock
> signal ceases for more than the SMBus timeout of 35 ms.
> 
> If the I2C driver / device is not capable of creating atomic I2C
> transactions, a context change can cause a ceasing of the clock signal.
> This can happen if for example a real-time thread is scheduled. Then
> the DA9063 in SMBus mode will reset the 2-wire interface. Subsequently
> a write message could end up in the wrong register. This could cause
> unpredictable system behavior.
> 
> The DA9063 PMIC also supports an I2C compliant mode for the 2-wire
> interface. This mode does not reset the interface when the clock
> signal ceases. Thus the problem depicted above does not occur.
> 
> This patch tests for the bus functionality "I2C_FUNC_I2C". It can
> reasonably be assumed that the bus cannot obey SMBus timings if
> this functionality is set. SMBus commands most probably are emulated
> in this case which is prone to the latency issue described above.
> 
> This patch enables the I2C bus mode if I2C_FUNC_I2C is set or
> otherwise keeps the default SMBus mode.
> 
> Signed-off-by: Hubert Streidl <hubert.streidl@de.bosch.com>
> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
> ---
>  drivers/mfd/da9063-i2c.c             | 10 ++++++++++
>  include/linux/mfd/da9063/registers.h |  3 +++
>  2 files changed, 13 insertions(+)

Code looks good to me now, thanks.

However, this doesn't look like it would pass checkpatch.

Have you tried to build with W=1 and checkpatch?

> diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
> index 3781d0bb7786..e8a022e697c5 100644
> --- a/drivers/mfd/da9063-i2c.c
> +++ b/drivers/mfd/da9063-i2c.c
> @@ -442,6 +442,16 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
>  		return ret;
>  	}
>  
> +	/* If SMBus is not available and only I2C is possible, enter I2C mode */
> +	if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) {
> +		ret = regmap_clear_bits(da9063->regmap, DA9063_REG_CONFIG_J,
> +			  DA9063_TWOWIRE_TO);
> +		if (ret < 0) {
> +			dev_err(da9063->dev, "Failed to set Two-Wire Bus Mode.\n");
> +			return -EIO;
> +		}
> +	}
> +
>  	return da9063_device_init(da9063, i2c->irq);
>  }
>  
> diff --git a/include/linux/mfd/da9063/registers.h b/include/linux/mfd/da9063/registers.h
> index 1dbabf1b3cb8..6e0f66a2e727 100644
> --- a/include/linux/mfd/da9063/registers.h
> +++ b/include/linux/mfd/da9063/registers.h
> @@ -1037,6 +1037,9 @@
>  #define		DA9063_NONKEY_PIN_AUTODOWN	0x02
>  #define		DA9063_NONKEY_PIN_AUTOFLPRT	0x03
>  
> +/* DA9063_REG_CONFIG_J (addr=0x10F) */
> +#define DA9063_TWOWIRE_TO			0x40
> +
>  /* DA9063_REG_MON_REG_5 (addr=0x116) */
>  #define DA9063_MON_A8_IDX_MASK			0x07
>  #define		DA9063_MON_A8_IDX_NONE		0x00

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v5] mfd: da9063: Support SMBus and I2C mode
  2021-03-16  7:21 ` Lee Jones
@ 2021-03-16  8:23   ` Jonas Mark (BT-FIR/ENG1-Grb)
  2021-03-16  9:54     ` Lee Jones
  2021-03-16  9:54     ` Lee Jones
  0 siblings, 2 replies; 6+ messages in thread
From: Jonas Mark (BT-FIR/ENG1-Grb) @ 2021-03-16  8:23 UTC (permalink / raw)
  To: Lee Jones
  Cc: Support Opensource, linux-kernel, linux-i2c,
	Adam.Thomson.Opensource, stwiss.opensource, marek.vasut,
	RUAN Tingquan (BT-FIR/ENG1-Zhu), Streidl Hubert (BT-FIR/ENG1-Grb),
	Wolfram Sang, Jonas Mark (BT-FIR/ENG1-Grb)

Hi Lee,

> Code looks good to me now, thanks.
> 
> However, this doesn't look like it would pass checkpatch.
> 
> Have you tried to build with W=1 and checkpatch?

Yes, we used checkpatch.pl.

    $ ./scripts/checkpatch.pl 0001-mfd-da9063-Support-SMBus-and-I2C-mode.v5
    total: 0 errors, 0 warnings, 25 lines checked

    0001-mfd-da9063-Support-SMBus-and-I2C-mode.v5 has no obvious style problems and is ready for submission.

Using the option --strict we get a check hint that the broken line of the regmap_clear_bits() is not aligned. We tried but were not able to make the tool happy. This matches our experience with this check hint and previous patches.
 
Also compiling Linux 5.10.14 with our patch and W=1 does not yield a warning.

    $ make W=1
      CALL    scripts/checksyscalls.sh
      CALL    scripts/atomic/check-atomics.sh
      CHK     include/generated/compile.h
      CC [M]  drivers/mfd/da9063-i2c.o
      LD [M]  drivers/mfd/da9063.o
      Kernel: arch/arm/boot/Image is ready
      Kernel: arch/arm/boot/zImage is ready
      MODPOST Module.symvers
      LD [M]  drivers/mfd/da9063.ko

> > diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c index
> > 3781d0bb7786..e8a022e697c5 100644
> > --- a/drivers/mfd/da9063-i2c.c
> > +++ b/drivers/mfd/da9063-i2c.c
> > @@ -442,6 +442,16 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
> >  		return ret;
> >  	}
> >
> > +	/* If SMBus is not available and only I2C is possible, enter I2C mode */
> > +	if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) {
> > +		ret = regmap_clear_bits(da9063->regmap,
> DA9063_REG_CONFIG_J,
> > +			  DA9063_TWOWIRE_TO);
> > +		if (ret < 0) {
> > +			dev_err(da9063->dev, "Failed to set Two-Wire Bus
> Mode.\n");
> > +			return -EIO;
> > +		}
> > +	}
> > +
> >  	return da9063_device_init(da9063, i2c->irq);  }
> >
> > diff --git a/include/linux/mfd/da9063/registers.h
> > b/include/linux/mfd/da9063/registers.h
> > index 1dbabf1b3cb8..6e0f66a2e727 100644
> > --- a/include/linux/mfd/da9063/registers.h
> > +++ b/include/linux/mfd/da9063/registers.h
> > @@ -1037,6 +1037,9 @@
> >  #define		DA9063_NONKEY_PIN_AUTODOWN	0x02
> >  #define		DA9063_NONKEY_PIN_AUTOFLPRT	0x03
> >
> > +/* DA9063_REG_CONFIG_J (addr=0x10F) */
> > +#define DA9063_TWOWIRE_TO			0x40
> > +
> >  /* DA9063_REG_MON_REG_5 (addr=0x116) */
> >  #define DA9063_MON_A8_IDX_MASK			0x07
> >  #define		DA9063_MON_A8_IDX_NONE		0x00

Cheers,
Mark

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v5] mfd: da9063: Support SMBus and I2C mode
  2021-03-16  8:23   ` Jonas Mark (BT-FIR/ENG1-Grb)
@ 2021-03-16  9:54     ` Lee Jones
  2021-03-16  9:54     ` Lee Jones
  1 sibling, 0 replies; 6+ messages in thread
From: Lee Jones @ 2021-03-16  9:54 UTC (permalink / raw)
  To: Jonas Mark (BT-FIR/ENG1-Grb)
  Cc: Support Opensource, linux-kernel, linux-i2c,
	Adam.Thomson.Opensource, stwiss.opensource, marek.vasut,
	RUAN Tingquan (BT-FIR/ENG1-Zhu), Streidl Hubert (BT-FIR/ENG1-Grb),
	Wolfram Sang

On Tue, 16 Mar 2021, Jonas Mark (BT-FIR/ENG1-Grb) wrote:

> Hi Lee,
> 
> > Code looks good to me now, thanks.
> > 
> > However, this doesn't look like it would pass checkpatch.
> > 
> > Have you tried to build with W=1 and checkpatch?
> 
> Yes, we used checkpatch.pl.
> 
>     $ ./scripts/checkpatch.pl 0001-mfd-da9063-Support-SMBus-and-I2C-mode.v5
>     total: 0 errors, 0 warnings, 25 lines checked
> 
>     0001-mfd-da9063-Support-SMBus-and-I2C-mode.v5 has no obvious style problems and is ready for submission.
> 
> Using the option --strict we get a check hint that the broken line
> of the regmap_clear_bits() is not aligned. We tried but were not
> able to make the tool happy. This matches our experience with this
> check hint and previous patches.

Lining up the first char of the broken line with the open parenthesis
will make checkpatch happy.  It's easier if you configure your editor
to always do this, rather than with strict tabs.

> Also compiling Linux 5.10.14 with our patch and W=1 does not yield a warning.
> 
>     $ make W=1
>       CALL    scripts/checksyscalls.sh
>       CALL    scripts/atomic/check-atomics.sh
>       CHK     include/generated/compile.h
>       CC [M]  drivers/mfd/da9063-i2c.o
>       LD [M]  drivers/mfd/da9063.o
>       Kernel: arch/arm/boot/Image is ready
>       Kernel: arch/arm/boot/zImage is ready
>       MODPOST Module.symvers
>       LD [M]  drivers/mfd/da9063.ko
> 
> > > diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c index
> > > 3781d0bb7786..e8a022e697c5 100644
> > > --- a/drivers/mfd/da9063-i2c.c
> > > +++ b/drivers/mfd/da9063-i2c.c
> > > @@ -442,6 +442,16 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
> > >  		return ret;
> > >  	}
> > >
> > > +	/* If SMBus is not available and only I2C is possible, enter I2C mode */
> > > +	if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) {
> > > +		ret = regmap_clear_bits(da9063->regmap,
> > DA9063_REG_CONFIG_J,
> > > +			  DA9063_TWOWIRE_TO);
> > > +		if (ret < 0) {
> > > +			dev_err(da9063->dev, "Failed to set Two-Wire Bus
> > Mode.\n");
> > > +			return -EIO;
> > > +		}
> > > +	}
> > > +
> > >  	return da9063_device_init(da9063, i2c->irq);  }
> > >
> > > diff --git a/include/linux/mfd/da9063/registers.h
> > > b/include/linux/mfd/da9063/registers.h
> > > index 1dbabf1b3cb8..6e0f66a2e727 100644
> > > --- a/include/linux/mfd/da9063/registers.h
> > > +++ b/include/linux/mfd/da9063/registers.h
> > > @@ -1037,6 +1037,9 @@
> > >  #define		DA9063_NONKEY_PIN_AUTODOWN	0x02
> > >  #define		DA9063_NONKEY_PIN_AUTOFLPRT	0x03
> > >
> > > +/* DA9063_REG_CONFIG_J (addr=0x10F) */
> > > +#define DA9063_TWOWIRE_TO			0x40
> > > +
> > >  /* DA9063_REG_MON_REG_5 (addr=0x116) */
> > >  #define DA9063_MON_A8_IDX_MASK			0x07
> > >  #define		DA9063_MON_A8_IDX_NONE		0x00
> 

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v5] mfd: da9063: Support SMBus and I2C mode
  2021-03-16  8:23   ` Jonas Mark (BT-FIR/ENG1-Grb)
  2021-03-16  9:54     ` Lee Jones
@ 2021-03-16  9:54     ` Lee Jones
  1 sibling, 0 replies; 6+ messages in thread
From: Lee Jones @ 2021-03-16  9:54 UTC (permalink / raw)
  To: Jonas Mark (BT-FIR/ENG1-Grb)
  Cc: Support Opensource, linux-kernel, linux-i2c,
	Adam.Thomson.Opensource, stwiss.opensource, marek.vasut,
	RUAN Tingquan (BT-FIR/ENG1-Zhu), Streidl Hubert (BT-FIR/ENG1-Grb),
	Wolfram Sang

On Tue, 16 Mar 2021, Jonas Mark (BT-FIR/ENG1-Grb) wrote:

> Hi Lee,
> 
> > Code looks good to me now, thanks.
> > 
> > However, this doesn't look like it would pass checkpatch.
> > 
> > Have you tried to build with W=1 and checkpatch?
> 
> Yes, we used checkpatch.pl.
> 
>     $ ./scripts/checkpatch.pl 0001-mfd-da9063-Support-SMBus-and-I2C-mode.v5
>     total: 0 errors, 0 warnings, 25 lines checked
> 
>     0001-mfd-da9063-Support-SMBus-and-I2C-mode.v5 has no obvious style problems and is ready for submission.
> 
> Using the option --strict we get a check hint that the broken line of the regmap_clear_bits() is not aligned. We tried but were not able to make the tool happy. This matches our experience with this check hint and previous patches.
>  
> Also compiling Linux 5.10.14 with our patch and W=1 does not yield a warning.

FYI, you should be using -next for upstream development.

>     $ make W=1
>       CALL    scripts/checksyscalls.sh
>       CALL    scripts/atomic/check-atomics.sh
>       CHK     include/generated/compile.h
>       CC [M]  drivers/mfd/da9063-i2c.o
>       LD [M]  drivers/mfd/da9063.o
>       Kernel: arch/arm/boot/Image is ready
>       Kernel: arch/arm/boot/zImage is ready
>       MODPOST Module.symvers
>       LD [M]  drivers/mfd/da9063.ko

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-16  9:55 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-15 16:09 [PATCH v5] mfd: da9063: Support SMBus and I2C mode Mark Jonas
2021-03-15 18:31 ` Wolfram Sang
2021-03-16  7:21 ` Lee Jones
2021-03-16  8:23   ` Jonas Mark (BT-FIR/ENG1-Grb)
2021-03-16  9:54     ` Lee Jones
2021-03-16  9:54     ` Lee Jones

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.