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

On 8/3/26 08:03, Nuno Sá wrote:
> 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.
> 

No, I found that other chips do the same.

I am working on a generic function pmbus_read_smbus_i2c_block_data() which calls
i2c_smbus_read_block_data() if supported and otherwise (if supoported) calls
i2c_smbus_read_i2c_block_data(). It should be ready within a day or so.

Thanks,
Guenter