> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Monday, December 18, 2023 9:01 PM
> To: 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; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 1/3] hwmon: max31827: Add PEC support
>
> [External]
>
> 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 =
> ®map_i2c_smbus_i2c_block;
> drivers/base/regmap/regmap-i2c.c: bus =
> ®map_i2c_smbus_i2c_block_reg16;
> drivers/base/regmap/regmap-i2c.c: bus =
> ®map_smbus_word;
> drivers/base/regmap/regmap-i2c.c: bus =
> ®map_smbus_word_swapped;
> drivers/base/regmap/regmap-i2c.c: bus = ®map_smbus_byte;
>
> If that doesn't work for some reason, I'd rather figure out why instead of
> starting to drop regmap support.
>
> Guenter
I tried to figure it out and this is what I came up with. The code snippet below is from drivers/base/regmap/regmap-i2c.c:
static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
const struct regmap_config *config)
{
const struct i2c_adapter_quirks *quirks;
const struct regmap_bus *bus = NULL;
struct regmap_bus *ret_bus;
u16 max_read = 0, max_write = 0;
if (i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C))
bus = ®map_i2c;
else if (config->val_bits == 8 && config->reg_bits == 8 &&
i2c_check_functionality(i2c->adapter,
I2C_FUNC_SMBUS_I2C_BLOCK))
bus = ®map_i2c_smbus_i2c_block;
else if (config->val_bits == 8 && config->reg_bits == 16 &&
i2c_check_functionality(i2c->adapter,
I2C_FUNC_SMBUS_I2C_BLOCK))
bus = ®map_i2c_smbus_i2c_block_reg16;
else if (config->val_bits == 16 && config->reg_bits == 8 &&
i2c_check_functionality(i2c->adapter,
I2C_FUNC_SMBUS_WORD_DATA))
switch (regmap_get_val_endian(&i2c->dev, NULL, config)) {
case REGMAP_ENDIAN_LITTLE:
bus = ®map_smbus_word;
break;
case REGMAP_ENDIAN_BIG:
bus = ®map_smbus_word_swapped;
break;
default: /* everything else is not supported */
break;
}
This is executed when regmap is initialized. My adapter has the I2C_FUNC_I2C functionality (I use a raspberry pi 4), so it seems to me like regmap_i2c is loaded as the bus. This uses i2c_transfer internally to read and write.
For PEC I need regmap_smbus_word. This uses i2c_smbus_xfer internally. Unlike i2c_transfer, i2c_smbus_xfer can be used to send and receive PEC byte.
What should I do?
Kind regards,
Daniel
