| Message ID | 2adcd797-f3cc-c8c7-c18c-d3726b2db4c0@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: gpio: support write-only sda | expand |
Hi Heiner, > /* read ack: SDA should be pulled down by slave, or it may > * NAK (usually to report problems with the data we wrote). > + * Report ACK if SDA is write-only. Minor nit: On first read, I didn't understand. "Always report ACK..." is maybe a tad clearer. > */ > @@ -203,6 +204,9 @@ static int i2c_inb(struct i2c_adapter *i2c_adap) > unsigned char indata = 0; > struct i2c_algo_bit_data *adap = i2c_adap->algo_data; > > + if (!adap->getsda) > + return -EOPNOTSUPP; Wouldn't it be better in 'readbytes' returning an errno there? > - /* Complain if SCL can't be read */ > - if (bit_adap->getscl == NULL) { > + if (bit_adap->getscl == NULL && bit_adap->getsda == NULL) > + dev_info(&adap->dev, "I2C-like interface, SDA and SCL are write-only\n"); > + else if (bit_adap->getscl == NULL) { > + /* Complain if SCL can't be read */ > dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n"); > dev_warn(&adap->dev, "Bus may be unreliable\n"); Hmm, this is a bit inconsistent with dev_warn and dev_info. How about this? if (bit_adap->getscl == NULL) dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n"); if (bit_adap->getsda == NULL) dev_warn(&adap->dev, "Not I2C compliant: can't read SDA\n"); if (bit_adap->getscl == NULL || bit_adap->getsda == NULL) dev_warn(&adap->dev, "Bus may be unreliable\n"); The above code can surely be simplified. I just wanted to show this simple approach so we can discuss my suggestion. All the best, Wolfram
On 16.05.2022 21:31, Wolfram Sang wrote: > Hi Heiner, > Hi Wolfram, sorry for answering quite late .. >> /* read ack: SDA should be pulled down by slave, or it may >> * NAK (usually to report problems with the data we wrote). >> + * Report ACK if SDA is write-only. > > Minor nit: On first read, I didn't understand. "Always report ACK..." is > maybe a tad clearer. > OK >> */ >> @@ -203,6 +204,9 @@ static int i2c_inb(struct i2c_adapter *i2c_adap) >> unsigned char indata = 0; >> struct i2c_algo_bit_data *adap = i2c_adap->algo_data; >> >> + if (!adap->getsda) >> + return -EOPNOTSUPP; > > Wouldn't it be better in 'readbytes' returning an errno there? > I think that's something we can do in addition. We have other users of i2c_inb() than readbytes() (in i2c_algo_pcf), therefore I'd prefer to let i2c_inb() return an error instead of relying on upper layers only. >> - /* Complain if SCL can't be read */ >> - if (bit_adap->getscl == NULL) { >> + if (bit_adap->getscl == NULL && bit_adap->getsda == NULL) >> + dev_info(&adap->dev, "I2C-like interface, SDA and SCL are write-only\n"); >> + else if (bit_adap->getscl == NULL) { >> + /* Complain if SCL can't be read */ >> dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n"); >> dev_warn(&adap->dev, "Bus may be unreliable\n"); > > Hmm, this is a bit inconsistent with dev_warn and dev_info. How about > this? > Right, it would be a bit inconsistent. My thought was: If both getscl and getsda are NULL, then the driver is intentionally used this way and it reflects the design of the respective system. It's expected that both are NULL and there's nothing wrong with it. At least to me a warning means: Something isn't ok and requires an action. However I could also understand the point of view that everything not being really I2C-compliant should trigger a warning. I'm fine with both options, please advise. > if (bit_adap->getscl == NULL) > dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n"); > > if (bit_adap->getsda == NULL) > dev_warn(&adap->dev, "Not I2C compliant: can't read SDA\n"); > > if (bit_adap->getscl == NULL || bit_adap->getsda == NULL) > dev_warn(&adap->dev, "Bus may be unreliable\n"); > > The above code can surely be simplified. I just wanted to show this > simple approach so we can discuss my suggestion. > > All the best, > > Wolfram >
On 14.08.2022 23:06, Heiner Kallweit wrote: > On 16.05.2022 21:31, Wolfram Sang wrote: >> Hi Heiner, >> > Hi Wolfram, > > sorry for answering quite late .. > >>> /* read ack: SDA should be pulled down by slave, or it may >>> * NAK (usually to report problems with the data we wrote). >>> + * Report ACK if SDA is write-only. >> >> Minor nit: On first read, I didn't understand. "Always report ACK..." is >> maybe a tad clearer. >> > > OK > >>> */ >>> @@ -203,6 +204,9 @@ static int i2c_inb(struct i2c_adapter *i2c_adap) >>> unsigned char indata = 0; >>> struct i2c_algo_bit_data *adap = i2c_adap->algo_data; >>> >>> + if (!adap->getsda) >>> + return -EOPNOTSUPP; >> >> Wouldn't it be better in 'readbytes' returning an errno there? >> > > I think that's something we can do in addition. We have other users of i2c_inb() > than readbytes() (in i2c_algo_pcf), therefore I'd prefer to let i2c_inb() > return an error instead of relying on upper layers only. > >>> - /* Complain if SCL can't be read */ >>> - if (bit_adap->getscl == NULL) { >>> + if (bit_adap->getscl == NULL && bit_adap->getsda == NULL) >>> + dev_info(&adap->dev, "I2C-like interface, SDA and SCL are write-only\n"); >>> + else if (bit_adap->getscl == NULL) { >>> + /* Complain if SCL can't be read */ >>> dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n"); >>> dev_warn(&adap->dev, "Bus may be unreliable\n"); >> >> Hmm, this is a bit inconsistent with dev_warn and dev_info. How about >> this? >> > Right, it would be a bit inconsistent. My thought was: > If both getscl and getsda are NULL, then the driver is intentionally used this way > and it reflects the design of the respective system. > It's expected that both are NULL and there's nothing wrong with it. > At least to me a warning means: Something isn't ok and requires an action. > > However I could also understand the point of view that everything not being really > I2C-compliant should trigger a warning. > > I'm fine with both options, please advise. > >> if (bit_adap->getscl == NULL) >> dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n"); >> >> if (bit_adap->getsda == NULL) >> dev_warn(&adap->dev, "Not I2C compliant: can't read SDA\n"); >> >> if (bit_adap->getscl == NULL || bit_adap->getsda == NULL) >> dev_warn(&adap->dev, "Bus may be unreliable\n"); >> >> The above code can surely be simplified. I just wanted to show this >> simple approach so we can discuss my suggestion. >> >> All the best, >> >> Wolfram >> > Just stumbled across this open discussion. Could you please have a look at my feedback to your review comments? Thanks
diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c index fc90293af..ab5a73e90 100644 --- a/drivers/i2c/algos/i2c-algo-bit.c +++ b/drivers/i2c/algos/i2c-algo-bit.c @@ -184,8 +184,9 @@ static int i2c_outb(struct i2c_adapter *i2c_adap, unsigned char c) /* read ack: SDA should be pulled down by slave, or it may * NAK (usually to report problems with the data we wrote). + * Report ACK if SDA is write-only. */ - ack = !getsda(adap); /* ack: sda is pulled low -> success */ + ack = !adap->getsda || !getsda(adap); /* ack: sda is pulled low -> success */ bit_dbg(2, &i2c_adap->dev, "i2c_outb: 0x%02x %s\n", (int)c, ack ? "A" : "NA"); @@ -203,6 +204,9 @@ static int i2c_inb(struct i2c_adapter *i2c_adap) unsigned char indata = 0; struct i2c_algo_bit_data *adap = i2c_adap->algo_data; + if (!adap->getsda) + return -EOPNOTSUPP; + /* assert: scl is low */ sdahi(adap); for (i = 0; i < 8; i++) { @@ -232,6 +236,10 @@ static int test_bus(struct i2c_adapter *i2c_adap) const char *name = i2c_adap->name; int scl, sda, ret; + /* Testing not possible if both pins are write-only. */ + if (adap->getscl == NULL && adap->getsda == NULL) + return 0; + if (adap->pre_xfer) { ret = adap->pre_xfer(i2c_adap); if (ret < 0) @@ -670,8 +678,10 @@ static int __i2c_bit_add_bus(struct i2c_adapter *adap, if (ret < 0) return ret; - /* Complain if SCL can't be read */ - if (bit_adap->getscl == NULL) { + if (bit_adap->getscl == NULL && bit_adap->getsda == NULL) + dev_info(&adap->dev, "I2C-like interface, SDA and SCL are write-only\n"); + else if (bit_adap->getscl == NULL) { + /* Complain if SCL can't be read */ dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n"); dev_warn(&adap->dev, "Bus may be unreliable\n"); }
This is in preparation of supporting write-only SDA in i2c-gpio. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/i2c/algos/i2c-algo-bit.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)