* [PATCH] i2c: mv64xxx: Implement I2C_M_RECV_LEN and I2C_FUNC_SMBUS_READ_BLOCK_DATA
@ 2020-01-18 11:58 ` Federico Fuga
0 siblings, 0 replies; 6+ messages in thread
From: Federico Fuga @ 2020-01-18 11:58 UTC (permalink / raw)
Cc: Federico Fuga, Gregory CLEMENT, linux-i2c, linux-kernel
The i2c_mv64xxx driver doesn't implement the I2C_M_REC_LEN function
essential to allow blocks with variable length to be read from an i2c
slave.
This is needed to implement the SMBus Read Block Data function.
This patch implements the function by changing the bytes_left and
msg len on the fly if the flag is specified.
It has been successfully tested on Allwinner A33 with a special
i2c chip that returns variable length blocks on reading.
Signed-off-by: Federico Fuga <fuga@studiofuga.com>
---
drivers/i2c/busses/i2c-mv64xxx.c | 67 +++++++++++++++++++++++++-------
1 file changed, 53 insertions(+), 14 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index a5a95ea5b81a..cff9cb20bcc9 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -128,6 +128,7 @@ struct mv64xxx_i2c_data {
u32 addr1;
u32 addr2;
u32 bytes_left;
+ u32 effective_length;
u32 byte_posn;
u32 send_stop;
u32 block;
@@ -333,7 +334,18 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
{
drv_data->msg = drv_data->msgs;
drv_data->byte_posn = 0;
- drv_data->bytes_left = drv_data->msg->len;
+
+ /* If we should retrieve the length from the buffer, make sure */
+ /* to read enough bytes to avoid sending the */
+ /* STOP bit after the read if the first byte */
+ if (drv_data->msg->flags & I2C_M_RECV_LEN) {
+ drv_data->effective_length = -1;
+ drv_data->bytes_left = 3;
+ } else {
+ drv_data->effective_length = drv_data->msg->len;
+ drv_data->bytes_left = drv_data->msg->len;
+ }
+
drv_data->aborting = 0;
drv_data->rc = 0;
@@ -342,6 +354,42 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
drv_data->reg_base + drv_data->reg_offsets.control);
}
+static void
+mv64xxx_i2c_do_send_stop(struct mv64xxx_i2c_data *drv_data)
+{
+ drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
+ writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
+ drv_data->reg_base + drv_data->reg_offsets.control);
+ drv_data->block = 0;
+ if (drv_data->errata_delay)
+ udelay(5);
+
+ wake_up(&drv_data->waitq);
+}
+
+static void
+mv64xxx_i2c_do_read_data(struct mv64xxx_i2c_data *drv_data)
+{
+ u8 data;
+
+ data = readl(drv_data->reg_base + drv_data->reg_offsets.data);
+ drv_data->msg->buf[drv_data->byte_posn++] = data;
+
+ if (drv_data->effective_length == -1) {
+ /* length=0 should not be allowed, but is indeed possible.
+ * To avoid locking the chip, we keep reading at least 2 bytes
+ */
+ if (data < 1)
+ data = 1;
+ drv_data->effective_length = data+1;
+ drv_data->bytes_left = data+1;
+ drv_data->msg->len = data+1;
+ }
+
+ writel(drv_data->cntl_bits,
+ drv_data->reg_base + drv_data->reg_offsets.control);
+}
+
static void
mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
{
@@ -392,23 +440,13 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
break;
case MV64XXX_I2C_ACTION_RCV_DATA:
- drv_data->msg->buf[drv_data->byte_posn++] =
- readl(drv_data->reg_base + drv_data->reg_offsets.data);
- writel(drv_data->cntl_bits,
- drv_data->reg_base + drv_data->reg_offsets.control);
+ mv64xxx_i2c_do_read_data(drv_data);
break;
case MV64XXX_I2C_ACTION_RCV_DATA_STOP:
drv_data->msg->buf[drv_data->byte_posn++] =
readl(drv_data->reg_base + drv_data->reg_offsets.data);
- drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
- writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
- drv_data->reg_base + drv_data->reg_offsets.control);
- drv_data->block = 0;
- if (drv_data->errata_delay)
- udelay(5);
-
- wake_up(&drv_data->waitq);
+ mv64xxx_i2c_do_send_stop(drv_data);
break;
case MV64XXX_I2C_ACTION_INVALID:
@@ -706,7 +744,8 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data)
static u32
mv64xxx_i2c_functionality(struct i2c_adapter *adap)
{
- return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;
+ return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR |
+ I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_EMUL;
}
static int
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] i2c: mv64xxx: Implement I2C_M_RECV_LEN and I2C_FUNC_SMBUS_READ_BLOCK_DATA
@ 2020-01-18 11:58 ` Federico Fuga
0 siblings, 0 replies; 6+ messages in thread
From: Federico Fuga @ 2020-01-18 11:58 UTC (permalink / raw)
Cc: Federico Fuga, Gregory CLEMENT, linux-i2c, linux-kernel
The i2c_mv64xxx driver doesn't implement the I2C_M_REC_LEN function
essential to allow blocks with variable length to be read from an i2c
slave.
This is needed to implement the SMBus Read Block Data function.
This patch implements the function by changing the bytes_left and
msg len on the fly if the flag is specified.
It has been successfully tested on Allwinner A33 with a special
i2c chip that returns variable length blocks on reading.
Signed-off-by: Federico Fuga <fuga@studiofuga.com>
---
drivers/i2c/busses/i2c-mv64xxx.c | 67 +++++++++++++++++++++++++-------
1 file changed, 53 insertions(+), 14 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index a5a95ea5b81a..cff9cb20bcc9 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -128,6 +128,7 @@ struct mv64xxx_i2c_data {
u32 addr1;
u32 addr2;
u32 bytes_left;
+ u32 effective_length;
u32 byte_posn;
u32 send_stop;
u32 block;
@@ -333,7 +334,18 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
{
drv_data->msg = drv_data->msgs;
drv_data->byte_posn = 0;
- drv_data->bytes_left = drv_data->msg->len;
+
+ /* If we should retrieve the length from the buffer, make sure */
+ /* to read enough bytes to avoid sending the */
+ /* STOP bit after the read if the first byte */
+ if (drv_data->msg->flags & I2C_M_RECV_LEN) {
+ drv_data->effective_length = -1;
+ drv_data->bytes_left = 3;
+ } else {
+ drv_data->effective_length = drv_data->msg->len;
+ drv_data->bytes_left = drv_data->msg->len;
+ }
+
drv_data->aborting = 0;
drv_data->rc = 0;
@@ -342,6 +354,42 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
drv_data->reg_base + drv_data->reg_offsets.control);
}
+static void
+mv64xxx_i2c_do_send_stop(struct mv64xxx_i2c_data *drv_data)
+{
+ drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
+ writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
+ drv_data->reg_base + drv_data->reg_offsets.control);
+ drv_data->block = 0;
+ if (drv_data->errata_delay)
+ udelay(5);
+
+ wake_up(&drv_data->waitq);
+}
+
+static void
+mv64xxx_i2c_do_read_data(struct mv64xxx_i2c_data *drv_data)
+{
+ u8 data;
+
+ data = readl(drv_data->reg_base + drv_data->reg_offsets.data);
+ drv_data->msg->buf[drv_data->byte_posn++] = data;
+
+ if (drv_data->effective_length == -1) {
+ /* length=0 should not be allowed, but is indeed possible.
+ * To avoid locking the chip, we keep reading at least 2 bytes
+ */
+ if (data < 1)
+ data = 1;
+ drv_data->effective_length = data+1;
+ drv_data->bytes_left = data+1;
+ drv_data->msg->len = data+1;
+ }
+
+ writel(drv_data->cntl_bits,
+ drv_data->reg_base + drv_data->reg_offsets.control);
+}
+
static void
mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
{
@@ -392,23 +440,13 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
break;
case MV64XXX_I2C_ACTION_RCV_DATA:
- drv_data->msg->buf[drv_data->byte_posn++] =
- readl(drv_data->reg_base + drv_data->reg_offsets.data);
- writel(drv_data->cntl_bits,
- drv_data->reg_base + drv_data->reg_offsets.control);
+ mv64xxx_i2c_do_read_data(drv_data);
break;
case MV64XXX_I2C_ACTION_RCV_DATA_STOP:
drv_data->msg->buf[drv_data->byte_posn++] =
readl(drv_data->reg_base + drv_data->reg_offsets.data);
- drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
- writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
- drv_data->reg_base + drv_data->reg_offsets.control);
- drv_data->block = 0;
- if (drv_data->errata_delay)
- udelay(5);
-
- wake_up(&drv_data->waitq);
+ mv64xxx_i2c_do_send_stop(drv_data);
break;
case MV64XXX_I2C_ACTION_INVALID:
@@ -706,7 +744,8 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data)
static u32
mv64xxx_i2c_functionality(struct i2c_adapter *adap)
{
- return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;
+ return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR |
+ I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_EMUL;
}
static int
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] i2c: mv64xxx: Implement I2C_M_RECV_LEN and I2C_FUNC_SMBUS_READ_BLOCK_DATA
2020-01-18 11:58 ` Federico Fuga
(?)
@ 2020-02-22 12:48 ` Wolfram Sang
2020-03-13 20:48 ` Gregory CLEMENT
-1 siblings, 1 reply; 6+ messages in thread
From: Wolfram Sang @ 2020-02-22 12:48 UTC (permalink / raw)
To: Federico Fuga; +Cc: Gregory CLEMENT, linux-i2c, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4667 bytes --]
On Sat, Jan 18, 2020 at 12:58:20PM +0100, Federico Fuga wrote:
> The i2c_mv64xxx driver doesn't implement the I2C_M_REC_LEN function
> essential to allow blocks with variable length to be read from an i2c
> slave.
> This is needed to implement the SMBus Read Block Data function.
>
> This patch implements the function by changing the bytes_left and
> msg len on the fly if the flag is specified.
>
> It has been successfully tested on Allwinner A33 with a special
> i2c chip that returns variable length blocks on reading.
>
> Signed-off-by: Federico Fuga <fuga@studiofuga.com>
> ---
Gregory, any comment? I can't say much about the implementation. In
general, this is a nice feature to have...
> drivers/i2c/busses/i2c-mv64xxx.c | 67 +++++++++++++++++++++++++-------
> 1 file changed, 53 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index a5a95ea5b81a..cff9cb20bcc9 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -128,6 +128,7 @@ struct mv64xxx_i2c_data {
> u32 addr1;
> u32 addr2;
> u32 bytes_left;
> + u32 effective_length;
> u32 byte_posn;
> u32 send_stop;
> u32 block;
> @@ -333,7 +334,18 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
> {
> drv_data->msg = drv_data->msgs;
> drv_data->byte_posn = 0;
> - drv_data->bytes_left = drv_data->msg->len;
> +
> + /* If we should retrieve the length from the buffer, make sure */
> + /* to read enough bytes to avoid sending the */
> + /* STOP bit after the read if the first byte */
> + if (drv_data->msg->flags & I2C_M_RECV_LEN) {
> + drv_data->effective_length = -1;
> + drv_data->bytes_left = 3;
> + } else {
> + drv_data->effective_length = drv_data->msg->len;
> + drv_data->bytes_left = drv_data->msg->len;
> + }
> +
> drv_data->aborting = 0;
> drv_data->rc = 0;
>
> @@ -342,6 +354,42 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
> drv_data->reg_base + drv_data->reg_offsets.control);
> }
>
> +static void
> +mv64xxx_i2c_do_send_stop(struct mv64xxx_i2c_data *drv_data)
> +{
> + drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
> + writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
> + drv_data->reg_base + drv_data->reg_offsets.control);
> + drv_data->block = 0;
> + if (drv_data->errata_delay)
> + udelay(5);
> +
> + wake_up(&drv_data->waitq);
> +}
> +
> +static void
> +mv64xxx_i2c_do_read_data(struct mv64xxx_i2c_data *drv_data)
> +{
> + u8 data;
> +
> + data = readl(drv_data->reg_base + drv_data->reg_offsets.data);
> + drv_data->msg->buf[drv_data->byte_posn++] = data;
> +
> + if (drv_data->effective_length == -1) {
> + /* length=0 should not be allowed, but is indeed possible.
> + * To avoid locking the chip, we keep reading at least 2 bytes
> + */
> + if (data < 1)
> + data = 1;
> + drv_data->effective_length = data+1;
> + drv_data->bytes_left = data+1;
> + drv_data->msg->len = data+1;
> + }
> +
> + writel(drv_data->cntl_bits,
> + drv_data->reg_base + drv_data->reg_offsets.control);
> +}
> +
> static void
> mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
> {
> @@ -392,23 +440,13 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
> break;
>
> case MV64XXX_I2C_ACTION_RCV_DATA:
> - drv_data->msg->buf[drv_data->byte_posn++] =
> - readl(drv_data->reg_base + drv_data->reg_offsets.data);
> - writel(drv_data->cntl_bits,
> - drv_data->reg_base + drv_data->reg_offsets.control);
> + mv64xxx_i2c_do_read_data(drv_data);
> break;
>
> case MV64XXX_I2C_ACTION_RCV_DATA_STOP:
> drv_data->msg->buf[drv_data->byte_posn++] =
> readl(drv_data->reg_base + drv_data->reg_offsets.data);
> - drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
> - writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
> - drv_data->reg_base + drv_data->reg_offsets.control);
> - drv_data->block = 0;
> - if (drv_data->errata_delay)
> - udelay(5);
> -
> - wake_up(&drv_data->waitq);
> + mv64xxx_i2c_do_send_stop(drv_data);
> break;
>
> case MV64XXX_I2C_ACTION_INVALID:
> @@ -706,7 +744,8 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data)
> static u32
> mv64xxx_i2c_functionality(struct i2c_adapter *adap)
> {
> - return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;
> + return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR |
> + I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_EMUL;
> }
>
> static int
> --
> 2.17.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] i2c: mv64xxx: Implement I2C_M_RECV_LEN and I2C_FUNC_SMBUS_READ_BLOCK_DATA
2020-02-22 12:48 ` Wolfram Sang
@ 2020-03-13 20:48 ` Gregory CLEMENT
2020-03-13 20:58 ` Federico Fuga
0 siblings, 1 reply; 6+ messages in thread
From: Gregory CLEMENT @ 2020-03-13 20:48 UTC (permalink / raw)
To: Wolfram Sang, Federico Fuga; +Cc: linux-i2c, linux-kernel
Hello Wolfram,
> On Sat, Jan 18, 2020 at 12:58:20PM +0100, Federico Fuga wrote:
>> The i2c_mv64xxx driver doesn't implement the I2C_M_REC_LEN function
>> essential to allow blocks with variable length to be read from an i2c
>> slave.
>> This is needed to implement the SMBus Read Block Data function.
>>
>> This patch implements the function by changing the bytes_left and
>> msg len on the fly if the flag is specified.
>>
>> It has been successfully tested on Allwinner A33 with a special
>> i2c chip that returns variable length blocks on reading.
>>
>> Signed-off-by: Federico Fuga <fuga@studiofuga.com>
>> ---
>
> Gregory, any comment? I can't say much about the implementation. In
> general, this is a nice feature to have...
I thought I was already sent a reviewed by, as I reviewed this patch
before Frederico actually sent it.
So you can even add a
Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>
Thanks,
Gregory
>
>> drivers/i2c/busses/i2c-mv64xxx.c | 67 +++++++++++++++++++++++++-------
>> 1 file changed, 53 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
>> index a5a95ea5b81a..cff9cb20bcc9 100644
>> --- a/drivers/i2c/busses/i2c-mv64xxx.c
>> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
>> @@ -128,6 +128,7 @@ struct mv64xxx_i2c_data {
>> u32 addr1;
>> u32 addr2;
>> u32 bytes_left;
>> + u32 effective_length;
>> u32 byte_posn;
>> u32 send_stop;
>> u32 block;
>> @@ -333,7 +334,18 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
>> {
>> drv_data->msg = drv_data->msgs;
>> drv_data->byte_posn = 0;
>> - drv_data->bytes_left = drv_data->msg->len;
>> +
>> + /* If we should retrieve the length from the buffer, make sure */
>> + /* to read enough bytes to avoid sending the */
>> + /* STOP bit after the read if the first byte */
>> + if (drv_data->msg->flags & I2C_M_RECV_LEN) {
>> + drv_data->effective_length = -1;
>> + drv_data->bytes_left = 3;
>> + } else {
>> + drv_data->effective_length = drv_data->msg->len;
>> + drv_data->bytes_left = drv_data->msg->len;
>> + }
>> +
>> drv_data->aborting = 0;
>> drv_data->rc = 0;
>>
>> @@ -342,6 +354,42 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
>> drv_data->reg_base + drv_data->reg_offsets.control);
>> }
>>
>> +static void
>> +mv64xxx_i2c_do_send_stop(struct mv64xxx_i2c_data *drv_data)
>> +{
>> + drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
>> + writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
>> + drv_data->reg_base + drv_data->reg_offsets.control);
>> + drv_data->block = 0;
>> + if (drv_data->errata_delay)
>> + udelay(5);
>> +
>> + wake_up(&drv_data->waitq);
>> +}
>> +
>> +static void
>> +mv64xxx_i2c_do_read_data(struct mv64xxx_i2c_data *drv_data)
>> +{
>> + u8 data;
>> +
>> + data = readl(drv_data->reg_base + drv_data->reg_offsets.data);
>> + drv_data->msg->buf[drv_data->byte_posn++] = data;
>> +
>> + if (drv_data->effective_length == -1) {
>> + /* length=0 should not be allowed, but is indeed possible.
>> + * To avoid locking the chip, we keep reading at least 2 bytes
>> + */
>> + if (data < 1)
>> + data = 1;
>> + drv_data->effective_length = data+1;
>> + drv_data->bytes_left = data+1;
>> + drv_data->msg->len = data+1;
>> + }
>> +
>> + writel(drv_data->cntl_bits,
>> + drv_data->reg_base + drv_data->reg_offsets.control);
>> +}
>> +
>> static void
>> mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>> {
>> @@ -392,23 +440,13 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>> break;
>>
>> case MV64XXX_I2C_ACTION_RCV_DATA:
>> - drv_data->msg->buf[drv_data->byte_posn++] =
>> - readl(drv_data->reg_base + drv_data->reg_offsets.data);
>> - writel(drv_data->cntl_bits,
>> - drv_data->reg_base + drv_data->reg_offsets.control);
>> + mv64xxx_i2c_do_read_data(drv_data);
>> break;
>>
>> case MV64XXX_I2C_ACTION_RCV_DATA_STOP:
>> drv_data->msg->buf[drv_data->byte_posn++] =
>> readl(drv_data->reg_base + drv_data->reg_offsets.data);
>> - drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
>> - writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
>> - drv_data->reg_base + drv_data->reg_offsets.control);
>> - drv_data->block = 0;
>> - if (drv_data->errata_delay)
>> - udelay(5);
>> -
>> - wake_up(&drv_data->waitq);
>> + mv64xxx_i2c_do_send_stop(drv_data);
>> break;
>>
>> case MV64XXX_I2C_ACTION_INVALID:
>> @@ -706,7 +744,8 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data)
>> static u32
>> mv64xxx_i2c_functionality(struct i2c_adapter *adap)
>> {
>> - return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;
>> + return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR |
>> + I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_EMUL;
>> }
>>
>> static int
>> --
>> 2.17.1
>>
--
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] i2c: mv64xxx: Implement I2C_M_RECV_LEN and I2C_FUNC_SMBUS_READ_BLOCK_DATA
2020-03-13 20:48 ` Gregory CLEMENT
@ 2020-03-13 20:58 ` Federico Fuga
0 siblings, 0 replies; 6+ messages in thread
From: Federico Fuga @ 2020-03-13 20:58 UTC (permalink / raw)
To: Gregory CLEMENT; +Cc: Wolfram Sang, linux-i2c, linux-kernel
Hello all,
> Il giorno 13 mar 2020, alle ore 21:48, Gregory CLEMENT <gregory.clement@bootlin.com> ha scritto:
>
> Hello Wolfram,
>
>> On Sat, Jan 18, 2020 at 12:58:20PM +0100, Federico Fuga wrote:
>>> The i2c_mv64xxx driver doesn't implement the I2C_M_REC_LEN function
>>> essential to allow blocks with variable length to be read from an i2c
>>> slave.
>>> This is needed to implement the SMBus Read Block Data function.
>>>
>>> This patch implements the function by changing the bytes_left and
>>> msg len on the fly if the flag is specified.
>>>
>>> It has been successfully tested on Allwinner A33 with a special
>>> i2c chip that returns variable length blocks on reading.
>>>
>>> Signed-off-by: Federico Fuga <fuga@studiofuga.com>
>>> ---
>>
>> Gregory, any comment? I can't say much about the implementation. In
>> general, this is a nice feature to have...
>
> I thought I was already sent a reviewed by, as I reviewed this patch
> before Frederico actually sent it.
>
> So you can even add a
>
> Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>
>
> Thanks,
For what it worth, I can confirm that after many weeks of use the patch works as expected without any issue.
Federico
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: mv64xxx: Implement I2C_M_RECV_LEN and I2C_FUNC_SMBUS_READ_BLOCK_DATA
2020-01-18 11:58 ` Federico Fuga
(?)
(?)
@ 2020-03-20 14:55 ` Wolfram Sang
-1 siblings, 0 replies; 6+ messages in thread
From: Wolfram Sang @ 2020-03-20 14:55 UTC (permalink / raw)
To: Federico Fuga; +Cc: Gregory CLEMENT, linux-i2c, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1400 bytes --]
> + /* If we should retrieve the length from the buffer, make sure */
> + /* to read enough bytes to avoid sending the */
> + /* STOP bit after the read if the first byte */
Please stick to Kernel commenting style.
> + /* length=0 should not be allowed, but is indeed possible.
> + * To avoid locking the chip, we keep reading at least 2 bytes
> + */
Ditto.
> + drv_data->effective_length = data+1;
> + drv_data->bytes_left = data+1;
> + drv_data->msg->len = data+1;
Also, checkpatch complains about this if you run it with "--strict'
CHECKPATCH
CHECK: spaces preferred around that '+' (ctx:VxV)
#81: FILE: drivers/i2c/busses/i2c-mv64xxx.c:384:
+ drv_data->effective_length = data+1;
^
CHECK: spaces preferred around that '+' (ctx:VxV)
#82: FILE: drivers/i2c/busses/i2c-mv64xxx.c:385:
+ drv_data->bytes_left = data+1;
^
CHECK: spaces preferred around that '+' (ctx:VxV)
#83: FILE: drivers/i2c/busses/i2c-mv64xxx.c:386:
+ drv_data->msg->len = data+1;
^
I think the complaints make sense.
> - return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;
> + return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR |
> + I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_EMUL;
Likely I2C_FUNC_SMBUS_BLOCK_PROC_CALL will work as well, but I assume
you can't test it?
Otherwise, looks good.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-20 14:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-18 11:58 [PATCH] i2c: mv64xxx: Implement I2C_M_RECV_LEN and I2C_FUNC_SMBUS_READ_BLOCK_DATA Federico Fuga
2020-01-18 11:58 ` Federico Fuga
2020-02-22 12:48 ` Wolfram Sang
2020-03-13 20:48 ` Gregory CLEMENT
2020-03-13 20:58 ` Federico Fuga
2020-03-20 14:55 ` Wolfram Sang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.