| Message ID | 20191014112558.3881-1-biwen.li@nxp.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/2] dt-bindings: i2c: replace property i2c-mux-idle-disconnect | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
Hi Peter, Rob This patch is a new method for https://lore.kernel.org/patchwork/patch/1132445/ Best Regards, Biwen Li > > This replaces property i2c-mux-idle-disconnect with idle-state > > Signed-off-by: Biwen Li <biwen.li@nxp.com> > --- > Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > index 30ac6a60f041..f2db517b1635 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > @@ -25,9 +25,7 @@ Required Properties: > Optional Properties: > > - reset-gpios: Reference to the GPIO connected to the reset input. > - - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect > all > - children in idle state. This is necessary for example, if there are several > - multiplexers on the bus and the devices behind them use same I2C > addresses. > + - idle-state: Please refer to > Documentation/devicetree/bindings/mux/mux-controller.txt > - interrupts: Interrupt mapping for IRQ. > - interrupt-controller: Marks the device node as an interrupt controller. > - #interrupt-cells : Should be two. > -- > 2.17.1
On 2019-10-14 13:25, Biwen Li wrote: > This replaces property i2c-mux-idle-disconnect with idle-state > > Signed-off-by: Biwen Li <biwen.li@nxp.com> > --- > Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > index 30ac6a60f041..f2db517b1635 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > @@ -25,9 +25,7 @@ Required Properties: > Optional Properties: > > - reset-gpios: Reference to the GPIO connected to the reset input. > - - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all > - children in idle state. This is necessary for example, if there are several > - multiplexers on the bus and the devices behind them use same I2C addresses. > + - idle-state: Please refer to Documentation/devicetree/bindings/mux/mux-controller.txt > - interrupts: Interrupt mapping for IRQ. > - interrupt-controller: Marks the device node as an interrupt controller. > - #interrupt-cells : Should be two. > You can't just remove i2c-mux-idle-disconnect. It needs to remain, and the driver needs to maintain support for this in case a new kernel is running with an old devicetree. Cheers, Peter
On 2019-10-14 13:25, Biwen Li wrote: > This replaces property i2c-mux-idle-disconnect with idle-state > > Signed-off-by: Biwen Li <biwen.li@nxp.com> > --- > drivers/i2c/muxes/i2c-mux-pca954x.c | 44 ++++++++++++++++++++--------- > 1 file changed, 31 insertions(+), 13 deletions(-) > > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c > index 923aa3a5a3dc..a330929c4d67 100644 > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c > @@ -86,7 +86,7 @@ struct pca954x { > > u8 last_chan; /* last register value */ > /* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */ > - s8 idle_state; > + s32 idle_state; > > struct i2c_client *client; > > @@ -256,7 +256,7 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan) > { > struct pca954x *data = i2c_mux_priv(muxc); > struct i2c_client *client = data->client; > - s8 idle_state; > + s32 idle_state; > > idle_state = READ_ONCE(data->idle_state); > if (idle_state >= 0) > @@ -411,7 +411,6 @@ static int pca954x_probe(struct i2c_client *client, > struct i2c_adapter *adap = client->adapter; > struct device *dev = &client->dev; > struct device_node *np = dev->of_node; > - bool idle_disconnect_dt; > struct gpio_desc *gpio; > struct i2c_mux_core *muxc; > struct pca954x *data; > @@ -462,22 +461,31 @@ static int pca954x_probe(struct i2c_client *client, > } > } > > + if (of_property_read_u32(np, "idle-state", &data->idle_state)) > + data->idle_state = MUX_IDLE_AS_IS; > + > /* 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 (data->idle_state >= 0){ Space before { > + data->last_chan = data->idle_state; data->last_chan should have the actual register value, i.e. (untested) if (data->chip->muxtype == pca954x_ismux) data->last_chan = data->idle_state | data->chip->enable; else data->last_chan = 1 << data->idle_state; ret = i2c_smbus_write_byte(client, data->last_chan); But since this regval calculation is now needed in three places, it should be moved to a helper function. > + /* Always enable multiplexer */ > + ret = i2c_smbus_write_byte(client, data->last_chan | > + (data->chip->muxtype == pca954x_ismux ? > + data->chip->enable : 0)); > + } > + else{ Space before { Naturally, you have the exact same issues in the pca954x_resume hunk, below. > + data->last_chan = 0; /* force the first selection */ > + /* Disconnect multiplexer */ > + ret = i2c_smbus_write_byte(client, data->last_chan); > + } > + > + if (ret < 0) { > dev_warn(dev, "probe failed\n"); > return -ENODEV; > } > > - data->last_chan = 0; /* force the first selection */ > - data->idle_state = MUX_IDLE_AS_IS; > - > - idle_disconnect_dt = np && > - of_property_read_bool(np, "i2c-mux-idle-disconnect"); > - if (idle_disconnect_dt) > - data->idle_state = MUX_IDLE_DISCONNECT; In case the idle-state property is missing, you need to fall back to the old behavior and handle i2c-mux-idle-disconnect as before. Cheers, Peter > > ret = pca954x_irq_setup(muxc); > if (ret) > @@ -531,8 +539,18 @@ 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->idle_state >= 0){ > + data->last_chan = data->idle_state; > + /* Always enable multiplexer */ > + return i2c_smbus_write_byte(client, data->last_chan | > + (data->chip->muxtype == pca954x_ismux ? > + data->chip->enable : 0)); > + } > + else{ > + data->last_chan = 0; > + /* Disconnect multiplexer */ > + return i2c_smbus_write_byte(client, data->last_chan); > + } > } > #endif > >
> > Caution: EXT Email > > On 2019-10-14 13:25, Biwen Li wrote: > > This replaces property i2c-mux-idle-disconnect with idle-state > > > > Signed-off-by: Biwen Li <biwen.li@nxp.com> > > --- > > drivers/i2c/muxes/i2c-mux-pca954x.c | 44 > > ++++++++++++++++++++--------- > > 1 file changed, 31 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c > > b/drivers/i2c/muxes/i2c-mux-pca954x.c > > index 923aa3a5a3dc..a330929c4d67 100644 > > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c > > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c > > @@ -86,7 +86,7 @@ struct pca954x { > > > > u8 last_chan; /* last register value */ > > /* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */ > > - s8 idle_state; > > + s32 idle_state; > > > > struct i2c_client *client; > > > > @@ -256,7 +256,7 @@ static int pca954x_deselect_mux(struct > > i2c_mux_core *muxc, u32 chan) { > > struct pca954x *data = i2c_mux_priv(muxc); > > struct i2c_client *client = data->client; > > - s8 idle_state; > > + s32 idle_state; > > > > idle_state = READ_ONCE(data->idle_state); > > if (idle_state >= 0) > > @@ -411,7 +411,6 @@ static int pca954x_probe(struct i2c_client *client, > > struct i2c_adapter *adap = client->adapter; > > struct device *dev = &client->dev; > > struct device_node *np = dev->of_node; > > - bool idle_disconnect_dt; > > struct gpio_desc *gpio; > > struct i2c_mux_core *muxc; > > struct pca954x *data; > > @@ -462,22 +461,31 @@ static int pca954x_probe(struct i2c_client > *client, > > } > > } > > > > + if (of_property_read_u32(np, "idle-state", &data->idle_state)) > > + data->idle_state = MUX_IDLE_AS_IS; > > + > > /* 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 (data->idle_state >= 0){ > > Space before { Okay, got it, I will add it in v2. > > > + data->last_chan = data->idle_state; > > data->last_chan should have the actual register value, i.e. (untested) Okay, got it, I will adjust it in v2. > > if (data->chip->muxtype == pca954x_ismux) > data->last_chan = data->idle_state | > data->chip->enable; > else > data->last_chan = 1 << data->idle_state; > > ret = i2c_smbus_write_byte(client, data->last_chan); > > But since this regval calculation is now needed in three places, it should be > moved to a helper function. You are right, I will add a helper function in v2. > > > + /* Always enable multiplexer */ > > + ret = i2c_smbus_write_byte(client, data->last_chan | > > + (data->chip->muxtype == > pca954x_ismux ? > > + data->chip->enable : 0)); > > + } > > + else{ > > Space before { Got it, I will add it in v2. > > Naturally, you have the exact same issues in the pca954x_resume hunk, > below. Got it > > > + data->last_chan = 0; /* force the first > selection */ > > + /* Disconnect multiplexer */ > > + ret = i2c_smbus_write_byte(client, data->last_chan); > > + } > > + > > + if (ret < 0) { > > dev_warn(dev, "probe failed\n"); > > return -ENODEV; > > } > > > > - data->last_chan = 0; /* force the first selection */ > > - data->idle_state = MUX_IDLE_AS_IS; > > - > > - idle_disconnect_dt = np && > > - of_property_read_bool(np, "i2c-mux-idle-disconnect"); > > - if (idle_disconnect_dt) > > - data->idle_state = MUX_IDLE_DISCONNECT; > > In case the idle-state property is missing, you need to fall back to the old > behavior and handle i2c-mux-idle-disconnect as before. Okay, got it, I will add it in v2. > > Cheers, > Peter > > > > > ret = pca954x_irq_setup(muxc); > > if (ret) > > @@ -531,8 +539,18 @@ 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->idle_state >= 0){ > > + data->last_chan = data->idle_state; > > + /* Always enable multiplexer */ > > + return i2c_smbus_write_byte(client, data->last_chan | > > + (data->chip->muxtype == > pca954x_ismux ? > > + data->chip->enable : 0)); > > + } > > + else{ > > + data->last_chan = 0; > > + /* Disconnect multiplexer */ > > + return i2c_smbus_write_byte(client, data->last_chan); > > + } > > } > > #endif > > > >
> Caution: EXT Email > > On 2019-10-14 13:25, Biwen Li wrote: > > This replaces property i2c-mux-idle-disconnect with idle-state > > > > Signed-off-by: Biwen Li <biwen.li@nxp.com> > > --- > > Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > > b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > > index 30ac6a60f041..f2db517b1635 100644 > > --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt > > @@ -25,9 +25,7 @@ Required Properties: > > Optional Properties: > > > > - reset-gpios: Reference to the GPIO connected to the reset input. > > - - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect > all > > - children in idle state. This is necessary for example, if there are several > > - multiplexers on the bus and the devices behind them use same I2C > addresses. > > + - idle-state: Please refer to > > + Documentation/devicetree/bindings/mux/mux-controller.txt > > - interrupts: Interrupt mapping for IRQ. > > - interrupt-controller: Marks the device node as an interrupt controller. > > - #interrupt-cells : Should be two. > > > > You can't just remove i2c-mux-idle-disconnect. It needs to remain, and the > driver needs to maintain support for this in case a new kernel is running > with an old devicetree. You are right, I will add it in v2. > > Cheers, > Peter
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt index 30ac6a60f041..f2db517b1635 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt @@ -25,9 +25,7 @@ Required Properties: Optional Properties: - reset-gpios: Reference to the GPIO connected to the reset input. - - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all - children in idle state. This is necessary for example, if there are several - multiplexers on the bus and the devices behind them use same I2C addresses. + - idle-state: Please refer to Documentation/devicetree/bindings/mux/mux-controller.txt - interrupts: Interrupt mapping for IRQ. - interrupt-controller: Marks the device node as an interrupt controller. - #interrupt-cells : Should be two.
This replaces property i2c-mux-idle-disconnect with idle-state Signed-off-by: Biwen Li <biwen.li@nxp.com> --- Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)