Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1292106 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/3] i2c: xlr: fix extra read/write at end of rx transfer Wolfram Sang <wsa@the-dreams.de> - 2015-12-15 13:30 +0100
    Re: [PATCH 2/3] i2c: xlr: fix extra read/write at end of rx transfer Måns Rullgård <mans@mansr.com> - 2015-12-15 13:50 +0100

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

[Multipart message — attachments visible in raw view] — view raw

On Mon, Nov 02, 2015 at 02:03:37AM +0000, 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

[toc] | [next] | [standalone]

#1292128

Wolfram Sang <wsa@the-dreams.de> writes:

> On Mon, Nov 02, 2015 at 02:03:37AM +0000, 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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web