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

From: Guenter Roeck <linux@roeck-us.net>
To: "Matyas, Daniel" <Daniel.Matyas@analog.com>
Cc: Jean Delvare <jdelvare@suse.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Jonathan Corbet <corbet@lwn.net>,
	"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>
Subject: Re: [PATCH 1/3] hwmon: max31827: Add PEC support
Date: Mon, 18 Dec 2023 11:01:12 -0800	[thread overview]
Message-ID: <7f470259-89c9-4a4c-90d8-0997a706f7dd@roeck-us.net> (raw)
In-Reply-To: <PH0PR03MB6771CF6A95CB576E534C225F8990A@PH0PR03MB6771.namprd03.prod.outlook.com>

On 12/18/23 09:59, Matyas, Daniel wrote:
> 
> 
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> *Von:* Guenter Roeck <groeck7@gmail.com> im Auftrag von Guenter Roeck <linux@roeck-us.net>
> *Gesendet:* Montag, Dezember 18, 2023 6:26:57 nachm.
> *An:* Matyas, Daniel <Daniel.Matyas@analog.com>
> *Cc:* Jean Delvare <jdelvare@suse.com>; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; Jonathan Corbet <corbet@lwn.net>; linux-hwmon@vger.kernel.org <linux-hwmon@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-doc@vger.kernel.org <linux-doc@vger.kernel.org>
> *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