The vendor BSP used for the WM8350 development provided an I2C driver
which incorrectly returned zero on succesful sends rather than the
number of transmitted bytes, an error which was then propagated into the
WM8350 I2C accessors.
Signed-off-by: Mark Brown <[email protected]>
---
drivers/mfd/wm8350-i2c.c | 15 +++++++++++++--
1 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/mfd/wm8350-i2c.c b/drivers/mfd/wm8350-i2c.c
index 8dfe21b..3e0ce0e 100644
--- a/drivers/mfd/wm8350-i2c.c
+++ b/drivers/mfd/wm8350-i2c.c
@@ -30,7 +30,12 @@ static int wm8350_i2c_read_device(struct wm8350 *wm8350, char reg,
ret = i2c_master_send(wm8350->i2c_client, ®, 1);
if (ret < 0)
return ret;
- return i2c_master_recv(wm8350->i2c_client, dest, bytes);
+ ret = i2c_master_recv(wm8350->i2c_client, dest, bytes);
+ if (ret < 0)
+ return ret;
+ if (ret != bytes)
+ return -EIO;
+ return 0;
}
static int wm8350_i2c_write_device(struct wm8350 *wm8350, char reg,
@@ -38,13 +43,19 @@ static int wm8350_i2c_write_device(struct wm8350 *wm8350, char reg,
{
/* we add 1 byte for device register */
u8 msg[(WM8350_MAX_REGISTER << 1) + 1];
+ int ret;
if (bytes > ((WM8350_MAX_REGISTER << 1) + 1))
return -EINVAL;
msg[0] = reg;
memcpy(&msg[1], src, bytes);
- return i2c_master_send(wm8350->i2c_client, msg, bytes + 1);
+ ret = i2c_master_send(wm8350->i2c_client, msg, bytes + 1);
+ if (ret < 0)
+ return ret;
+ if (ret != bytes + 1)
+ return -EIO;
+ return 0;
}
static int wm8350_i2c_probe(struct i2c_client *i2c,
--
1.5.6.5
Hi Mark,
On Mon, Nov 10, 2008 at 01:41:17PM +0000, Mark Brown wrote:
> The vendor BSP used for the WM8350 development provided an I2C driver
> which incorrectly returned zero on succesful sends rather than the
> number of transmitted bytes, an error which was then propagated into the
> WM8350 I2C accessors.
Shouldnt we fix the accessors behaviour instead ?
Currently, that would mean fixing some of the wm8350-core static functions.
Slightly bigger patch, but that would keep the i2c interface consistent.
What do you think ?
Cheers,
Samuel.
> Signed-off-by: Mark Brown <[email protected]>
> ---
> drivers/mfd/wm8350-i2c.c | 15 +++++++++++++--
> 1 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mfd/wm8350-i2c.c b/drivers/mfd/wm8350-i2c.c
> index 8dfe21b..3e0ce0e 100644
> --- a/drivers/mfd/wm8350-i2c.c
> +++ b/drivers/mfd/wm8350-i2c.c
> @@ -30,7 +30,12 @@ static int wm8350_i2c_read_device(struct wm8350 *wm8350, char reg,
> ret = i2c_master_send(wm8350->i2c_client, ®, 1);
> if (ret < 0)
> return ret;
> - return i2c_master_recv(wm8350->i2c_client, dest, bytes);
> + ret = i2c_master_recv(wm8350->i2c_client, dest, bytes);
> + if (ret < 0)
> + return ret;
> + if (ret != bytes)
> + return -EIO;
> + return 0;
> }
>
> static int wm8350_i2c_write_device(struct wm8350 *wm8350, char reg,
> @@ -38,13 +43,19 @@ static int wm8350_i2c_write_device(struct wm8350 *wm8350, char reg,
> {
> /* we add 1 byte for device register */
> u8 msg[(WM8350_MAX_REGISTER << 1) + 1];
> + int ret;
>
> if (bytes > ((WM8350_MAX_REGISTER << 1) + 1))
> return -EINVAL;
>
> msg[0] = reg;
> memcpy(&msg[1], src, bytes);
> - return i2c_master_send(wm8350->i2c_client, msg, bytes + 1);
> + ret = i2c_master_send(wm8350->i2c_client, msg, bytes + 1);
> + if (ret < 0)
> + return ret;
> + if (ret != bytes + 1)
> + return -EIO;
> + return 0;
> }
>
> static int wm8350_i2c_probe(struct i2c_client *i2c,
> --
> 1.5.6.5
>
--
Intel Open Source Technology Centre
http://oss.intel.com/
On Wed, Nov 12, 2008 at 07:49:57PM +0100, Samuel Ortiz wrote:
> On Mon, Nov 10, 2008 at 01:41:17PM +0000, Mark Brown wrote:
> > The vendor BSP used for the WM8350 development provided an I2C driver
> > which incorrectly returned zero on succesful sends rather than the
> > number of transmitted bytes, an error which was then propagated into the
> > WM8350 I2C accessors.
> Shouldnt we fix the accessors behaviour instead ?
> Currently, that would mean fixing some of the wm8350-core static functions.
> Slightly bigger patch, but that would keep the i2c interface consistent.
I don't really understand what you mean by "keep the i2c interface
consistent" here? The purpose of this abstraction is to abstract away
the control interface used to communicate with the chip since it
supports both I2C and SPI.
> What do you think ?
That would expose the details of the I2C API and the wire format used to
access the device over I2C to the WM8350 core which would mean the SPI
control code would have to jump through hoops to emulate it. If the
device only had I2C control it would be better to just remove this
abstraction entirely and use the I2C API directly in the register access
functions.
On Wed, Nov 12, 2008 at 08:00:07PM +0000, Mark Brown wrote:
> On Wed, Nov 12, 2008 at 07:49:57PM +0100, Samuel Ortiz wrote:
> > On Mon, Nov 10, 2008 at 01:41:17PM +0000, Mark Brown wrote:
>
> > > The vendor BSP used for the WM8350 development provided an I2C driver
> > > which incorrectly returned zero on succesful sends rather than the
> > > number of transmitted bytes, an error which was then propagated into the
> > > WM8350 I2C accessors.
>
> > Shouldnt we fix the accessors behaviour instead ?
> > Currently, that would mean fixing some of the wm8350-core static functions.
> > Slightly bigger patch, but that would keep the i2c interface consistent.
>
> I don't really understand what you mean by "keep the i2c interface
> consistent" here? The purpose of this abstraction is to abstract away
> the control interface used to communicate with the chip since it
> supports both I2C and SPI.
I understand that. I'm just saying that I would prefer wm8350->read_dev() to
return the actual bytes read, be it for SPI or I2C. Same for write_dev(), of
course.
With this patch you're breaking that expectation because the read|write_dev()
callers basically expect it to return 0 when the you've read|written the right
number of bytes.
I'd prefer to fix the callers code, so that we keep the expected semantics
for your read|write_dev() routines. For example with wm8350_clear_bits():
diff --git a/drivers/mfd/wm8350-core.c b/drivers/mfd/wm8350-core.c
index 0d47fb9..12ff3a6 100644
--- a/drivers/mfd/wm8350-core.c
+++ b/drivers/mfd/wm8350-core.c
@@ -199,19 +199,22 @@ static int wm8350_write(struct wm8350 *wm8350, u8 reg, int num_regs, u16 *src)
int wm8350_clear_bits(struct wm8350 *wm8350, u16 reg, u16 mask)
{
u16 data;
- int err;
+ int err = 0, ret;
mutex_lock(&io_mutex);
- err = wm8350_read(wm8350, reg, 1, &data);
- if (err) {
+ ret = wm8350_read(wm8350, reg, 1, &data);
+ if (ret != 1) {
dev_err(wm8350->dev, "read from reg R%d failed\n", reg);
+ err = -EIO;
goto out;
}
data &= ~mask;
- err = wm8350_write(wm8350, reg, 1, &data);
- if (err)
+ ret = wm8350_write(wm8350, reg, 1, &data);
+ if (ret != 1) {
dev_err(wm8350->dev, "write to reg R%d failed\n", reg);
+ err = -EIO;
+ }
out:
mutex_unlock(&io_mutex);
return err;
--
Intel Open Source Technology Centre
http://oss.intel.com/
On Thu, Nov 13, 2008 at 12:06:36AM +0100, Samuel Ortiz wrote:
> I understand that. I'm just saying that I would prefer wm8350->read_dev() to
> return the actual bytes read, be it for SPI or I2C. Same for write_dev(), of
> course.
Hrm. That's never been the case, even with the buggy code since the
register address was included in the physical access for at least the
writes.
> With this patch you're breaking that expectation because the read|write_dev()
> I'd prefer to fix the callers code, so that we keep the expected semantics
Just to clarify, when you say "expectation" and "fix" are you talking
about your preference that the hardware access functions return the
number of bytes read or written or is there something else going on
here?
> for your read|write_dev() routines. For example with wm8350_clear_bits():
> - if (err) {
> + ret = wm8350_read(wm8350, reg, 1, &data);
> + if (ret != 1) {
> dev_err(wm8350->dev, "read from reg R%d failed\n", reg);
> + err = -EIO;
Note that wm8350_read() works a level up, only calling the physical read
function if the register cache can't satisfy the access. Changing that
would be another layer of alteration.
To be honest I'm really not sure it's worth changing this - I'm having a
hard time thinking of a user that would be able to do anything useful
with a short access. It also seems like it'd be a rather invasive
change for an -rc.
On Wed, Nov 12, 2008 at 11:43:31PM +0000, Mark Brown wrote:
> > With this patch you're breaking that expectation because the read|write_dev()
> > I'd prefer to fix the callers code, so that we keep the expected semantics
>
> Just to clarify, when you say "expectation" and "fix" are you talking
> about your preference that the hardware access functions return the
> number of bytes read or written or is there something else going on
> here?
No, definitely just my preference. I personnaly expect this kind of IO
routines to return the number of bytes read/written.
> To be honest I'm really not sure it's worth changing this - I'm having a
> hard time thinking of a user that would be able to do anything useful
> with a short access.
I agree with that, yes.
> It also seems like it'd be a rather invasive
> change for an -rc.
Fair enough, I applied your patch, and sent Linus a pull request.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/