| Message ID | 20230830115744.4102929-2-Naresh.Solanki@9elements.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] dt-binding: Add custom property for MAX7357 | expand |
Hi Naresh, On Wed, Aug 30, 2023 at 01:57:43PM +0200, Naresh Solanki wrote: > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > Detect that max7357 is being used and run custom init sequence. > > By default MAX7357 disconnects all channels on a bus lock-up and > signals this condition to the bus master using an interrupt. please replace this tab with a space. > Disable the interrupt as it's not useful within the kernel and > it might conflict with the reset functionality that shares the same > pin. > > Use the introduced 'maxim,bus-lockup-fix' property to enable > faulty channel isolation and flush-out sequence generation. > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> > --- > drivers/i2c/muxes/i2c-mux-pca954x.c | 56 ++++++++++++++++++++++++++++- > 1 file changed, 55 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c > index 2219062104fb..0c1ff1438e7c 100644 > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c > @@ -57,6 +57,21 @@ > > #define PCA954X_IRQ_OFFSET 4 > > +/* > + * MAX7357 exposes 7 registers on POR which allow to configure additional > + * features. The configuration register holds the following settings: > + */ > +#define MAX7357_CONF_INT_ENABLE BIT(0) > +#define MAX7357_CONF_FLUSH_OUT BIT(1) > +#define MAX7357_CONF_RELEASE_INT BIT(2) > +#define MAX7357_CONF_LOCK_UP_CLEAR BIT(3) > +#define MAX7357_CONF_DISCON_SINGLE_CHAN BIT(4) > +#define MAX7357_CONF_BUS_LOCKUP_DETECT_DIS BIT(5) > +#define MAX7357_CONF_ENABLE_BASIC_MODE BIT(6) > +#define MAX7357_CONF_PRECONNECT_TEST BIT(7) Not all these defines are are used, can we remove those that we don't need? > +#define MAX7357_POR_DEFAULT_CONF BIT(0) I think: #define MAX7357_POR_DEFAULT_CONF MAX7357_CONF_INT_ENABLE has a better meaning... but overall, do we need it? > + > enum pca_type { > max_7356, > max_7357, > @@ -477,6 +492,41 @@ static int pca954x_init(struct i2c_client *client, struct pca954x *data) > return ret; > } > > +static int max7357_init(struct i2c_client *client, struct pca954x *data) > +{ > + struct i2c_adapter *adap = client->adapter; > + u8 conf = MAX7357_POR_DEFAULT_CONF; > + int ret; > + > + if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) > + return pca954x_init(client, data); > + > + if (data->idle_state >= 0) > + data->last_chan = pca954x_regval(data, data->idle_state); > + else > + data->last_chan = 0; /* Disconnect multiplexer */ > + > + /* > + * The interrupt signals downstream channels that are stuck, but > + * there's nothing to do and it prevents using the shared pin as reset. > + */ > + conf &= MAX7357_CONF_INT_ENABLE; > + > + /* > + * On bus lock-up isolate the failing channel and try to clear the > + * fault by sending the flush-out sequence. > + */ > + if (device_property_read_bool(&client->dev, "maxim,bus-lockup-fix")) > + conf |= MAX7357_CONF_DISCON_SINGLE_CHAN | > + MAX7357_CONF_FLUSH_OUT; this function is identical to pca954x_init() except for the conf. If you: u8 conf = 0; ... if (i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) { conf &= MAX7357_CONF_INT_ENABLE; if (device_property_read_bool(&client->dev, "maxim,bus-lockup-fix")) conf |= MAX7357_CONF_DISCON_SINGLE_CHAN | MAX7357_CONF_FLUSH_OUT; } ret = i2c_smbus_write_byte_data(client, data->last_chan, conf); ... You basically should obtain the same thing, I guess and we make things easier. > + ret = i2c_smbus_write_byte_data(client, data->last_chan, conf); > + if (ret < 0) > + data->last_chan = 0; > + return ret; > +} > + > /* > * I2C init/probing/exit functions > */ > @@ -560,7 +610,11 @@ static int pca954x_probe(struct i2c_client *client) > * initializes the mux to a channel > * or disconnected state. > */ > - ret = pca954x_init(client, data); > + if ((dev->of_node && of_device_is_compatible(dev->of_node, "maxim,max7357")) || > + id->driver_data == max_7357) > + ret = max7357_init(client, data); what happens if this is true and in max7357_init(); the i2c functionality check fails? Which of the two if's is redundant? Should they be merged? Andi > + else > + ret = pca954x_init(client, data); > if (ret < 0) { > dev_warn(dev, "probe failed\n"); > ret = -ENODEV; > -- > 2.41.0 >
Hi Andy, On Sun, 3 Sept 2023 at 00:13, Andi Shyti <andi.shyti@kernel.org> wrote: > > Hi Naresh, > > On Wed, Aug 30, 2023 at 01:57:43PM +0200, Naresh Solanki wrote: > > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > > > Detect that max7357 is being used and run custom init sequence. > > > > By default MAX7357 disconnects all channels on a bus lock-up and > > signals this condition to the bus master using an interrupt. > > please replace this tab with a space. Ack > > > Disable the interrupt as it's not useful within the kernel and > > it might conflict with the reset functionality that shares the same > > pin. > > > > Use the introduced 'maxim,bus-lockup-fix' property to enable > > faulty channel isolation and flush-out sequence generation. > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> > > --- > > drivers/i2c/muxes/i2c-mux-pca954x.c | 56 ++++++++++++++++++++++++++++- > > 1 file changed, 55 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c > > index 2219062104fb..0c1ff1438e7c 100644 > > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c > > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c > > @@ -57,6 +57,21 @@ > > > > #define PCA954X_IRQ_OFFSET 4 > > > > +/* > > + * MAX7357 exposes 7 registers on POR which allow to configure additional > > + * features. The configuration register holds the following settings: > > + */ > > +#define MAX7357_CONF_INT_ENABLE BIT(0) > > +#define MAX7357_CONF_FLUSH_OUT BIT(1) > > +#define MAX7357_CONF_RELEASE_INT BIT(2) > > +#define MAX7357_CONF_LOCK_UP_CLEAR BIT(3) > > +#define MAX7357_CONF_DISCON_SINGLE_CHAN BIT(4) > > +#define MAX7357_CONF_BUS_LOCKUP_DETECT_DIS BIT(5) > > +#define MAX7357_CONF_ENABLE_BASIC_MODE BIT(6) > > +#define MAX7357_CONF_PRECONNECT_TEST BIT(7) > > Not all these defines are are used, can we remove those that we > don't need? Ack. Will keep the ones that are used. > > > +#define MAX7357_POR_DEFAULT_CONF BIT(0) > > I think: > > #define MAX7357_POR_DEFAULT_CONF MAX7357_CONF_INT_ENABLE > > has a better meaning... but overall, do we need it? Ack. Will keep the ones that are in use. > > > + > > enum pca_type { > > max_7356, > > max_7357, > > @@ -477,6 +492,41 @@ static int pca954x_init(struct i2c_client *client, struct pca954x *data) > > return ret; > > } > > > > +static int max7357_init(struct i2c_client *client, struct pca954x *data) > > +{ > > + struct i2c_adapter *adap = client->adapter; > > + u8 conf = MAX7357_POR_DEFAULT_CONF; > > + int ret; > > + > > + if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) > > + return pca954x_init(client, data); > > + > > + if (data->idle_state >= 0) > > + data->last_chan = pca954x_regval(data, data->idle_state); > > + else > > + data->last_chan = 0; /* Disconnect multiplexer */ > > + > > + /* > > + * The interrupt signals downstream channels that are stuck, but > > + * there's nothing to do and it prevents using the shared pin as reset. > > + */ > > + conf &= MAX7357_CONF_INT_ENABLE; > > + > > + /* > > + * On bus lock-up isolate the failing channel and try to clear the > > + * fault by sending the flush-out sequence. > > + */ > > + if (device_property_read_bool(&client->dev, "maxim,bus-lockup-fix")) > > + conf |= MAX7357_CONF_DISCON_SINGLE_CHAN | > > + MAX7357_CONF_FLUSH_OUT; > > this function is identical to pca954x_init() except for the > conf. > > If you: > > u8 conf = 0; > > ... > > if (i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) { > conf &= MAX7357_CONF_INT_ENABLE; > > if (device_property_read_bool(&client->dev, > "maxim,bus-lockup-fix")) > conf |= MAX7357_CONF_DISCON_SINGLE_CHAN | > MAX7357_CONF_FLUSH_OUT; > } > > ret = i2c_smbus_write_byte_data(client, data->last_chan, conf); > ... > > > You basically should obtain the same thing, I guess and we make > things easier. Ack. Will do the changes as suggested. Also based on feedback from Krzysztof, it was suggested that the dt property might not be needed & the settings can be configured/enabled by default. So will remove the DT property check ? Ref: https://lore.kernel.org/lkml/9100e41b-291e-9723-4188-b4d3e5adb6f8@linaro.org/#t Regards, Naresh > > > > + ret = i2c_smbus_write_byte_data(client, data->last_chan, conf); > > + if (ret < 0) > > + data->last_chan = 0; > > + return ret; > > +} > > + > > /* > > * I2C init/probing/exit functions > > */ > > @@ -560,7 +610,11 @@ static int pca954x_probe(struct i2c_client *client) > > * initializes the mux to a channel > > * or disconnected state. > > */ > > - ret = pca954x_init(client, data); > > + if ((dev->of_node && of_device_is_compatible(dev->of_node, "maxim,max7357")) || > > + id->driver_data == max_7357) > > + ret = max7357_init(client, data); > > what happens if this is true and in max7357_init(); the i2c > functionality check fails? > > Which of the two if's is redundant? Should they be merged? > > Andi > > > + else > > + ret = pca954x_init(client, data); > > if (ret < 0) { > > dev_warn(dev, "probe failed\n"); > > ret = -ENODEV; > > -- > > 2.41.0 > >
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c index 2219062104fb..0c1ff1438e7c 100644 --- a/drivers/i2c/muxes/i2c-mux-pca954x.c +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c @@ -57,6 +57,21 @@ #define PCA954X_IRQ_OFFSET 4 +/* + * MAX7357 exposes 7 registers on POR which allow to configure additional + * features. The configuration register holds the following settings: + */ +#define MAX7357_CONF_INT_ENABLE BIT(0) +#define MAX7357_CONF_FLUSH_OUT BIT(1) +#define MAX7357_CONF_RELEASE_INT BIT(2) +#define MAX7357_CONF_LOCK_UP_CLEAR BIT(3) +#define MAX7357_CONF_DISCON_SINGLE_CHAN BIT(4) +#define MAX7357_CONF_BUS_LOCKUP_DETECT_DIS BIT(5) +#define MAX7357_CONF_ENABLE_BASIC_MODE BIT(6) +#define MAX7357_CONF_PRECONNECT_TEST BIT(7) + +#define MAX7357_POR_DEFAULT_CONF BIT(0) + enum pca_type { max_7356, max_7357, @@ -477,6 +492,41 @@ static int pca954x_init(struct i2c_client *client, struct pca954x *data) return ret; } +static int max7357_init(struct i2c_client *client, struct pca954x *data) +{ + struct i2c_adapter *adap = client->adapter; + u8 conf = MAX7357_POR_DEFAULT_CONF; + int ret; + + if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) + return pca954x_init(client, data); + + if (data->idle_state >= 0) + data->last_chan = pca954x_regval(data, data->idle_state); + else + data->last_chan = 0; /* Disconnect multiplexer */ + + /* + * The interrupt signals downstream channels that are stuck, but + * there's nothing to do and it prevents using the shared pin as reset. + */ + conf &= MAX7357_CONF_INT_ENABLE; + + /* + * On bus lock-up isolate the failing channel and try to clear the + * fault by sending the flush-out sequence. + */ + if (device_property_read_bool(&client->dev, "maxim,bus-lockup-fix")) + conf |= MAX7357_CONF_DISCON_SINGLE_CHAN | + MAX7357_CONF_FLUSH_OUT; + + ret = i2c_smbus_write_byte_data(client, data->last_chan, conf); + if (ret < 0) + data->last_chan = 0; + + return ret; +} + /* * I2C init/probing/exit functions */ @@ -560,7 +610,11 @@ static int pca954x_probe(struct i2c_client *client) * initializes the mux to a channel * or disconnected state. */ - ret = pca954x_init(client, data); + if ((dev->of_node && of_device_is_compatible(dev->of_node, "maxim,max7357")) || + id->driver_data == max_7357) + ret = max7357_init(client, data); + else + ret = pca954x_init(client, data); if (ret < 0) { dev_warn(dev, "probe failed\n"); ret = -ENODEV;