lkml.org 
[lkml]   [2019]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRE: [EXT] Re: [PATCH] i2c: pca954x: Add property to skip disabling PCA954x MUX device
    Date
    > 
    > On 2019-09-29 12:36, Biwen Li wrote:
    > > 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.
    >
    > Hi!
    >
    > Are you saying that the parent bus of the mux is relying on some pull-ups inside
    > the mux?
    Yes, as follows:
    VCC
    -------
    |-------------------
    | |
    \ \
    /10K resister / 10k resister
    \ \
    | |
    | |
    I2C1_SCL ------------------------ I2C1_SCL | ----------------------
    --------------------|SCL | ----------------------------------------|SCL |
    I2C1_SDA | PCA9547 | I2C1_SDA | | PCA9547 |
    --------------------|SDA | ----------------------------|-----------|SDA |
    ------------------------ ----------------------
    --wrong design(need software fix as above or hardware fix)-- --proper design--
    >
    > > 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 (Board revision
    > > found on Rev.B, Rev.C and Rev.D)
    >
    > I think you should follow the example of the i2c-mux-gpio driver and implement
    > the idle-state property instead.
    >
    > That is a lot more consistent, assuming it solves the problem at hand?
    Got it, thanks, I will try it.
    >
    > >
    > > Signed-off-by: Biwen Li <biwen.li@nxp.com>
    > > ---
    > > drivers/i2c/muxes/i2c-mux-pca954x.c | 33
    > > +++++++++++++++++++++++++----
    > > 1 file changed, 29 insertions(+), 4 deletions(-)
    > >
    > > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
    > > b/drivers/i2c/muxes/i2c-mux-pca954x.c
    > > index 923aa3a5a3dc..ea8aca54d572 100644
    > > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
    > > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
    > > @@ -93,6 +93,7 @@ struct pca954x {
    > > struct irq_domain *irq;
    > > unsigned int irq_mask;
    > > raw_spinlock_t lock;
    > > + u8 disable_mux; /* do not disable mux if val not 0 */
    >
    > Awful number of negations there. The name is also backwards given that a
    > non-zero value means that the mux should *not* be disabled. I would have
    > reused the name from the binding.
    >
    > bool never_disable;
    >
    > A bit less confusing...
    Got it,thanks, I will let it clear in v2.
    >
    > > };
    > >
    > > /* Provide specs for the PCA954x types we know about */ @@ -258,6
    > > +259,11 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc,
    > u32 chan)
    > > struct i2c_client *client = data->client;
    > > s8 idle_state;
    > >
    > > + if (data->disable_mux != 0) {
    >
    > Please drop " != 0" and use the variable as a truth value. More instances below...
    Got it, I will correct it in v2.
    >
    > > + data->last_chan = data->chip->nchans;
    > > + return pca954x_reg_write(muxc->parent, client,
    > data->disable_mux);
    > > + }
    > > +
    > > idle_state = READ_ONCE(data->idle_state);
    > > if (idle_state >= 0)
    > > /* Set the mux back to a predetermined channel */ @@
    > > -462,16 +468,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->disable_mux = np &&
    > > + of_property_read_bool(np, "i2c-mux-never-disable") &&
    >
    > i2c-mux-never-disable needs to be documented. However see above for my
    > point that you should do it like the i2c-mux-gpio driver. Any usage of idle-state
    > still needs to be documented for pca954x binding.
    Got it, I will update the document in v2.
    >
    > > + data->chip->muxtype == pca954x_ismux ?
    > > + data->chip->enable : 0;
    >
    > Why do you not allow never-disable for switches?
    Currently, the hardware bug is on pca9547(LS2085ARDB and LS2088ARDB not populate resistors for pca9547),
    muxtype of pca9547 is 'pca954x_ismux'
    >
    > Cheers,
    > Peter
    >
    > > +
    > > /* 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->disable_mux) < 0) {
    > > dev_warn(dev, "probe failed\n");
    > > return -ENODEV;
    > > }
    > >
    > > - data->last_chan = 0; /* force the first selection */
    > > + if (data->disable_mux != 0)
    > > + 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 +553,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->disable_mux != 0)
    > > + data->last_chan = data->chip->nchans;
    > > + else
    > > + data->last_chan = 0;
    > > + return i2c_smbus_write_byte(client, data->disable_mux);
    > > }
    > > #endif
    > >
    > >

    \
     
     \ /
      Last update: 2019-09-30 04:44    [W:9.289 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site