Re: [PATCH 2/3] i2c: xlr: fix extra read/write at end of rx transfer

[Wolfram Sang]

On Mon, Nov 02, 2015 at 02:03:37AM +, Mans Rullgard wrote:
> The BYTECNT register holds the transfer size minus one.  Setting it
> to the correct value requires a dummy read/write only for zero-length
> transfers as it is impossible to request one from the hardware.  If a
> zero-length transfer is requested, changing the length to 1 and setting
> "buf" to a dummy location allows making the main loops less convoluted.
> 
> In other words, this patch makes the driver transfer the number of bytes
> requested unless this is zero, which is not supported by the hardware,
> in which case one byte is transferred instead.

Uh, this is wrong, zero byte should really not transfer anything. We
need to fix that and bail out, so probably something like

if (!len)
return -EOPNOTSUPP;

Also, the xlr_func() should mask out I2C_FUNC_SMBUS_QUICK.

Other than that, the patch looks good to me.

Out of curiosity, your first driver had the registers 32bit apart. Now
you can deal with 8bit. Is this configurable on this SoC?

Thanks,

   Wolfram



signature.asc
Description: Digital signature

Re: [PATCH 2/3] i2c: xlr: fix extra read/write at end of rx transfer

[Måns Rullgård]

Wolfram Sang  writes:

> On Mon, Nov 02, 2015 at 02:03:37AM +, Mans Rullgard wrote:
>> The BYTECNT register holds the transfer size minus one.  Setting it
>> to the correct value requires a dummy read/write only for zero-length
>> transfers as it is impossible to request one from the hardware.  If a
>> zero-length transfer is requested, changing the length to 1 and setting
>> "buf" to a dummy location allows making the main loops less convoluted.
>> 
>> In other words, this patch makes the driver transfer the number of bytes
>> requested unless this is zero, which is not supported by the hardware,
>> in which case one byte is transferred instead.
>
> Uh, this is wrong, zero byte should really not transfer anything. We
> need to fix that and bail out, so probably something like
>
>   if (!len)
>   return -EOPNOTSUPP;

So the existing driver is wrong to allow it.  Makes sense to drop that.

> Also, the xlr_func() should mask out I2C_FUNC_SMBUS_QUICK.

OK.

> Other than that, the patch looks good to me.
>
> Out of curiosity, your first driver had the registers 32bit apart. Now
> you can deal with 8bit. Is this configurable on this SoC?

It's all 32 bits.  The XLR driver uses a u32 * to access the registers.

-- 
Måns Rullgård
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html