Re: [PATCH 1/3] hwmon: max31827: Add PEC support

Guenter Roeck <linux@xxxxxxxxxxxx> · Mon, 18 Dec 2023 11:01:12 -0800

On 12/18/23 09:59, Matyas, Daniel wrote:

------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
*Von:* Guenter Roeck <groeck7@xxxxxxxxx> im Auftrag von Guenter Roeck <linux@xxxxxxxxxxxx>
*Gesendet:* Montag, Dezember 18, 2023 6:26:57 nachm.
*An:* Matyas, Daniel <Daniel.Matyas@xxxxxxxxxx>
*Cc:* Jean Delvare <jdelvare@xxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Jonathan Corbet <corbet@xxxxxxx>; linux-hwmon@xxxxxxxxxxxxxxx <linux-hwmon@xxxxxxxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx <devicetree@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>; linux-doc@xxxxxxxxxxxxxxx <linux-doc@xxxxxxxxxxxxxxx>
*Betreff:* Re: [PATCH 1/3] hwmon: max31827: Add PEC support

[External]

On 12/18/23 06:55, Matyas, Daniel wrote:
[ ... ]
On top of that, it is not clear why regmap can't be used in the first place.
It seems that the major change is that one needs to read the configuration
register after a write to see if there was a PEC error. It is not immediately
obvious why that additional read (if indeed necessary) would require
regmap support to be dropped.

I tried out writing and and reading with regmap, but it is not working properly. Even if I modify the client flag, I still receive only 2 bytes of data (a word). I should be receiving 2+1 bytes = data + CRC-8.

With i2c_smbus reads and writes, when I set the flag, I receive the 2+1 bytes, as expected.

The SMBus code in drivers/i2c/i2c-core-smbus.c is supposed to check
if the received PEC is correct for SMBus transfers. Are you saying
that this doesn't work, or that regmap doesn't use SMBus functions
to communicate with the chip ?

Thanks,
Guenter

I am 70% sure, that the regmap does not use SMBus functions.

It should.

$ git grep smbus drivers/base/regmap/regmap-i2c.c
drivers/base/regmap/regmap-i2c.c:static int regmap_smbus_byte_reg_read(void *context, unsigned int reg,
drivers/base/regmap/regmap-i2c.c:       ret = i2c_smbus_read_byte_data(i2c, reg);
drivers/base/regmap/regmap-i2c.c:static int regmap_smbus_byte_reg_write(void *context, unsigned int reg,
drivers/base/regmap/regmap-i2c.c:       return i2c_smbus_write_byte_data(i2c, reg, val);
drivers/base/regmap/regmap-i2c.c:static const struct regmap_bus regmap_smbus_byte = {
drivers/base/regmap/regmap-i2c.c:       .reg_write = regmap_smbus_byte_reg_write,
drivers/base/regmap/regmap-i2c.c:       .reg_read = regmap_smbus_byte_reg_read,
drivers/base/regmap/regmap-i2c.c:static int regmap_smbus_word_reg_read(void *context, unsigned int reg,
drivers/base/regmap/regmap-i2c.c:       ret = i2c_smbus_read_word_data(i2c, reg);
drivers/base/regmap/regmap-i2c.c:static int regmap_smbus_word_reg_write(void *context, unsigned int reg,
drivers/base/regmap/regmap-i2c.c:       return i2c_smbus_write_word_data(i2c, reg, val);
drivers/base/regmap/regmap-i2c.c:static const struct regmap_bus regmap_smbus_word = {
drivers/base/regmap/regmap-i2c.c:       .reg_write = regmap_smbus_word_reg_write,
drivers/base/regmap/regmap-i2c.c:       .reg_read = regmap_smbus_word_reg_read,
drivers/base/regmap/regmap-i2c.c:static int regmap_smbus_word_read_swapped(void *context, unsigned int reg,
drivers/base/regmap/regmap-i2c.c:       ret = i2c_smbus_read_word_swapped(i2c, reg);
drivers/base/regmap/regmap-i2c.c:static int regmap_smbus_word_write_swapped(void *context, unsigned int reg,
drivers/base/regmap/regmap-i2c.c:       return i2c_smbus_write_word_swapped(i2c, reg, val);
drivers/base/regmap/regmap-i2c.c:static const struct regmap_bus regmap_smbus_word_swapped = {
drivers/base/regmap/regmap-i2c.c:       .reg_write = regmap_smbus_word_write_swapped,
drivers/base/regmap/regmap-i2c.c:       .reg_read = regmap_smbus_word_read_swapped,
drivers/base/regmap/regmap-i2c.c:static int regmap_i2c_smbus_i2c_write(void *context, const void *data,
drivers/base/regmap/regmap-i2c.c:       return i2c_smbus_write_i2c_block_data(i2c, ((u8 *)data)[0], count,
drivers/base/regmap/regmap-i2c.c:static int regmap_i2c_smbus_i2c_read(void *context, const void *reg,
drivers/base/regmap/regmap-i2c.c:       ret = i2c_smbus_read_i2c_block_data(i2c, ((u8 *)reg)[0], val_size, val);
drivers/base/regmap/regmap-i2c.c:static const struct regmap_bus regmap_i2c_smbus_i2c_block = {
drivers/base/regmap/regmap-i2c.c:       .write = regmap_i2c_smbus_i2c_write,
drivers/base/regmap/regmap-i2c.c:       .read = regmap_i2c_smbus_i2c_read,
drivers/base/regmap/regmap-i2c.c:static int regmap_i2c_smbus_i2c_write_reg16(void *context, const void *data,
drivers/base/regmap/regmap-i2c.c:       return i2c_smbus_write_i2c_block_data(i2c, ((u8 *)data)[0], count,
drivers/base/regmap/regmap-i2c.c:static int regmap_i2c_smbus_i2c_read_reg16(void *context, const void *reg,
drivers/base/regmap/regmap-i2c.c:       ret = i2c_smbus_write_byte_data(i2c, ((u16 *)reg)[0] & 0xff,
drivers/base/regmap/regmap-i2c.c:               ret = i2c_smbus_read_byte(i2c);
drivers/base/regmap/regmap-i2c.c:static const struct regmap_bus regmap_i2c_smbus_i2c_block_reg16 = {
drivers/base/regmap/regmap-i2c.c:       .write = regmap_i2c_smbus_i2c_write_reg16,
drivers/base/regmap/regmap-i2c.c:       .read = regmap_i2c_smbus_i2c_read_reg16,
drivers/base/regmap/regmap-i2c.c:               bus = &regmap_i2c_smbus_i2c_block;
drivers/base/regmap/regmap-i2c.c:               bus = &regmap_i2c_smbus_i2c_block_reg16;
drivers/base/regmap/regmap-i2c.c:                       bus = &regmap_smbus_word;
drivers/base/regmap/regmap-i2c.c:                       bus = &regmap_smbus_word_swapped;
drivers/base/regmap/regmap-i2c.c:               bus = &regmap_smbus_byte;

If that doesn't work for some reason, I'd rather figure out why instead of
starting to drop regmap support.

Guenter