[v2,2/2] dt-bindings: i2c-mux-pca954x: Add optional property i2c-mux-never-disable
diff mbox series

Message ID 20190930032503.44425-2-biwen.li@nxp.com
State Superseded
Headers show
Series
  • [v2,1/2] i2c: pca954x: Add property to skip disabling PCA954x MUX device
Related show

Commit Message

Biwen Li Sept. 30, 2019, 3:25 a.m. UTC
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(+)

Comments

Rob Herring Oct. 11, 2019, 2:44 p.m. UTC | #1
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
Biwen Li Oct. 14, 2019, 3:21 a.m. UTC | #2
> 
> 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
Biwen Li Oct. 14, 2019, 4:16 a.m. UTC | #3
> 
> >
> > 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
Peter Rosin Oct. 14, 2019, 7:06 a.m. UTC | #4
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/
Biwen Li Oct. 14, 2019, 7:26 a.m. UTC | #5
> 
> 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

Patch
diff mbox series

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: