Thread (28 messages) 28 messages, 2 authors, 2021-05-27

Re: [PATCH 1/3] hwmon: (pmbus) Add driver for Delta DPS-920AB PSU

From: Robert Marko <robert.marko@sartura.hr>
Date: 2021-05-27 19:43:48
Also in: linux-hwmon, lkml

On Thu, May 27, 2021 at 9:27 PM Guenter Roeck [off-list ref] wrote:
On Thu, May 27, 2021 at 09:14:03PM +0200, Robert Marko wrote:
quoted
On Thu, May 27, 2021 at 6:37 PM Guenter Roeck [off-list ref] wrote:
quoted
On 5/27/21 7:58 AM, Robert Marko wrote:
[ ... ]
quoted
quoted
quoted
quoted
quoted
quoted
I tried applying the block support for mv64xx as well:
https://patchwork.ozlabs.org/project/linux-i2c/patch/20200118115820.9080-1-fuga@studiofuga.com/
That patch would be needed, but it looks buggy to me. This chunk:

+               drv_data->effective_length = data+1;
+               drv_data->bytes_left = data+1;
+               drv_data->msg->len = data+1;

should be:

+               drv_data->effective_length = data+1;
+               drv_data->bytes_left = data;
+               drv_data->msg->len = data+1;

It should also make sure that 'data' is not larger than I2C_SMBUS_BLOCK_MAX,
and bail out if it isn't.
Yeah, I did not check the contents, I just saw 2 reviews and tested it
since it can't hurt.
That patch doesn't work at all. Make the above change, and also change
the type of effective_length from u32 to int, and try again.
I was just looking and it, and doing the changes you recommended make
no difference at all.
Is the i2c controller compatible with marvell,mv78230-i2c ?
The block transfers would not work in that case. Let me know
and I'll send you a patch that might fix it.
Yes, the CPU is Armada 7040 and I can see that compatible is
marvell,mv78230-i2c indeed.

It would be great if you have the patch,
See below.
I can confirm that this does indeed enable block reads and
i2c_smbus_read_block_data()
works now.

PMBUS_MFR_ID and PMBUS_MFR_MODEL are indeed populated.

Regards,
Robert
quoted hunk ↗ jump to hunk
Guenter

---
From 41f6113560e3082293cef6e8f2013e70ee8f54db Mon Sep 17 00:00:00 2001
From: Federico Fuga <redacted>
Date: Sat, 18 Jan 2020 12:58:20 +0100
Subject: [PATCH] i2c: mv64xxx: Implement I2C_M_RECV_LEN and
 I2C_FUNC_SMBUS_READ_BLOCK_DATA

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 <redacted>
Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>
Link: https://lore.kernel.org/r/20200118115820.9080-1-fuga@studiofuga.com (local)
[groeck: Various fixes]
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 75 ++++++++++++++++++++++++++------
 1 file changed, 61 insertions(+), 14 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 5c8e94b6cdb5..9dda90359966 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -341,7 +341,17 @@ 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->bytes_left = 3;
+       else
+               drv_data->bytes_left = drv_data->msg->len;
+
        drv_data->aborting = 0;
        drv_data->rc = 0;
@@ -350,6 +360,47 @@ 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->msg->flags & I2C_M_RECV_LEN) {
+               if (!data || data > I2C_SMBUS_BLOCK_MAX) {
+                       /*
+                        * FIXME
+                        * Abort and report error. Needs to be
+                        * verified/tested on real hardware.
+                        */
+                       drv_data->rc = -EPROTO;
+                       mv64xxx_i2c_do_send_stop(drv_data);
+                       return;
+               }
+               drv_data->msg->flags &= ~I2C_M_RECV_LEN;
+               drv_data->bytes_left = data;
+               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)
 {
@@ -400,23 +451,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:
@@ -680,6 +721,10 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data)
        if (!drv_data->offload_enabled)
                return false;

+       /* Offload not supported for block data transfers */
+       if (msgs[0].flags & I2C_M_RECV_LEN)
+               return false;
+
        /*
         * We can offload a transaction consisting of a single
         * message, as long as the message has a length between 1 and
@@ -697,6 +742,7 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data)
            mv64xxx_i2c_valid_offload_sz(msgs) &&
            mv64xxx_i2c_valid_offload_sz(msgs + 1) &&
            !(msgs[0].flags & I2C_M_RD) &&
+           !(msgs[1].flags & I2C_M_RECV_LEN) &&
            msgs[1].flags & I2C_M_RD)
                return true;
@@ -713,7 +759,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.25.1

-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help