Re: [PATCH 3/5] hwmon: (pmbus/core) Add mapping function to pmbus_read_block_data()

On Fri, Jul 31, 2026 at 09:46:29AM -0700, Guenter Roeck wrote:
> On 7/31/26 08:42, Nuno Sá wrote:
> > On Thu, Jul 30, 2026 at 08:54:25AM -0700, Guenter Roeck wrote:
> > > On 7/30/26 08:19, Nuno Sá wrote:
> > > > On Thu, Jul 30, 2026 at 07:47:37AM -0700, Guenter Roeck wrote:
> > > > > On 7/30/26 07:27, Nuno Sá wrote:
> > > > > > On Tue, Jul 28, 2026 at 02:05:53PM -0700, Guenter Roeck wrote:
> > > > > > > On 7/28/26 09:03, Nuno Sá via B4 Relay wrote:
> > > > > > > > From: Nuno Sá <[email protected]>
> > > > > > > > 
> > > > > > > > This is in preparation for adding support to a device which needs to
> > > > > > > > use it's own read_block implementation.
> > > > > > > > 
> > > > > > > 
> > > > > > > The chip-specific implementation calls i2c_smbus_read_i2c_block_data().
> > > > > > > I'll need to know if this is a chip limit or a controller limit.
> > > > > > > If it is a controller limit, a chip specific override would be
> > > > > > > inappropriate.
> > > > > > 
> > > > > > I'll reply from top of my head (did not looked at the driver again).
> > > > > > IIRC, the biggest reason we need the chip-specific implementation is because of
> > > > > > the RAIL selection logic (mainly when not in page mode).
> > > > > > 
> > > > > 
> > > > > Yes, I have seen that. Question is why you use i2c_smbus_read_i2c_block_data()
> > > > > instead of i2c_smbus_read_block_data(). The rail selection logic would not
> > > > > require that.
> > > > 
> > > > IIRC the reason was because the i2c controller on the raspberry pie does not
> > > > support i2c_smbus_read_block_data(). So I guess this:
> > > > 
> > > > https://elixir.bootlin.com/linux/v7.1.4/source/drivers/i2c/i2c-core-smbus.c#L220
> > > > 
> > > 
> > > Hmm, I think we really need a common solution for that problem. Not all
> > > controllers support i2c_smbus_read_i2c_block_data(), so you are just moving
> > > the problem from one controller to another.
> > 
> > Yikes. I actually though i2c_smbus_read_i2c_block_data(9 was more widely
> > supported (if not always). Not sure if you have something in mind but
> > one straight way would be to choose different implementations (for
> > read_block) depending on i2c_check_functionality()
> > 
> 
> Handling it in pmbus_read_block_data() would be straightforward. Actually, I
> wonder why it isn't handled as fallback in i2c_smbus_read_block_data(),
> but I assume there must be a reason.
> 
> Either case, I don't understand how the existing calls to i2c_smbus_read_i2c_block_data()
> work. For example, in drivers/hwmon/pmbus/max20830.c, the assumption is that the
> first returned data byte would be the length field. However, that is already done
> in i2c_smbus_read_i2c_block_data():
> 
> /* Returns the number of read bytes */
> s32 i2c_smbus_read_i2c_block_data(const struct i2c_client *client, u8 command,
>                                   u8 length, u8 *values)
> {
>         union i2c_smbus_data data;
>         int status;
> 
>         if (length > I2C_SMBUS_BLOCK_MAX)
>                 length = I2C_SMBUS_BLOCK_MAX;
>         data.block[0] = length;
>         status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
>                                 I2C_SMBUS_READ, command,
>                                 I2C_SMBUS_I2C_BLOCK_DATA, &data);
>         if (status < 0)
>                 return status;
> 
>         memcpy(values, &data.block[1], data.block[0]);
>         return data.block[0];
> }
> 
> Does the command return the length twice ?

I think the device is actually the one sending the block size again. I
went looking at very old emails and found this chain:

Me:

"Thanks for the inputs... As I said, I think know what’s my issue. I’m fairly sure the first
byte I’m reading is the block size. I’ll take this into account and re-test."

Reply:

"On one hand, that does make sense. The first byte in a block read transaction should indeed
be the block size. That said, 0xB1 is 6 bytes long when not including this first byte. So we
are still missing a byte with actual information. Could you perhaps please share the full 
packet data that you are capturing?"

The above was me struggling with per phase reads.

But maybe the above changed with newer FW versions for the chips. I'll
ask around.

- Nuno Sá

> 
> > > 
> > > One key example is the PIIX4 driver. Your new driver does not support the
> > > majority of PC style systems with AMD CPUs.
> > > 
> > 
> > I see!
> > 
> > - Nuno Sá
> > 
>