Message ID | 20240822074426.7241-1-wenliang202407@163.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | [linux,dev,6.11] hwmon:add new hwmon driver sq52205 | expand |
On 8/22/24 00:44, Wenliang wrote: > Thank you for bringing up your questions and suggestions.The SQ52205 is a > part number specific to the Asian region, which is why you might not be > able to find it through a search. I'll provide you the website > (https://us1.silergy.com/zh/productsview/SQ52205FBP). That page does not point to the chip datasheet. The almost identical page at https://us1.silergy.com/productsview/SQ52205FBP does. The datasheet is _not_ "Publicly available" as claimed in this submission. The version I was able to obtain is tagged with "Silergy Confidential For <my employer>", so I am not even sure if I can use it to review this driver submission. > Some registers of this chip are similar to those of the INA226, but it has > additional registers such as integrators, which is the main reason why I'm > offering a new driver.And I plan add drivers of the same series based on That is not a reason to add a separate driver. Look at, for example, lm90.c, which supports a variety of chips in a single driver. The ina2xx driver already does support several chips, and adding another one would be straightforward, even if it is from a different manufacturer. On top of that, only the EIN and ACCUM_CONFIG registers are additional, and the rest appear to be exactly the same as INA226. > this. I commit the new patch and look forward to your reply. > Additonal comments: - Please review and follow Documentation/process/submitting-patches.rst Documentation/hwmon/submitting-patches.rst - As mentioned before, a reworked version of the ina2xx.c driver is available. I am not inclined to accept a new driver for this chip. Even if I were, I would not accept a driver based on deprecated hwmon APIs. I would strongly advise to have a look into the reworked driver. As mentioned before, it is available in the hwmon-staging branch of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git. - "calculate_avg_power" is (unnecessarily) not a standard attribute and therefore unacceptable. Its value (on top of the already averaged power attribute) seems questionable. I would understand an attempt to report the energy, but I fail to understand the value in reporting yet another power average - even more so one that is not well defined in terms of number of samples used to determine the average. Guenter
Hi Wenliang, kernel test robot noticed the following build errors: [auto build test ERROR on linux/master] url: https://github.com/intel-lab-lkp/linux/commits/Wenliang/hwmon-add-new-hwmon-driver-sq52205/20240826-103932 base: linux/master patch link: https://lore.kernel.org/r/20240822074426.7241-1-wenliang202407%40163.com patch subject: [PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205 config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240826/202408261627.lm2xKacD-lkp@intel.com/config) compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240826/202408261627.lm2xKacD-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408261627.lm2xKacD-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/hwmon/sq52205.c:493:37: error: cannot assign to variable 'sq522xx_regmap_config' with const-qualified type 'const struct regmap_config' 493 | sq522xx_regmap_config.max_register = data->config->registers; | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ drivers/hwmon/sq52205.c:67:35: note: variable 'sq522xx_regmap_config' declared const here 67 | static const struct regmap_config sq522xx_regmap_config = { | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~ 68 | .reg_bits = 8, | ~~~~~~~~~~~~~~ 69 | .val_bits = 16, | ~~~~~~~~~~~~~~~ 70 | }; | ~ 1 error generated. vim +493 drivers/hwmon/sq52205.c 460 461 static int sq522xx_probe(struct i2c_client *client) 462 { 463 struct device *dev = &client->dev; 464 struct sq522xx_data *data; 465 struct device *hwmon_dev; 466 u32 val; 467 int ret, group = 0; 468 enum sq522xx_ids chip; 469 470 if (client->dev.of_node) 471 chip = (uintptr_t)of_device_get_match_data(&client->dev); 472 else 473 chip = i2c_match_id(sq522xx_id, client)->driver_data; 474 475 data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); 476 if (!data) 477 return -ENOMEM; 478 479 /* set the device type */ 480 data->client = client; 481 data->config = &sq522xx_config[chip]; 482 mutex_init(&data->config_lock); 483 484 if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) 485 val = SQ522XX_RSHUNT_DEFAULT; 486 487 488 if (val <= 0 || val > data->config->calibration_factor) 489 return -ENODEV; 490 491 data->rshunt = val; 492 > 493 sq522xx_regmap_config.max_register = data->config->registers; 494 495 data->regmap = devm_regmap_init_i2c(client, &sq522xx_regmap_config); 496 if (IS_ERR(data->regmap)) { 497 dev_err(dev, "failed to allocate register map\n"); 498 return PTR_ERR(data->regmap); 499 } 500 501 502 ret = sq522xx_init(data); 503 if (ret < 0) { 504 dev_err(dev, "error configuring the device: %d\n", ret); 505 return -ENODEV; 506 } 507 if (chip == sq52205) { 508 ret = sq52205_init(data); 509 if (ret < 0) { 510 dev_err(dev, "error configuring the device cal: %d\n", ret); 511 return -ENODEV; 512 } 513 } 514 515 data->groups[group++] = &sq522xx_group; 516 if (chip == sq52205) 517 data->groups[group++] = &sq52205_group; 518 519 hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name, 520 data, data->groups); 521 if (IS_ERR(hwmon_dev)) 522 return PTR_ERR(hwmon_dev); 523 524 dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n", 525 client->name, data->rshunt); 526 527 return 0; 528 } 529
Hi Wenliang, kernel test robot noticed the following build errors: [auto build test ERROR on linux/master] url: https://github.com/intel-lab-lkp/linux/commits/Wenliang/hwmon-add-new-hwmon-driver-sq52205/20240826-103932 base: linux/master patch link: https://lore.kernel.org/r/20240822074426.7241-1-wenliang202407%40163.com patch subject: [PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205 config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20240827/202408270025.5A0Q5KZO-lkp@intel.com/config) compiler: s390-linux-gcc (GCC) 14.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240827/202408270025.5A0Q5KZO-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408270025.5A0Q5KZO-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/hwmon/sq52205.c: In function 'sq522xx_probe': >> drivers/hwmon/sq52205.c:493:44: error: assignment of member 'max_register' in read-only object 493 | sq522xx_regmap_config.max_register = data->config->registers; | ^ vim +/max_register +493 drivers/hwmon/sq52205.c 460 461 static int sq522xx_probe(struct i2c_client *client) 462 { 463 struct device *dev = &client->dev; 464 struct sq522xx_data *data; 465 struct device *hwmon_dev; 466 u32 val; 467 int ret, group = 0; 468 enum sq522xx_ids chip; 469 470 if (client->dev.of_node) 471 chip = (uintptr_t)of_device_get_match_data(&client->dev); 472 else 473 chip = i2c_match_id(sq522xx_id, client)->driver_data; 474 475 data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); 476 if (!data) 477 return -ENOMEM; 478 479 /* set the device type */ 480 data->client = client; 481 data->config = &sq522xx_config[chip]; 482 mutex_init(&data->config_lock); 483 484 if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) 485 val = SQ522XX_RSHUNT_DEFAULT; 486 487 488 if (val <= 0 || val > data->config->calibration_factor) 489 return -ENODEV; 490 491 data->rshunt = val; 492 > 493 sq522xx_regmap_config.max_register = data->config->registers; 494 495 data->regmap = devm_regmap_init_i2c(client, &sq522xx_regmap_config); 496 if (IS_ERR(data->regmap)) { 497 dev_err(dev, "failed to allocate register map\n"); 498 return PTR_ERR(data->regmap); 499 } 500 501 502 ret = sq522xx_init(data); 503 if (ret < 0) { 504 dev_err(dev, "error configuring the device: %d\n", ret); 505 return -ENODEV; 506 } 507 if (chip == sq52205) { 508 ret = sq52205_init(data); 509 if (ret < 0) { 510 dev_err(dev, "error configuring the device cal: %d\n", ret); 511 return -ENODEV; 512 } 513 } 514 515 data->groups[group++] = &sq522xx_group; 516 if (chip == sq52205) 517 data->groups[group++] = &sq52205_group; 518 519 hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name, 520 data, data->groups); 521 if (IS_ERR(hwmon_dev)) 522 return PTR_ERR(hwmon_dev); 523 524 dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n", 525 client->name, data->rshunt); 526 527 return 0; 528 } 529
On 8/25/24 15:50, Guenter Roeck wrote: > On 8/22/24 00:44, Wenliang wrote: >> Thank you for bringing up your questions and suggestions.The SQ52205 is a >> part number specific to the Asian region, which is why you might not be >> able to find it through a search. I'll provide you the website >> (https://us1.silergy.com/zh/productsview/SQ52205FBP). > > That page does not point to the chip datasheet. The almost identical > page at https://us1.silergy.com/productsview/SQ52205FBP does. ... and that page is no longer available. Given that, if you really need support for this chip, you might want to consider adding support for SY24655 and/or SY24657 since public datasheets are available for those chips. The EIN and ACCUM_CONFIG registers in those chips match the definitions in your driver submission, so at least at first glance this should work. I noticed that there is also SY24656, which appears to be register compatible to INA226/INA230. Thanks, Guenter
Hi Wenliang, kernel test robot noticed the following build errors: [auto build test ERROR on linux/master] url: https://github.com/intel-lab-lkp/linux/commits/Wenliang/hwmon-add-new-hwmon-driver-sq52205/20240912-002906 base: linux/master patch link: https://lore.kernel.org/r/20240822074426.7241-1-wenliang202407%40163.com patch subject: [PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205 config: openrisc-randconfig-r072-20240913 (https://download.01.org/0day-ci/archive/20240913/202409131320.Ne0lQtTj-lkp@intel.com/config) compiler: or1k-linux-gcc (GCC) 14.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240913/202409131320.Ne0lQtTj-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409131320.Ne0lQtTj-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/hwmon/sq52205.c: In function 'sq522xx_probe': >> drivers/hwmon/sq52205.c:493:44: error: assignment of member 'max_register' in read-only object 493 | sq522xx_regmap_config.max_register = data->config->registers; | ^ vim +/max_register +493 drivers/hwmon/sq52205.c 460 461 static int sq522xx_probe(struct i2c_client *client) 462 { 463 struct device *dev = &client->dev; 464 struct sq522xx_data *data; 465 struct device *hwmon_dev; 466 u32 val; 467 int ret, group = 0; 468 enum sq522xx_ids chip; 469 470 if (client->dev.of_node) 471 chip = (uintptr_t)of_device_get_match_data(&client->dev); 472 else 473 chip = i2c_match_id(sq522xx_id, client)->driver_data; 474 475 data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); 476 if (!data) 477 return -ENOMEM; 478 479 /* set the device type */ 480 data->client = client; 481 data->config = &sq522xx_config[chip]; 482 mutex_init(&data->config_lock); 483 484 if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) 485 val = SQ522XX_RSHUNT_DEFAULT; 486 487 488 if (val <= 0 || val > data->config->calibration_factor) 489 return -ENODEV; 490 491 data->rshunt = val; 492 > 493 sq522xx_regmap_config.max_register = data->config->registers; 494 495 data->regmap = devm_regmap_init_i2c(client, &sq522xx_regmap_config); 496 if (IS_ERR(data->regmap)) { 497 dev_err(dev, "failed to allocate register map\n"); 498 return PTR_ERR(data->regmap); 499 } 500 501 502 ret = sq522xx_init(data); 503 if (ret < 0) { 504 dev_err(dev, "error configuring the device: %d\n", ret); 505 return -ENODEV; 506 } 507 if (chip == sq52205) { 508 ret = sq52205_init(data); 509 if (ret < 0) { 510 dev_err(dev, "error configuring the device cal: %d\n", ret); 511 return -ENODEV; 512 } 513 } 514 515 data->groups[group++] = &sq522xx_group; 516 if (chip == sq52205) 517 data->groups[group++] = &sq52205_group; 518 519 hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name, 520 data, data->groups); 521 if (IS_ERR(hwmon_dev)) 522 return PTR_ERR(hwmon_dev); 523 524 dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n", 525 client->name, data->rshunt); 526 527 return 0; 528 } 529
Hi Wenliang, kernel test robot noticed the following build errors: [auto build test ERROR on linux/master] url: https://github.com/intel-lab-lkp/linux/commits/Wenliang/hwmon-add-new-hwmon-driver-sq52205/20240912-002906 base: linux/master patch link: https://lore.kernel.org/r/20240822074426.7241-1-wenliang202407%40163.com patch subject: [PATCH linux dev 6.11] hwmon:add new hwmon driver sq52205 config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240914/202409140727.4pErU6oc-lkp@intel.com/config) compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project bf684034844c660b778f0eba103582f582b710c9) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409140727.4pErU6oc-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409140727.4pErU6oc-lkp@intel.com/ All errors (new ones prefixed by >>): In file included from drivers/hwmon/sq52205.c:12: In file included from include/linux/i2c.h:19: In file included from include/linux/regulator/consumer.h:35: In file included from include/linux/suspend.h:5: In file included from include/linux/swap.h:9: In file included from include/linux/memcontrol.h:13: In file included from include/linux/cgroup.h:25: In file included from include/linux/kernel_stat.h:8: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:14: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 548 | val = __raw_readb(PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); | ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu' 37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x)) | ^ In file included from drivers/hwmon/sq52205.c:12: In file included from include/linux/i2c.h:19: In file included from include/linux/regulator/consumer.h:35: In file included from include/linux/suspend.h:5: In file included from include/linux/swap.h:9: In file included from include/linux/memcontrol.h:13: In file included from include/linux/cgroup.h:25: In file included from include/linux/kernel_stat.h:8: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:14: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); | ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu' 35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x)) | ^ In file included from drivers/hwmon/sq52205.c:12: In file included from include/linux/i2c.h:19: In file included from include/linux/regulator/consumer.h:35: In file included from include/linux/suspend.h:5: In file included from include/linux/swap.h:9: In file included from include/linux/memcontrol.h:13: In file included from include/linux/cgroup.h:25: In file included from include/linux/kernel_stat.h:8: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:14: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 585 | __raw_writeb(value, PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); | ~~~~~~~~~~ ^ In file included from drivers/hwmon/sq52205.c:12: In file included from include/linux/i2c.h:19: In file included from include/linux/regulator/consumer.h:35: In file included from include/linux/suspend.h:5: In file included from include/linux/swap.h:9: In file included from include/linux/memcontrol.h:21: In file included from include/linux/mm.h:2232: include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion] 517 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_" | ~~~~~~~~~~~ ^ ~~~ >> drivers/hwmon/sq52205.c:493:37: error: cannot assign to variable 'sq522xx_regmap_config' with const-qualified type 'const struct regmap_config' 493 | sq522xx_regmap_config.max_register = data->config->registers; | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ drivers/hwmon/sq52205.c:67:35: note: variable 'sq522xx_regmap_config' declared const here 67 | static const struct regmap_config sq522xx_regmap_config = { | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~ 68 | .reg_bits = 8, | ~~~~~~~~~~~~~~ 69 | .val_bits = 16, | ~~~~~~~~~~~~~~~ 70 | }; | ~ 7 warnings and 1 error generated. vim +493 drivers/hwmon/sq52205.c 460 461 static int sq522xx_probe(struct i2c_client *client) 462 { 463 struct device *dev = &client->dev; 464 struct sq522xx_data *data; 465 struct device *hwmon_dev; 466 u32 val; 467 int ret, group = 0; 468 enum sq522xx_ids chip; 469 470 if (client->dev.of_node) 471 chip = (uintptr_t)of_device_get_match_data(&client->dev); 472 else 473 chip = i2c_match_id(sq522xx_id, client)->driver_data; 474 475 data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); 476 if (!data) 477 return -ENOMEM; 478 479 /* set the device type */ 480 data->client = client; 481 data->config = &sq522xx_config[chip]; 482 mutex_init(&data->config_lock); 483 484 if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) 485 val = SQ522XX_RSHUNT_DEFAULT; 486 487 488 if (val <= 0 || val > data->config->calibration_factor) 489 return -ENODEV; 490 491 data->rshunt = val; 492 > 493 sq522xx_regmap_config.max_register = data->config->registers; 494 495 data->regmap = devm_regmap_init_i2c(client, &sq522xx_regmap_config); 496 if (IS_ERR(data->regmap)) { 497 dev_err(dev, "failed to allocate register map\n"); 498 return PTR_ERR(data->regmap); 499 } 500 501 502 ret = sq522xx_init(data); 503 if (ret < 0) { 504 dev_err(dev, "error configuring the device: %d\n", ret); 505 return -ENODEV; 506 } 507 if (chip == sq52205) { 508 ret = sq52205_init(data); 509 if (ret < 0) { 510 dev_err(dev, "error configuring the device cal: %d\n", ret); 511 return -ENODEV; 512 } 513 } 514 515 data->groups[group++] = &sq522xx_group; 516 if (chip == sq52205) 517 data->groups[group++] = &sq52205_group; 518 519 hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name, 520 data, data->groups); 521 if (IS_ERR(hwmon_dev)) 522 return PTR_ERR(hwmon_dev); 523 524 dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n", 525 client->name, data->rshunt); 526 527 return 0; 528 } 529
On 10/8/24 04:58, wenliang yan wrote: > This change makes the INA2XX driver compatible with sy24655, > > involving the reading of average power > Your patches are corrupted. Also, please read and follow both Documentation/process/submitting-patches.rst and Documentation/hwmon/submitting-patches.rst. The most important violations are in both subject and description of your patches. Please use imperative mood as requested. Guenter
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst index 913c11390a45..5130270c8efe 100644 --- a/Documentation/hwmon/index.rst +++ b/Documentation/hwmon/index.rst @@ -219,6 +219,7 @@ Hardware Monitoring Kernel Drivers smsc47m1 sparx5-temp spd5118 + sq52205 stpddc60 surface_fan sy7636a-hwmon diff --git a/Documentation/hwmon/sq52205.rst b/Documentation/hwmon/sq52205.rst new file mode 100644 index 000000000000..e66bf883b42b --- /dev/null +++ b/Documentation/hwmon/sq52205.rst @@ -0,0 +1,44 @@ +SPDX-License-Identifier: GPL-2.0-only + +Kernel driver sq52205 +==================== + +Supported chips: + + * Silergy SQ52205 + + + Prefix: 'sq52205' + Addresses: I2C 0x40 - 0x4f + + Datasheet: Publicly available at the Silergy website + + https://us1.silergy.com/zh + + +Author: Wenliang Yan <wenliang202407@163.com> + +Description +----------- + +The SQ52205 is a high- and low-side current shunt and power monitor with an I2C +interface. The SQ52205 both shunt drop and supply voltage, with programmable +calibration value and conversion times. The SQ52205 can also calculate average +power for use in energy conversion. + + + +Sysfs entries +--------------------- + +======================= =============================== +in0_input Shunt voltage(mV) channel +in1_input Bus voltage(mV) channel +curr1_input Current(mA) measurement channel +power1_input Power(uW) measurement channel +shunt_resistor Shunt resistance(uOhm) channel +update_interval data conversion time; affects number of samples used + to average results for shunt and bus voltages. +calculate_avg_power calculate average power from last reading to the present +======================= =============================== + diff --git a/MAINTAINERS b/MAINTAINERS index 8766f3e5e87e..4794c703da34 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -20822,6 +20822,11 @@ S: Maintained F: drivers/input/touchscreen/silead.c F: drivers/platform/x86/touchscreen_dmi.c +SILERGY HARDWARE MONITOR DRIVER +M: Wenliang Yan <wenliang202407@163.com> +S: Maintained +F: drivers/hwmon/sq52205.c + SILICON LABS WIRELESS DRIVERS (for WFxxx series) M: Jérôme Pouiller <jerome.pouiller@silabs.com> S: Supported diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index b60fe2e58ad6..7a6c21c9eee9 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -2521,6 +2521,19 @@ config SENSORS_INTEL_M10_BMC_HWMON sensors monitor various telemetry data of different components on the card, e.g. board temperature, FPGA core temperature/voltage/current. +config SENSORS_SQ52205 + tristate "Silergy SQ52205 and compatibles" + depends on I2C + select REGMAP_I2C + help + If you say yes here you get support for SQ52205 power monitor chips. + + The SQ52205 driver is configured for the default configuration of + the part as described in the datasheet. + Default value for Rshunt is 10 mOhms. + This driver can also be built as a module. If so, the module + will be called sq52205. + if ACPI comment "ACPI drivers" diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index b1c7056c37db..270f88e3c6e6 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -207,6 +207,7 @@ obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o obj-$(CONFIG_SENSORS_SPD5118) += spd5118.o +obj-$(CONFIG_SENSORS_SQ52205) += sq52205.o obj-$(CONFIG_SENSORS_STTS751) += stts751.o obj-$(CONFIG_SENSORS_SURFACE_FAN)+= surface_fan.o obj-$(CONFIG_SENSORS_SY7636A) += sy7636a-hwmon.o diff --git a/drivers/hwmon/sq52205.c b/drivers/hwmon/sq52205.c new file mode 100644 index 000000000000..a7a674a50289 --- /dev/null +++ b/drivers/hwmon/sq52205.c @@ -0,0 +1,558 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Driver for SQ52205 power monitor chip + * + * Copyright (C) 2024 Wenliang Yan <wenliang202407@163.com> + */ +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/init.h> +#include <linux/err.h> +#include <linux/slab.h> +#include <linux/i2c.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/jiffies.h> +#include <linux/of.h> +#include <linux/delay.h> +#include <linux/util_macros.h> +#include <linux/regmap.h> + + + +/* common register definitions */ +#define SQ522XX_CONFIG 0x00 +#define SQ522XX_SHUNT_VOLTAGE 0x01 /* readonly */ +#define SQ522XX_BUS_VOLTAGE 0x02 /* readonly */ +#define SQ522XX_POWER 0x03 /* readonly */ +#define SQ522XX_CURRENT 0x04 /* readonly */ +#define SQ522XX_CALIBRATION 0x05 + +/* SQ52205 register definitions */ +#define SQ52205_MASK_ENABLE 0x06 +#define SQ52205_ALERT_LIMIT 0x07 +#define SQ52205_EIN 0x0A +#define SQ52205_ACCUM_CONFIG 0x0D + +/* register count */ +#define SQ52205_REGISTERS 0x0D +#define SQ522XX_MAX_REGISTERS 0x0D + +/* settings - default */ +#define SQ52205_CONFIG_DEFAULT 0x4527 /* averages=16 */ +#define SQ52205_ACCUM_CONFIG_DEFAULT 0x044C + +/* worst case is 68.10 ms (~14.6Hz) */ +#define SQ522XX_CONVERSION_RATE 15 +#define SQ522XX_MAX_DELAY 69 /* worst case delay in ms */ + +#define SQ522XX_RSHUNT_DEFAULT 10000 //10mOhms +#define SQ52205_BUS_VOLTAGE_LSB 1250 //1.25mV + +/* bit mask for reading the averaging setting in the configuration register */ +#define SQ52205_AVG_RD_MASK 0x0E00 + +#define SQ52205_READ_AVG(reg) (((reg) & SQ52205_AVG_RD_MASK) >> 9) +#define SQ52205_SHIFT_AVG(val) ((val) << 9) + +/* common attrs, sq52205 attrs and NULL */ +#define SQ522XX_MAX_ATTRIBUTE_GROUPS 3 + +/* + * Both bus voltage and shunt voltage conversion times for sq52205 are set + * to 0b0100 on POR, which translates to 2200 microseconds in total. + */ +#define SQ52205_TOTAL_CONV_TIME_DEFAULT 2200 + +static const struct regmap_config sq522xx_regmap_config = { + .reg_bits = 8, + .val_bits = 16, +}; + +enum sq522xx_ids { sq52205 }; + +struct sq522xx_config { + u16 config_default; + int calibration_factor; + int registers; + int shunt_div; + int bus_voltage_shift; + int bus_voltage_lsb; /* uV */ + int power_lsb; /* uW */ +}; + +struct sq522xx_data { + const struct sq522xx_config *config; + + long rshunt; + + struct mutex config_lock; + struct i2c_client *client; + struct regmap *regmap; + + const struct attribute_group *groups[SQ522XX_MAX_ATTRIBUTE_GROUPS]; +}; + +static const struct sq522xx_config sq522xx_config[] = { + [sq52205] = { + .config_default = SQ52205_CONFIG_DEFAULT, + .calibration_factor = 5120000, + .registers = SQ52205_REGISTERS, + .shunt_div = 400, + .bus_voltage_shift = 0, + .bus_voltage_lsb = SQ52205_BUS_VOLTAGE_LSB, + .power_lsb = 25000, + }, +}; + +/* + * Available averaging rates for sq52205. The indices correspond with + * the bit values expected by the chip (according to the sq52205 datasheet) + */ +static const int sq52205_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 }; + +static int sq52205_reg_to_interval(u16 config) +{ + int avg = sq52205_avg_tab[SQ52205_READ_AVG(config)]; + + /* + * Multiply the total conversion time by the number of averages. + * Return the result in milliseconds. + */ + return DIV_ROUND_CLOSEST(avg * SQ52205_TOTAL_CONV_TIME_DEFAULT, 1000); +} + +/* + * Return the new, shifted AVG field value of CONFIG register, + * to use with regmap_update_bits + */ +static u16 sq52205_interval_to_reg(int interval) +{ + int avg, avg_bits; + + avg = DIV_ROUND_CLOSEST(interval * 1000, + SQ52205_TOTAL_CONV_TIME_DEFAULT); + avg_bits = find_closest(avg, sq52205_avg_tab, + ARRAY_SIZE(sq52205_avg_tab)); + + return SQ52205_SHIFT_AVG(avg_bits); +} + +/* + * Calibration register is set to the best value, which eliminates + * truncation errors on calculating current register in hardware. + * According to datasheet the best values are 2048 for + * sq52205. They are hardcoded as calibration_value. + */ +static int sq522xx_calibrate(struct sq522xx_data *data) +{ + u16 val = DIV_ROUND_CLOSEST(data->config->calibration_factor, + data->rshunt); + + return regmap_write(data->regmap, SQ522XX_CALIBRATION, + val); +} + +/* + * Initialize the configuration and calibration registers. + */ +static int sq522xx_init(struct sq522xx_data *data) +{ + int ret = regmap_write(data->regmap, SQ522XX_CONFIG, + data->config->config_default); + if (ret < 0) + return ret; + + return sq522xx_calibrate(data); +} +static int sq52205_init(struct sq522xx_data *data) +{ + // configure ENABLE register + int ret = regmap_write(data->regmap, SQ52205_ACCUM_CONFIG, + SQ52205_ACCUM_CONFIG_DEFAULT); + if (ret < 0) + return ret; + + return 0; +} + +static int sq522xx_read_reg(struct device *dev, int reg, unsigned int *regval) +{ + struct sq522xx_data *data = dev_get_drvdata(dev); + int ret, retry; + + dev_dbg(dev, "Starting register %d read\n", reg); + + for (retry = 5; retry; retry--) { + + ret = regmap_read(data->regmap, reg, regval); + if (ret < 0) + return ret; + + dev_dbg(dev, "read %d, val = 0x%04x\n", reg, *regval); + + /* + * If the current value in the calibration register is 0, the + * power and current registers will also remain at 0. In case + * the chip has been reset let's check the calibration + * register and reinitialize if needed. + * We do that extra read of the calibration register if there + * is some hint of a chip reset. + */ + if (*regval == 0) { + unsigned int cal; + + ret = regmap_read(data->regmap, SQ522XX_CALIBRATION, + &cal); + if (ret < 0) + return ret; + + if (cal == 0) { + dev_warn(dev, "chip not calibrated, reinitializing\n"); + + ret = sq522xx_init(data); + if (ret < 0) + return ret; + /* + * Let's make sure the power and current + * registers have been updated before trying + * again. + */ + msleep(SQ522XX_MAX_DELAY); + continue; + } + } + return 0; + } + + /* + * If we're here then although all write operations succeeded, the + * chip still returns 0 in the calibration register. Nothing more we + * can do here. + */ + dev_err(dev, "unable to reinitialize the chip\n"); + return -ENODEV; +} + +static int sq522xx_get_value(struct sq522xx_data *data, u8 reg, + unsigned int regval) +{ + int val; + + switch (reg) { + case SQ522XX_SHUNT_VOLTAGE: + /* signed register , value is in mV*/ + val = DIV_ROUND_CLOSEST((s16)regval, data->config->shunt_div); + break; + case SQ522XX_BUS_VOLTAGE: + /* signed register , value is in mV*/ + val = (regval >> data->config->bus_voltage_shift) + * data->config->bus_voltage_lsb; + val = DIV_ROUND_CLOSEST(val, 1000); + break; + case SQ522XX_POWER: + /* value is in uV*/ + val = regval * data->config->power_lsb; + break; + case SQ522XX_CURRENT: + /* signed register, LSB=1mA (selected), in mA */ + val = (s16)regval; + break; + case SQ522XX_CALIBRATION: + val = DIV_ROUND_CLOSEST(data->config->calibration_factor, + regval); + break; + default: + /* programmer goofed */ + WARN_ON_ONCE(1); + val = 0; + break; + } + + return val; +} + +static ssize_t sq522xx_value_show(struct device *dev, + struct device_attribute *da, char *buf) +{ + struct sensor_device_attribute *attr = to_sensor_dev_attr(da); + struct sq522xx_data *data = dev_get_drvdata(dev); + unsigned int regval; + + int err = sq522xx_read_reg(dev, attr->index, ®val); + + if (err < 0) + return err; + + return sysfs_emit(buf, "%d\n", sq522xx_get_value(data, attr->index, regval)); +} + + +/* + * In order to keep calibration register value fixed, the product + * of current_lsb and shunt_resistor should also be fixed and equal + * to shunt_voltage_lsb = 1 / shunt_div multiplied by 10^9 in order + * to keep the scale. + */ +static ssize_t sq522xx_shunt_store(struct device *dev, + struct device_attribute *da, + const char *buf, size_t count) +{ + unsigned long val; + int status; + struct sq522xx_data *data = dev_get_drvdata(dev); + + status = kstrtoul(buf, 10, &val); + if (status < 0) + return status; + /* Values greater than the calibration factor make no sense. */ + if (val == 0 || val > data->config->calibration_factor) + return -EINVAL; + + mutex_lock(&data->config_lock); + data->rshunt = val; + + status = sq522xx_calibrate(data); + mutex_unlock(&data->config_lock); + if (status < 0) + return status; + + return count; +} + +static ssize_t sq522xx_shunt_show(struct device *dev, + struct device_attribute *da, char *buf) +{ + struct sq522xx_data *data = dev_get_drvdata(dev); + + return sysfs_emit(buf, "%li\n", data->rshunt); +} + + + +static ssize_t sq52205_interval_store(struct device *dev, + struct device_attribute *da, + const char *buf, size_t count) +{ + struct sq522xx_data *data = dev_get_drvdata(dev); + unsigned long val; + int status; + + status = kstrtoul(buf, 10, &val); + if (status < 0) + return status; + + if (val > INT_MAX || val == 0) + return -EINVAL; + + status = regmap_update_bits(data->regmap, SQ522XX_CONFIG, + SQ52205_AVG_RD_MASK, + sq52205_interval_to_reg(val)); + if (status < 0) + return status; + + return count; +} + +static ssize_t sq52205_interval_show(struct device *dev, + struct device_attribute *da, char *buf) +{ + struct sq522xx_data *data = dev_get_drvdata(dev); + int status; + unsigned int regval; + + status = regmap_read(data->regmap, SQ522XX_CONFIG, ®val); + if (status) + return status; + + return sysfs_emit(buf, "%d\n", sq52205_reg_to_interval(regval)); +} + +static int sq52205_read_reg48(const struct i2c_client *client, u8 reg, + long *accumulator_24, long *sample_count) +{ + u8 data[6]; + int err; + *accumulator_24 = 0; + *sample_count = 0; + + /* 48-bit register read */ + err = i2c_smbus_read_i2c_block_data(client, reg, 6, data); + if (err < 0) + return err; + if (err != 6) + return -EIO; + *accumulator_24 = ((data[3] << 16) | + (data[4] << 8) | + data[5]); + *sample_count = ((data[0] << 16) | + (data[1] << 8) | + data[2]); + + return 0; +} + +static ssize_t sq52205_avg_power_show(struct device *dev, + struct device_attribute *da, char *buf) +{ + struct sq522xx_data *data = dev_get_drvdata(dev); + long sample_count, accumulator_24, regval; + int status; + + status = sq52205_read_reg48(data->client, SQ52205_EIN, + &accumulator_24, &sample_count); + if (status) + return status; + regval = DIV_ROUND_CLOSEST(accumulator_24, sample_count); + regval = regval * data->config->power_lsb; + + + return sysfs_emit(buf, "%li\n", regval); +} + +/* shunt voltage */ +static SENSOR_DEVICE_ATTR_RO(in0_input, sq522xx_value, SQ522XX_SHUNT_VOLTAGE); + +/* bus voltage */ +static SENSOR_DEVICE_ATTR_RO(in1_input, sq522xx_value, SQ522XX_BUS_VOLTAGE); + +/* calculated current */ +static SENSOR_DEVICE_ATTR_RO(curr1_input, sq522xx_value, SQ522XX_CURRENT); + +/* calculated power */ +static SENSOR_DEVICE_ATTR_RO(power1_input, sq522xx_value, SQ522XX_POWER); + +/* shunt resistance */ +static SENSOR_DEVICE_ATTR_RW(shunt_resistor, sq522xx_shunt, SQ522XX_CALIBRATION); + +/* update interval (sq52205 only) */ +static SENSOR_DEVICE_ATTR_RW(update_interval, sq52205_interval, 0); + +/* calculate_avg_power (sq52205 only) */ +static SENSOR_DEVICE_ATTR_RO(calculate_avg_power, sq52205_avg_power, 0); + + +/* pointers to created device attributes */ +static struct attribute *sq522xx_attrs[] = { + &sensor_dev_attr_in0_input.dev_attr.attr, + &sensor_dev_attr_in1_input.dev_attr.attr, + &sensor_dev_attr_curr1_input.dev_attr.attr, + &sensor_dev_attr_power1_input.dev_attr.attr, + &sensor_dev_attr_shunt_resistor.dev_attr.attr, + NULL, +}; + +static const struct attribute_group sq522xx_group = { + .attrs = sq522xx_attrs, +}; + +static struct attribute *sq52205_attrs[] = { + &sensor_dev_attr_update_interval.dev_attr.attr, + &sensor_dev_attr_calculate_avg_power.dev_attr.attr, + NULL, +}; + +static const struct attribute_group sq52205_group = { + .attrs = sq52205_attrs, +}; + +static const struct i2c_device_id sq522xx_id[]; + +static int sq522xx_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct sq522xx_data *data; + struct device *hwmon_dev; + u32 val; + int ret, group = 0; + enum sq522xx_ids chip; + + if (client->dev.of_node) + chip = (uintptr_t)of_device_get_match_data(&client->dev); + else + chip = i2c_match_id(sq522xx_id, client)->driver_data; + + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + /* set the device type */ + data->client = client; + data->config = &sq522xx_config[chip]; + mutex_init(&data->config_lock); + + if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) + val = SQ522XX_RSHUNT_DEFAULT; + + + if (val <= 0 || val > data->config->calibration_factor) + return -ENODEV; + + data->rshunt = val; + + sq522xx_regmap_config.max_register = data->config->registers; + + data->regmap = devm_regmap_init_i2c(client, &sq522xx_regmap_config); + if (IS_ERR(data->regmap)) { + dev_err(dev, "failed to allocate register map\n"); + return PTR_ERR(data->regmap); + } + + + ret = sq522xx_init(data); + if (ret < 0) { + dev_err(dev, "error configuring the device: %d\n", ret); + return -ENODEV; + } + if (chip == sq52205) { + ret = sq52205_init(data); + if (ret < 0) { + dev_err(dev, "error configuring the device cal: %d\n", ret); + return -ENODEV; + } + } + + data->groups[group++] = &sq522xx_group; + if (chip == sq52205) + data->groups[group++] = &sq52205_group; + + hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name, + data, data->groups); + if (IS_ERR(hwmon_dev)) + return PTR_ERR(hwmon_dev); + + dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n", + client->name, data->rshunt); + + return 0; +} + +static const struct i2c_device_id sq522xx_id[] = { + { "sq52205", sq52205 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, sq522xx_id); + +static const struct of_device_id __maybe_unused sq522xx_of_match[] = { + { + .compatible = "silergy,sq52205", + .data = (void *)sq52205 + }, + { }, +}; +MODULE_DEVICE_TABLE(of, sq522xx_of_match); + +static struct i2c_driver sq522xx_driver = { + .driver = { + .name = "sq522xx", + .of_match_table = of_match_ptr(sq522xx_of_match), + }, + .probe = sq522xx_probe, + .id_table = sq522xx_id, +}; + +module_i2c_driver(sq522xx_driver); + +MODULE_AUTHOR(" <wenliang202407@163.com> "); +MODULE_DESCRIPTION("SQ522xx driver"); +MODULE_LICENSE("GPL");
Thank you for bringing up your questions and suggestions.The SQ52205 is a part number specific to the Asian region, which is why you might not be able to find it through a search. I'll provide you the website (https://us1.silergy.com/zh/productsview/SQ52205FBP). Some registers of this chip are similar to those of the INA226, but it has additional registers such as integrators, which is the main reason why I'm offering a new driver.And I plan add drivers of the same series based on this. I commit the new patch and look forward to your reply. Signed-off-by: Wenliang <wenliang202407@163.com> --- Documentation/hwmon/index.rst | 1 + Documentation/hwmon/sq52205.rst | 44 +++ MAINTAINERS | 5 + drivers/hwmon/Kconfig | 13 + drivers/hwmon/Makefile | 1 + drivers/hwmon/sq52205.c | 558 ++++++++++++++++++++++++++++++++ 6 files changed, 622 insertions(+) create mode 100644 Documentation/hwmon/sq52205.rst create mode 100644 drivers/hwmon/sq52205.c