linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [v2,1/2] i2c: pca954x: Add property to skip disabling PCA954x MUX device
@ 2019-09-30  3:25 Biwen Li
  2019-09-30  3:25 ` [v2,2/2] dt-bindings: i2c-mux-pca954x: Add optional property i2c-mux-never-disable Biwen Li
  0 siblings, 1 reply; 7+ messages in thread
From: Biwen Li @ 2019-09-30  3:25 UTC (permalink / raw)
  To: peda, leoyang.li, robh+dt, mark.rutland
  Cc: linux-i2c, linux-kernel, devicetree, Biwen Li

On some Layerscape boards like LS2085ARDB and LS2088ARDB,
input pull-up resistors on PCA954x MUX device are missing on board,
So, if MUX are disabled after powered-on, input lines will float
leading to incorrect functionality.

Hence, PCA954x MUX device should never be turned-off after
power-on.

Add property to skip disabling PCA954x MUX device
if device tree contains "i2c-mux-never-disable"
for PCA954x device node.

Errata ID: E-00013 on board LS2085ARDB and LS2088ARDB
(The hardware bug found on board revision
Rev.B, Rev.C and Rev.D)

Signed-off-by: Biwen Li <biwen.li@nxp.com>
---
Change in v2:
	- update variable name
	  disable_mux->never_disable

 drivers/i2c/muxes/i2c-mux-pca954x.c | 37 +++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 923aa3a5a3dc..b4647b033163 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -93,6 +93,11 @@ struct pca954x {
 	struct irq_domain *irq;
 	unsigned int irq_mask;
 	raw_spinlock_t lock;
+	/*
+	 * never disable value will write to control register of mux
+	 * to always enable mux
+	 */
+	u8 never_disable;
 };
 
 /* Provide specs for the PCA954x types we know about */
@@ -258,6 +263,11 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
 	struct i2c_client *client = data->client;
 	s8 idle_state;
 
+	if (data->never_disable) {
+		data->last_chan = data->chip->nchans;
+		return pca954x_reg_write(muxc->parent, client, data->never_disable);
+	}
+
 	idle_state = READ_ONCE(data->idle_state);
 	if (idle_state >= 0)
 		/* Set the mux back to a predetermined channel */
@@ -462,16 +472,32 @@ static int pca954x_probe(struct i2c_client *client,
 		}
 	}
 
+	/* Errata ID E-00013 on board LS2088ARDB and LS2088ARDB:
+	 * The point here is that you must not disable a mux if there
+	 * are no pullups on the input or you mess up the I2C. This
+	 * needs to be put into the DTS really as the kernel cannot
+	 * know this otherwise.
+	 */
+
+	data->never_disable = np &&
+		of_property_read_bool(np, "i2c-mux-never-disable") &&
+		data->chip->muxtype == pca954x_ismux ?
+		data->chip->enable : 0;
+
 	/* Write the mux register at addr to verify
 	 * that the mux is in fact present. This also
 	 * initializes the mux to disconnected state.
 	 */
-	if (i2c_smbus_write_byte(client, 0) < 0) {
+	if (i2c_smbus_write_byte(client, data->never_disable) < 0) {
 		dev_warn(dev, "probe failed\n");
 		return -ENODEV;
 	}
 
-	data->last_chan = 0;		   /* force the first selection */
+	if (data->never_disable)
+		data->last_chan = data->chip->nchans;
+	else
+		data->last_chan = 0;               /* force the first selection */
+
 	data->idle_state = MUX_IDLE_AS_IS;
 
 	idle_disconnect_dt = np &&
@@ -531,8 +557,11 @@ static int pca954x_resume(struct device *dev)
 	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
 	struct pca954x *data = i2c_mux_priv(muxc);
 
-	data->last_chan = 0;
-	return i2c_smbus_write_byte(client, 0);
+	if (data->never_disable)
+		data->last_chan = data->chip->nchans;
+	else
+		data->last_chan = 0;
+	return i2c_smbus_write_byte(client, data->never_disable);
 }
 #endif
 
-- 
2.17.1


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

* [v2,2/2] dt-bindings: i2c-mux-pca954x: Add optional property i2c-mux-never-disable
  2019-09-30  3:25 [v2,1/2] i2c: pca954x: Add property to skip disabling PCA954x MUX device Biwen Li
@ 2019-09-30  3:25 ` Biwen Li
  2019-10-11 14:44   ` Rob Herring
  0 siblings, 1 reply; 7+ messages in thread
From: Biwen Li @ 2019-09-30  3:25 UTC (permalink / raw)
  To: peda, leoyang.li, robh+dt, mark.rutland
  Cc: linux-i2c, linux-kernel, devicetree, Biwen Li

The patch adds an optional property i2c-mux-never-disable

Signed-off-by: Biwen Li <biwen.li@nxp.com>
---
Change in v2:
	- update documentation

 Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index 30ac6a60f041..71b73d0fdb62 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -34,6 +34,7 @@ Optional Properties:
     - first cell is the pin number
     - second cell is used to specify flags.
     See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+  - i2c-mux-never-disable: always forces mux to be enabled.
 
 Example:
 
-- 
2.17.1


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

* Re: [v2,2/2] dt-bindings: i2c-mux-pca954x: Add optional property i2c-mux-never-disable
  2019-09-30  3:25 ` [v2,2/2] dt-bindings: i2c-mux-pca954x: Add optional property i2c-mux-never-disable Biwen Li
@ 2019-10-11 14:44   ` Rob Herring
  2019-10-14  3:21     ` [EXT] " Biwen Li
  0 siblings, 1 reply; 7+ messages in thread
From: Rob Herring @ 2019-10-11 14:44 UTC (permalink / raw)
  To: Biwen Li
  Cc: peda, leoyang.li, mark.rutland, linux-i2c, linux-kernel, devicetree

On Mon, Sep 30, 2019 at 11:25:03AM +0800, Biwen Li wrote:
> The patch adds an optional property i2c-mux-never-disable
> 
> Signed-off-by: Biwen Li <biwen.li@nxp.com>
> ---
> Change in v2:
> 	- update documentation
> 
>  Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> index 30ac6a60f041..71b73d0fdb62 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> @@ -34,6 +34,7 @@ Optional Properties:
>      - first cell is the pin number
>      - second cell is used to specify flags.
>      See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +  - i2c-mux-never-disable: always forces mux to be enabled.

Either needs to have a vendor prefix or be documented as a common 
property.

IIRC, we already have a property for mux default state which seems like 
that would cover this unless you need to leave it in different states.

Rob

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

* RE: [EXT] Re: [v2,2/2] dt-bindings: i2c-mux-pca954x: Add optional property i2c-mux-never-disable
  2019-10-11 14:44   ` Rob Herring
@ 2019-10-14  3:21     ` Biwen Li
  2019-10-14  4:16       ` Biwen Li
  0 siblings, 1 reply; 7+ messages in thread
From: Biwen Li @ 2019-10-14  3:21 UTC (permalink / raw)
  To: Rob Herring
  Cc: peda, Leo Li, mark.rutland, linux-i2c, linux-kernel, devicetree

> 
> On Mon, Sep 30, 2019 at 11:25:03AM +0800, Biwen Li wrote:
> > The patch adds an optional property i2c-mux-never-disable
> >
> > Signed-off-by: Biwen Li <biwen.li@nxp.com>
> > ---
> > Change in v2:
> >       - update documentation
> >
> >  Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> > b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> > index 30ac6a60f041..71b73d0fdb62 100644
> > --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> > @@ -34,6 +34,7 @@ Optional Properties:
> >      - first cell is the pin number
> >      - second cell is used to specify flags.
> >      See also
> > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> > +  - i2c-mux-never-disable: always forces mux to be enabled.
> 
> Either needs to have a vendor prefix or be documented as a common
> property.
> 
> IIRC, we already have a property for mux default state which seems like that
> would cover this unless you need to leave it in different states.
Okay, you are right, thank you so much. I will try it in v3.
> 
> Rob

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

* RE: [EXT] Re: [v2,2/2] dt-bindings: i2c-mux-pca954x: Add optional property i2c-mux-never-disable
  2019-10-14  3:21     ` [EXT] " Biwen Li
@ 2019-10-14  4:16       ` Biwen Li
  2019-10-14  7:06         ` Peter Rosin
  0 siblings, 1 reply; 7+ messages in thread
From: Biwen Li @ 2019-10-14  4:16 UTC (permalink / raw)
  To: Rob Herring
  Cc: peda, Leo Li, mark.rutland, linux-i2c, linux-kernel, devicetree

> 
> >
> > On Mon, Sep 30, 2019 at 11:25:03AM +0800, Biwen Li wrote:
> > > The patch adds an optional property i2c-mux-never-disable
> > >
> > > Signed-off-by: Biwen Li <biwen.li@nxp.com>
> > > ---
> > > Change in v2:
> > >       - update documentation
> > >
> > >  Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> > > b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> > > index 30ac6a60f041..71b73d0fdb62 100644
> > > --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> > > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> > > @@ -34,6 +34,7 @@ Optional Properties:
> > >      - first cell is the pin number
> > >      - second cell is used to specify flags.
> > >      See also
> > > Documentation/devicetree/bindings/interrupt-controller/interrupts.tx
> > > t
> > > +  - i2c-mux-never-disable: always forces mux to be enabled.
> >
> > Either needs to have a vendor prefix or be documented as a common
> > property.
I choose to be documented as a common property.
> >
> > IIRC, we already have a property for mux default state which seems
> > like that would cover this unless you need to leave it in different states.
> Okay, you are right, thank you so much. I will try it in v3.
Do you mean that the property is i2c-mux-idle-disconnect in Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt?
If so, the property i2c-mux-idle-disconnect is not good for me.
Because condition of the property i2c-mux-idle-disconnect is in idle state(sometimes).
But I need always enable i2c multiplexer in whatever state(anytime), so I add a common property i2c-mux-never-disable.
> >
> > Rob

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

* Re: [EXT] Re: [v2,2/2] dt-bindings: i2c-mux-pca954x: Add optional property i2c-mux-never-disable
  2019-10-14  4:16       ` Biwen Li
@ 2019-10-14  7:06         ` Peter Rosin
  2019-10-14  7:26           ` Biwen Li
  0 siblings, 1 reply; 7+ messages in thread
From: Peter Rosin @ 2019-10-14  7:06 UTC (permalink / raw)
  To: Biwen Li, Rob Herring
  Cc: Leo Li, mark.rutland, linux-i2c, linux-kernel, devicetree

On 2019-10-14 06:16, Biwen Li wrote:
>>
>>>
>>> On Mon, Sep 30, 2019 at 11:25:03AM +0800, Biwen Li wrote:
>>>> The patch adds an optional property i2c-mux-never-disable
>>>>
>>>> Signed-off-by: Biwen Li <biwen.li@nxp.com>
>>>> ---
>>>> Change in v2:
>>>>       - update documentation
>>>>
>>>>  Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>>>> b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>>>> index 30ac6a60f041..71b73d0fdb62 100644
>>>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
>>>> @@ -34,6 +34,7 @@ Optional Properties:
>>>>      - first cell is the pin number
>>>>      - second cell is used to specify flags.
>>>>      See also
>>>> Documentation/devicetree/bindings/interrupt-controller/interrupts.tx
>>>> t
>>>> +  - i2c-mux-never-disable: always forces mux to be enabled.
>>>
>>> Either needs to have a vendor prefix or be documented as a common
>>> property.
> I choose to be documented as a common property.

Can we please just drop the never-disable approach and focus on idle-state
instead?

>>>
>>> IIRC, we already have a property for mux default state which seems
>>> like that would cover this unless you need to leave it in different states.
>> Okay, you are right, thank you so much. I will try it in v3.
> Do you mean that the property is i2c-mux-idle-disconnect in Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt?
> If so, the property i2c-mux-idle-disconnect is not good for me.
> Because condition of the property i2c-mux-idle-disconnect is in idle state(sometimes).
> But I need always enable i2c multiplexer in whatever state(anytime), so I add a common property i2c-mux-never-disable.

No, I do not think any new property is needed. AFAICT, idle-state fits
perfectly, and I will not consider this i2c-mux-never-disable
approach until some compelling reason is presented why idle-state
is not appropriate. You promised to take a stab at it, and until
I hear back on that, this series is on hold. As indicated here [1].

You need to patch the driver to look at the idle-state property
instead of inventing a new (and less flexible) property. If you
implement idle-state for this driver and set the idle-state to
some channel in the dts, the mux will never disconnect. Problem solved.
Perhaps not your first solution, but it does solve your problem and
may actually be useful for other purposes than your broken hardware.
And it is consistent across other i2c-muxes. I see no downside.

Cheers,
Peter

[1] https://lore.kernel.org/lkml/07d85748-0721-39d4-d2be-13eb16b0f1de@axentia.se/

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

* RE: [EXT] Re: [v2,2/2] dt-bindings: i2c-mux-pca954x: Add optional property i2c-mux-never-disable
  2019-10-14  7:06         ` Peter Rosin
@ 2019-10-14  7:26           ` Biwen Li
  0 siblings, 0 replies; 7+ messages in thread
From: Biwen Li @ 2019-10-14  7:26 UTC (permalink / raw)
  To: Peter Rosin, Rob Herring
  Cc: Leo Li, mark.rutland, linux-i2c, linux-kernel, devicetree

> 
> On 2019-10-14 06:16, Biwen Li wrote:
> >>
> >>>
> >>> On Mon, Sep 30, 2019 at 11:25:03AM +0800, Biwen Li wrote:
> >>>> The patch adds an optional property i2c-mux-never-disable
> >>>>
> >>>> Signed-off-by: Biwen Li <biwen.li@nxp.com>
> >>>> ---
> >>>> Change in v2:
> >>>>       - update documentation
> >>>>
> >>>>  Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 1 +
> >>>>  1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git
> >>>> a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> >>>> b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> >>>> index 30ac6a60f041..71b73d0fdb62 100644
> >>>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> >>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> >>>> @@ -34,6 +34,7 @@ Optional Properties:
> >>>>      - first cell is the pin number
> >>>>      - second cell is used to specify flags.
> >>>>      See also
> >>>> Documentation/devicetree/bindings/interrupt-controller/interrupts.t
> >>>> x
> >>>> t
> >>>> +  - i2c-mux-never-disable: always forces mux to be enabled.
> >>>
> >>> Either needs to have a vendor prefix or be documented as a common
> >>> property.
> > I choose to be documented as a common property.
> 
> Can we please just drop the never-disable approach and focus on idle-state
> instead?
I will focus on idle-state property, thanks.
> 
> >>>
> >>> IIRC, we already have a property for mux default state which seems
> >>> like that would cover this unless you need to leave it in different states.
> >> Okay, you are right, thank you so much. I will try it in v3.
> > Do you mean that the property is i2c-mux-idle-disconnect in
> Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt?
> > If so, the property i2c-mux-idle-disconnect is not good for me.
> > Because condition of the property i2c-mux-idle-disconnect is in idle
> state(sometimes).
> > But I need always enable i2c multiplexer in whatever state(anytime), so I
> add a common property i2c-mux-never-disable.
> 
> No, I do not think any new property is needed. AFAICT, idle-state fits
> perfectly, and I will not consider this i2c-mux-never-disable approach until
> some compelling reason is presented why idle-state is not appropriate. You
> promised to take a stab at it, and until I hear back on that, this series is on
> hold. As indicated here [1].
> 
> You need to patch the driver to look at the idle-state property instead of
> inventing a new (and less flexible) property. If you implement idle-state for
> this driver and set the idle-state to some channel in the dts, the mux will
> never disconnect. Problem solved.
> Perhaps not your first solution, but it does solve your problem and may
> actually be useful for other purposes than your broken hardware.
> And it is consistent across other i2c-muxes. I see no downside.
Got it, thanks, I will try it.
> 
> Cheers,
> Peter
> 
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.k
> ernel.org%2Flkml%2F07d85748-0721-39d4-d2be-13eb16b0f1de%40axenti
> a.se%2F&amp;data=02%7C01%7Cbiwen.li%40nxp.com%7C4fdea02b48b94
> ed1031f08d7507509ac%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%
> 7C0%7C637066335914038253&amp;sdata=aZfxDhLPX%2FSMFGuW8ryM
> %2BcxQetFUDpdxxLa%2BuUQs7I4%3D&amp;reserved=0

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

end of thread, other threads:[~2019-10-14  7:26 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-30  3:25 [v2,1/2] i2c: pca954x: Add property to skip disabling PCA954x MUX device Biwen Li
2019-09-30  3:25 ` [v2,2/2] dt-bindings: i2c-mux-pca954x: Add optional property i2c-mux-never-disable Biwen Li
2019-10-11 14:44   ` Rob Herring
2019-10-14  3:21     ` [EXT] " Biwen Li
2019-10-14  4:16       ` Biwen Li
2019-10-14  7:06         ` Peter Rosin
2019-10-14  7:26           ` Biwen Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).