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

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 ?

Thanks,
Guenter

>>
>> 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á
>