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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <7f470259-89c9-4a4c-90d8-0997a706f7dd@roeck-us.net>
Date: Mon, 18 Dec 2023 11:01:12 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: "Matyas, Daniel" <Daniel.Matyas@...log.com>
Cc: Jean Delvare <jdelvare@...e.com>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>,
 "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
Subject: Re: [PATCH 1/3] hwmon: max31827: Add PEC support

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