Hi Adam, > > From: Hubert Streidl <hubert.streidl@xxxxxxxxxxxx> > > > > By default the PMIC DA9063 2-wire interface is SMBus compliant. This > > means the PMIC will automatically reset the interface when the clock > > signal ceases for more than the SMBus timeout of 35 ms. > > > > If the I2C driver / device is not capable of creating atomic I2C > > transactions, a context change can cause a ceasing of the the clock > > signal. This can happen if for example a real-time thread is scheduled. > > Then the DA9063 in SMBus mode will reset the 2-wire interface. > > Subsequently a write message could end up in the wrong register. This > > could cause unpredictable system behavior. > > > > The DA9063 PMIC also supports an I2C compliant mode for the 2-wire > > interface. This mode does not reset the interface when the clock > > signal ceases. Thus the problem depicted above does not occur. > > > > This patch makes the I2C mode configurable by device tree. The SMBus > > compliant mode is kept as the default. > > Could we not just check the bus' functionality flags and set this accordingly? > Something like this is already done in regmap-i2c to determine how to access > the device: > > https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap-i2c.c#L309 > > This seems cleaner than a new DT property, or will this not work in this > situation? Thank you for the proposal. We were not aware of this possibility. We experienced the SMBus timeout problem on a i.MX6 Solo. When looking at the i2c-imx driver we see that it defines a list of functionality. return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_READ_BLOCK_DATA; https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-imx.c#L1133 None of that seems to model the inability to perform atomic transactions within the SMBus timeout. This is either a bug of this specific driver or maybe the expressiveness of I2C_FUNC_* is not sufficient. https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/i2c.h#L88 What flag do you think we could check to find out whether the bus is able to obey the SMBus timeout or not? > > Signed-off-by: Hubert Streidl <hubert.streidl@xxxxxxxxxxxx> > > Signed-off-by: Mark Jonas <mark.jonas@xxxxxxxxxxxx> > > --- > > Documentation/devicetree/bindings/mfd/da9063.txt | 7 +++++++ > > drivers/mfd/da9063-core.c | 9 +++++++++ > > drivers/mfd/da9063-i2c.c | 13 +++++++++++++ > > include/linux/mfd/da9063/core.h | 1 + > > include/linux/mfd/da9063/registers.h | 3 +++ > > 5 files changed, 33 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/mfd/da9063.txt > > b/Documentation/devicetree/bindings/mfd/da9063.txt > > index 8da879935c59..256f2a25fe0a 100644 > > --- a/Documentation/devicetree/bindings/mfd/da9063.txt > > +++ b/Documentation/devicetree/bindings/mfd/da9063.txt > > @@ -19,6 +19,12 @@ Required properties: > > - interrupts : IRQ line information. > > - interrupt-controller > > > > +Optional properties: > > + > > +- i2c-mode : Switch serial 2-wire interface into I2C mode. Without > > +this > > + property the PMIC uses the SMBus mode (resets the interface if the > > +clock > > + ceases for a longer time than the SMBus timeout). > > + > > Sub-nodes: > > > > - regulators : This node defines the settings for the LDOs and BUCKs. > > @@ -77,6 +83,7 @@ Example: > > interrupt-parent = <&gpio6>; > > interrupts = <11 IRQ_TYPE_LEVEL_LOW>; > > interrupt-controller; > > + i2c-mode; > > > > rtc { > > compatible = "dlg,da9063-rtc"; > > diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c > > index df407c3afce3..baa1e4310c8c 100644 > > --- a/drivers/mfd/da9063-core.c > > +++ b/drivers/mfd/da9063-core.c > > @@ -162,6 +162,15 @@ int da9063_device_init(struct da9063 *da9063, > > unsigned int irq) { > > int ret; > > > > + if (da9063->i2cmode) { > > + ret = regmap_update_bits(da9063->regmap, > > DA9063_REG_CONFIG_J, > > + DA9063_TWOWIRE_TO, 0); > > + if (ret < 0) { > > + dev_err(da9063->dev, "Cannot enable I2C mode.\n"); > > + return -EIO; > > + } > > + } > > + > > ret = da9063_clear_fault_log(da9063); > > if (ret < 0) > > dev_err(da9063->dev, "Cannot clear fault log\n"); diff --git > > a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c index > > 3781d0bb7786..af0bf13ab43e 100644 > > --- a/drivers/mfd/da9063-i2c.c > > +++ b/drivers/mfd/da9063-i2c.c > > @@ -351,6 +351,17 @@ static const struct of_device_id da9063_dt_ids[] = { > > { } > > }; > > MODULE_DEVICE_TABLE(of, da9063_dt_ids); > > + > > +static void da9063_i2c_parse_dt(struct i2c_client *client, struct > > +da9063 *da9063) { > > + struct device_node *np = client->dev.of_node; > > + > > + if (of_property_read_bool(np, "i2c-mode")) > > + da9063->i2cmode = true; > > + else > > + da9063->i2cmode = false; > > +} > > + > > static int da9063_i2c_probe(struct i2c_client *i2c, > > const struct i2c_device_id *id) { @@ -366,6 +377,8 > @@ static > > int da9063_i2c_probe(struct i2c_client *i2c, > > da9063->chip_irq = i2c->irq; > > da9063->type = id->driver_data; > > > > + da9063_i2c_parse_dt(i2c, da9063); > > + > > ret = da9063_get_device_type(i2c, da9063); > > if (ret) > > return ret; > > diff --git a/include/linux/mfd/da9063/core.h > > b/include/linux/mfd/da9063/core.h index fa7a43f02f27..866864c50f78 > > 100644 > > --- a/include/linux/mfd/da9063/core.h > > +++ b/include/linux/mfd/da9063/core.h > > @@ -77,6 +77,7 @@ struct da9063 { > > enum da9063_type type; > > unsigned char variant_code; > > unsigned int flags; > > + bool i2cmode; > > > > /* Control interface */ > > struct regmap *regmap; > > diff --git a/include/linux/mfd/da9063/registers.h > > b/include/linux/mfd/da9063/registers.h > > index 1dbabf1b3cb8..6e0f66a2e727 100644 > > --- a/include/linux/mfd/da9063/registers.h > > +++ b/include/linux/mfd/da9063/registers.h > > @@ -1037,6 +1037,9 @@ > > #define DA9063_NONKEY_PIN_AUTODOWN 0x02 > > #define DA9063_NONKEY_PIN_AUTOFLPRT 0x03 > > > > +/* DA9063_REG_CONFIG_J (addr=0x10F) */ > > +#define DA9063_TWOWIRE_TO 0x40 > > + > > /* DA9063_REG_MON_REG_5 (addr=0x116) */ > > #define DA9063_MON_A8_IDX_MASK 0x07 > > #define DA9063_MON_A8_IDX_NONE 0x00 > > -- > > 2.25.1 Cheers, Mark
