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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 


> -----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 =
> &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

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 = &regmap_i2c;
	else if (config->val_bits == 8 && config->reg_bits == 8 &&
		 i2c_check_functionality(i2c->adapter,
					 I2C_FUNC_SMBUS_I2C_BLOCK))
		bus = &regmap_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 = &regmap_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 = &regmap_smbus_word;
			break;
		case REGMAP_ENDIAN_BIG:
			bus = &regmap_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
[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux