* Re: [PATCH 2/2] drivers: hwmon: add driver for max42500
@ 2024-12-22 23:15 kernel test robot
0 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2024-12-22 23:15 UTC (permalink / raw)
Cc: oe-kbuild-all, llvm
In-Reply-To: <20241220012003.9568-3-Kent.Libetario@analog.com>
References: <20241220012003.9568-3-Kent.Libetario@analog.com>
TO: Kent Libetario <Kent.Libetario@analog.com>
TO: linux-hwmon@vger.kernel.org
TO: devicetree@vger.kernel.org
TO: linux-kernel@vger.kernel.org
CC: Jean Delvare <jdelvare@suse.com>
CC: Guenter Roeck <linux@roeck-us.net>
CC: Rob Herring <robh@kernel.org>
CC: Krzysztof Kozlowski <krzk@kernel.org>
CC: Conor Dooley <conor+dt@kernel.org>
Hi Kent,
kernel test robot noticed the following build warnings:
[auto build test WARNING on de076198d1e4934c5fc17aa52d5f1884f469ce1a]
url: https://github.com/intel-lab-lkp/linux/commits/Kent-Libetario/dt-bindings-hwmon-add-adi-max42500-yaml/20241220-092728
base: de076198d1e4934c5fc17aa52d5f1884f469ce1a
patch link: https://lore.kernel.org/r/20241220012003.9568-3-Kent.Libetario%40analog.com
patch subject: [PATCH 2/2] drivers: hwmon: add driver for max42500
config: x86_64-randconfig-003-20241223 (https://download.01.org/0day-ci/archive/20241223/202412230626.JsC2Jzh5-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241223/202412230626.JsC2Jzh5-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/202412230626.JsC2Jzh5-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/hwmon/max42500.c:18:
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:2223:
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> drivers/hwmon/max42500.c:201:33: warning: arithmetic between different enumeration types ('enum max42500_vm_input' and 'enum max42500_comp_stat') [-Wenum-enum-conversion]
201 | u8 comp_status[MAX42500_VM_MAX * MAX42500_COMP_STAT_MAX];
| ~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
vim +201 drivers/hwmon/max42500.c
189
190 struct max42500_state {
191 struct i2c_client *client;
192 struct regmap *regmap;
193 struct max42500_config *config;
194 long pwrup_stamp[MAX42500_VM_MAX];
195 long pwrdn_stamp[MAX42500_VM_MAX];
196 u8 ov_thresh1[MAX42500_VM_MAX - 2];
197 u8 ov_thresh2[MAX42500_VM_MAX - 5];
198 u8 uv_thresh1[MAX42500_VM_MAX - 2];
199 u8 uv_thresh2[MAX42500_VM_MAX - 5];
200 u8 nominal_volt[MAX42500_VM_MAX - 2];
> 201 u8 comp_status[MAX42500_VM_MAX * MAX42500_COMP_STAT_MAX];
202 };
203
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] drivers: hwmon: add driver for max42500
@ 2024-12-21 22:16 kernel test robot
0 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2024-12-21 22:16 UTC (permalink / raw)
To: oe-kbuild; +Cc: lkp, Dan Carpenter
BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20241220012003.9568-3-Kent.Libetario@analog.com>
References: <20241220012003.9568-3-Kent.Libetario@analog.com>
TO: Kent Libetario <Kent.Libetario@analog.com>
TO: linux-hwmon@vger.kernel.org
TO: devicetree@vger.kernel.org
TO: linux-kernel@vger.kernel.org
CC: Jean Delvare <jdelvare@suse.com>
CC: Guenter Roeck <linux@roeck-us.net>
CC: Rob Herring <robh@kernel.org>
CC: Krzysztof Kozlowski <krzk@kernel.org>
CC: Conor Dooley <conor+dt@kernel.org>
Hi Kent,
kernel test robot noticed the following build warnings:
[auto build test WARNING on de076198d1e4934c5fc17aa52d5f1884f469ce1a]
url: https://github.com/intel-lab-lkp/linux/commits/Kent-Libetario/dt-bindings-hwmon-add-adi-max42500-yaml/20241220-092728
base: de076198d1e4934c5fc17aa52d5f1884f469ce1a
patch link: https://lore.kernel.org/r/20241220012003.9568-3-Kent.Libetario%40analog.com
patch subject: [PATCH 2/2] drivers: hwmon: add driver for max42500
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: x86_64-randconfig-r071-20241221 (https://download.01.org/0day-ci/archive/20241222/202412220556.U9kI8Oix-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
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>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202412220556.U9kI8Oix-lkp@intel.com/
New smatch warnings:
drivers/hwmon/max42500.c:312 max42500_set_nominal_voltage() warn: impossible condition '(voltage > 255) => (0-255 > 255)'
drivers/hwmon/max42500.c:427 max42500_set_ov_thresh2() warn: impossible condition '(thresh > 255) => (0-255 > 255)'
drivers/hwmon/max42500.c:482 max42500_set_uv_thresh2() warn: impossible condition '(thresh > 255) => (0-255 > 255)'
Old smatch warnings:
drivers/hwmon/max42500.c:318 max42500_set_nominal_voltage() warn: impossible condition '(voltage > 255) => (0-255 > 255)'
vim +312 drivers/hwmon/max42500.c
38dfb309423437 Kent Libetario 2024-12-20 296
38dfb309423437 Kent Libetario 2024-12-20 297 /**
38dfb309423437 Kent Libetario 2024-12-20 298 * @brief Set nominal voltage for VM1 to VM5.
38dfb309423437 Kent Libetario 2024-12-20 299 * @return 0 in case of success, negative error code otherwise.
38dfb309423437 Kent Libetario 2024-12-20 300 */
38dfb309423437 Kent Libetario 2024-12-20 301 static int max42500_set_nominal_voltage(struct max42500_state *st,
38dfb309423437 Kent Libetario 2024-12-20 302 enum max42500_vm_input vm_in, u8 voltage)
38dfb309423437 Kent Libetario 2024-12-20 303 {
38dfb309423437 Kent Libetario 2024-12-20 304 u8 reg_addr;
38dfb309423437 Kent Libetario 2024-12-20 305
38dfb309423437 Kent Libetario 2024-12-20 306 switch (vm_in) {
38dfb309423437 Kent Libetario 2024-12-20 307 case MAX42500_VM1:
38dfb309423437 Kent Libetario 2024-12-20 308 case MAX42500_VM2:
38dfb309423437 Kent Libetario 2024-12-20 309 case MAX42500_VM3:
38dfb309423437 Kent Libetario 2024-12-20 310 case MAX42500_VM4:
38dfb309423437 Kent Libetario 2024-12-20 311 if (voltage < MAX42500_MIN_VNOM ||
38dfb309423437 Kent Libetario 2024-12-20 @312 voltage > MAX42500_VNOM_MAX_VM1_VM4)
38dfb309423437 Kent Libetario 2024-12-20 313 return -EINVAL;
38dfb309423437 Kent Libetario 2024-12-20 314 reg_addr = MAX42500_REG_VIN1 + vm_in;
38dfb309423437 Kent Libetario 2024-12-20 315 break;
38dfb309423437 Kent Libetario 2024-12-20 316 case MAX42500_VM5:
38dfb309423437 Kent Libetario 2024-12-20 317 if (voltage < MAX42500_MIN_VNOM ||
38dfb309423437 Kent Libetario 2024-12-20 318 voltage > MAX42500_VNOM_MAX_VM5)
38dfb309423437 Kent Libetario 2024-12-20 319 return -EINVAL;
38dfb309423437 Kent Libetario 2024-12-20 320 reg_addr = MAX42500_REG_VIN5;
38dfb309423437 Kent Libetario 2024-12-20 321 break;
38dfb309423437 Kent Libetario 2024-12-20 322 default:
38dfb309423437 Kent Libetario 2024-12-20 323 return -EINVAL;
38dfb309423437 Kent Libetario 2024-12-20 324 }
38dfb309423437 Kent Libetario 2024-12-20 325
38dfb309423437 Kent Libetario 2024-12-20 326 st->nominal_volt[vm_in] = voltage;
38dfb309423437 Kent Libetario 2024-12-20 327 return max42500_reg_write(st, reg_addr, voltage);
38dfb309423437 Kent Libetario 2024-12-20 328 }
38dfb309423437 Kent Libetario 2024-12-20 329
38dfb309423437 Kent Libetario 2024-12-20 330 /**
38dfb309423437 Kent Libetario 2024-12-20 331 * @brief Get the status of the voltage monitor input.
38dfb309423437 Kent Libetario 2024-12-20 332 * @return 0 in case of success, negative error code otherwise.
38dfb309423437 Kent Libetario 2024-12-20 333 */
38dfb309423437 Kent Libetario 2024-12-20 334 static int max42500_get_comp_status(struct max42500_state *st,
38dfb309423437 Kent Libetario 2024-12-20 335 u8 vm_in, u8 *status)
38dfb309423437 Kent Libetario 2024-12-20 336 {
38dfb309423437 Kent Libetario 2024-12-20 337 int ret;
38dfb309423437 Kent Libetario 2024-12-20 338 u8 reg_addr;
38dfb309423437 Kent Libetario 2024-12-20 339 u8 vm_in_status;
38dfb309423437 Kent Libetario 2024-12-20 340
38dfb309423437 Kent Libetario 2024-12-20 341 switch (vm_in % MAX42500_COMP_STAT_MAX) {
38dfb309423437 Kent Libetario 2024-12-20 342 case MAX42500_COMP_STAT_OFF:
38dfb309423437 Kent Libetario 2024-12-20 343 reg_addr = MAX42500_REG_STATOFF;
38dfb309423437 Kent Libetario 2024-12-20 344 break;
38dfb309423437 Kent Libetario 2024-12-20 345 case MAX42500_COMP_STAT_UV:
38dfb309423437 Kent Libetario 2024-12-20 346 reg_addr = MAX42500_REG_STATUV;
38dfb309423437 Kent Libetario 2024-12-20 347 break;
38dfb309423437 Kent Libetario 2024-12-20 348 case MAX42500_COMP_STAT_OV:
38dfb309423437 Kent Libetario 2024-12-20 349 reg_addr = MAX42500_REG_STATOV;
38dfb309423437 Kent Libetario 2024-12-20 350 break;
38dfb309423437 Kent Libetario 2024-12-20 351 default:
38dfb309423437 Kent Libetario 2024-12-20 352 return -EINVAL;
38dfb309423437 Kent Libetario 2024-12-20 353 }
38dfb309423437 Kent Libetario 2024-12-20 354
38dfb309423437 Kent Libetario 2024-12-20 355 ret = max42500_reg_read(st, reg_addr, &vm_in_status);
38dfb309423437 Kent Libetario 2024-12-20 356 if (ret)
38dfb309423437 Kent Libetario 2024-12-20 357 return ret;
38dfb309423437 Kent Libetario 2024-12-20 358
38dfb309423437 Kent Libetario 2024-12-20 359 switch (vm_in % MAX42500_VM_MAX) {
38dfb309423437 Kent Libetario 2024-12-20 360 case MAX42500_VM1:
38dfb309423437 Kent Libetario 2024-12-20 361 *status = (u8)FIELD_GET(BIT(MAX42500_VM1), vm_in_status);
38dfb309423437 Kent Libetario 2024-12-20 362 break;
38dfb309423437 Kent Libetario 2024-12-20 363 case MAX42500_VM2:
38dfb309423437 Kent Libetario 2024-12-20 364 *status = (u8)FIELD_GET(BIT(MAX42500_VM2), vm_in_status);
38dfb309423437 Kent Libetario 2024-12-20 365 break;
38dfb309423437 Kent Libetario 2024-12-20 366 case MAX42500_VM3:
38dfb309423437 Kent Libetario 2024-12-20 367 *status = (u8)FIELD_GET(BIT(MAX42500_VM3), vm_in_status);
38dfb309423437 Kent Libetario 2024-12-20 368 break;
38dfb309423437 Kent Libetario 2024-12-20 369 case MAX42500_VM4:
38dfb309423437 Kent Libetario 2024-12-20 370 *status = (u8)FIELD_GET(BIT(MAX42500_VM4), vm_in_status);
38dfb309423437 Kent Libetario 2024-12-20 371 break;
38dfb309423437 Kent Libetario 2024-12-20 372 case MAX42500_VM5:
38dfb309423437 Kent Libetario 2024-12-20 373 *status = (u8)FIELD_GET(BIT(MAX42500_VM5), vm_in_status);
38dfb309423437 Kent Libetario 2024-12-20 374 break;
38dfb309423437 Kent Libetario 2024-12-20 375 case MAX42500_VM6:
38dfb309423437 Kent Libetario 2024-12-20 376 *status = (u8)FIELD_GET(BIT(MAX42500_VM6), vm_in_status);
38dfb309423437 Kent Libetario 2024-12-20 377 break;
38dfb309423437 Kent Libetario 2024-12-20 378 case MAX42500_VM7:
38dfb309423437 Kent Libetario 2024-12-20 379 *status = (u8)FIELD_GET(BIT(MAX42500_VM7), vm_in_status);
38dfb309423437 Kent Libetario 2024-12-20 380 break;
38dfb309423437 Kent Libetario 2024-12-20 381 default:
38dfb309423437 Kent Libetario 2024-12-20 382 return -EINVAL;
38dfb309423437 Kent Libetario 2024-12-20 383 }
38dfb309423437 Kent Libetario 2024-12-20 384 st->comp_status[vm_in] = *status;
38dfb309423437 Kent Libetario 2024-12-20 385
38dfb309423437 Kent Libetario 2024-12-20 386 return 0;
38dfb309423437 Kent Libetario 2024-12-20 387 }
38dfb309423437 Kent Libetario 2024-12-20 388
38dfb309423437 Kent Libetario 2024-12-20 389 /**
38dfb309423437 Kent Libetario 2024-12-20 390 * @brief Set the overvoltage threshold of VM1 to VM5.
38dfb309423437 Kent Libetario 2024-12-20 391 * @return 0 in case of success, negative error code otherwise.
38dfb309423437 Kent Libetario 2024-12-20 392 */
38dfb309423437 Kent Libetario 2024-12-20 393 static int max42500_set_ov_thresh1(struct max42500_state *st,
38dfb309423437 Kent Libetario 2024-12-20 394 enum max42500_vm_input vm_in, u8 thresh)
38dfb309423437 Kent Libetario 2024-12-20 395 {
38dfb309423437 Kent Libetario 2024-12-20 396 if (thresh < MAX42500_MIN_THRESH_VM1_VM5 ||
38dfb309423437 Kent Libetario 2024-12-20 397 thresh > MAX42500_MAX_THRESH_VM1_VM5)
38dfb309423437 Kent Libetario 2024-12-20 398 return -EINVAL;
38dfb309423437 Kent Libetario 2024-12-20 399
38dfb309423437 Kent Libetario 2024-12-20 400 switch (vm_in) {
38dfb309423437 Kent Libetario 2024-12-20 401 case MAX42500_VM1:
38dfb309423437 Kent Libetario 2024-12-20 402 case MAX42500_VM2:
38dfb309423437 Kent Libetario 2024-12-20 403 case MAX42500_VM3:
38dfb309423437 Kent Libetario 2024-12-20 404 case MAX42500_VM4:
38dfb309423437 Kent Libetario 2024-12-20 405 case MAX42500_VM5:
38dfb309423437 Kent Libetario 2024-12-20 406 st->ov_thresh1[vm_in] = thresh;
38dfb309423437 Kent Libetario 2024-12-20 407 return max42500_reg_update(st,
38dfb309423437 Kent Libetario 2024-12-20 408 MAX42500_REG_OVUV1 + vm_in,
38dfb309423437 Kent Libetario 2024-12-20 409 GENMASK(7, 4),
38dfb309423437 Kent Libetario 2024-12-20 410 FIELD_PREP(GENMASK(7, 4),
38dfb309423437 Kent Libetario 2024-12-20 411 thresh));
38dfb309423437 Kent Libetario 2024-12-20 412 default:
38dfb309423437 Kent Libetario 2024-12-20 413 return -EINVAL;
38dfb309423437 Kent Libetario 2024-12-20 414 }
38dfb309423437 Kent Libetario 2024-12-20 415 }
38dfb309423437 Kent Libetario 2024-12-20 416
38dfb309423437 Kent Libetario 2024-12-20 417 /**
38dfb309423437 Kent Libetario 2024-12-20 418 * @brief Set the overvoltage threshold of VM6 and VM7.
38dfb309423437 Kent Libetario 2024-12-20 419 * @return 0 in case of success, negative error code otherwise.
38dfb309423437 Kent Libetario 2024-12-20 420 */
38dfb309423437 Kent Libetario 2024-12-20 421 static int max42500_set_ov_thresh2(struct max42500_state *st,
38dfb309423437 Kent Libetario 2024-12-20 422 enum max42500_vm_input vm_in, u8 thresh)
38dfb309423437 Kent Libetario 2024-12-20 423 {
38dfb309423437 Kent Libetario 2024-12-20 424 u8 reg_addr;
38dfb309423437 Kent Libetario 2024-12-20 425
38dfb309423437 Kent Libetario 2024-12-20 426 if (thresh < MAX42500_MIN_THRESH_VM6_V7 ||
38dfb309423437 Kent Libetario 2024-12-20 @427 thresh > MAX42500_MAX_THRESH_VM6_V7)
38dfb309423437 Kent Libetario 2024-12-20 428 return -EINVAL;
38dfb309423437 Kent Libetario 2024-12-20 429
38dfb309423437 Kent Libetario 2024-12-20 430 switch (vm_in) {
38dfb309423437 Kent Libetario 2024-12-20 431 case MAX42500_VM6:
38dfb309423437 Kent Libetario 2024-12-20 432 reg_addr = MAX42500_REG_VINO6;
38dfb309423437 Kent Libetario 2024-12-20 433 break;
38dfb309423437 Kent Libetario 2024-12-20 434 case MAX42500_VM7:
38dfb309423437 Kent Libetario 2024-12-20 435 reg_addr = MAX42500_REG_VINO7;
38dfb309423437 Kent Libetario 2024-12-20 436 break;
38dfb309423437 Kent Libetario 2024-12-20 437 default:
38dfb309423437 Kent Libetario 2024-12-20 438 return -EINVAL;
38dfb309423437 Kent Libetario 2024-12-20 439 }
38dfb309423437 Kent Libetario 2024-12-20 440
38dfb309423437 Kent Libetario 2024-12-20 441 st->ov_thresh2[vm_in] = thresh;
38dfb309423437 Kent Libetario 2024-12-20 442 return max42500_reg_write(st, reg_addr, thresh);
38dfb309423437 Kent Libetario 2024-12-20 443 }
38dfb309423437 Kent Libetario 2024-12-20 444
38dfb309423437 Kent Libetario 2024-12-20 445 /**
38dfb309423437 Kent Libetario 2024-12-20 446 * @brief Set the undervoltage threshold of VM1 to VM5.
38dfb309423437 Kent Libetario 2024-12-20 447 * @return 0 in case of success, negative error code otherwise.
38dfb309423437 Kent Libetario 2024-12-20 448 */
38dfb309423437 Kent Libetario 2024-12-20 449 static int max42500_set_uv_thresh1(struct max42500_state *st,
38dfb309423437 Kent Libetario 2024-12-20 450 enum max42500_vm_input vm_in, u8 thresh)
38dfb309423437 Kent Libetario 2024-12-20 451 {
38dfb309423437 Kent Libetario 2024-12-20 452 if (thresh < MAX42500_MIN_THRESH_VM1_VM5 ||
38dfb309423437 Kent Libetario 2024-12-20 453 thresh > MAX42500_MAX_THRESH_VM1_VM5)
38dfb309423437 Kent Libetario 2024-12-20 454 return -EINVAL;
38dfb309423437 Kent Libetario 2024-12-20 455
38dfb309423437 Kent Libetario 2024-12-20 456 switch (vm_in) {
38dfb309423437 Kent Libetario 2024-12-20 457 case MAX42500_VM1:
38dfb309423437 Kent Libetario 2024-12-20 458 case MAX42500_VM2:
38dfb309423437 Kent Libetario 2024-12-20 459 case MAX42500_VM3:
38dfb309423437 Kent Libetario 2024-12-20 460 case MAX42500_VM4:
38dfb309423437 Kent Libetario 2024-12-20 461 case MAX42500_VM5:
38dfb309423437 Kent Libetario 2024-12-20 462 st->uv_thresh1[vm_in] = thresh;
38dfb309423437 Kent Libetario 2024-12-20 463 return max42500_reg_update(st,
38dfb309423437 Kent Libetario 2024-12-20 464 MAX42500_REG_OVUV1 + vm_in,
38dfb309423437 Kent Libetario 2024-12-20 465 GENMASK(3, 0),
38dfb309423437 Kent Libetario 2024-12-20 466 thresh);
38dfb309423437 Kent Libetario 2024-12-20 467 default:
38dfb309423437 Kent Libetario 2024-12-20 468 return -EINVAL;
38dfb309423437 Kent Libetario 2024-12-20 469 }
38dfb309423437 Kent Libetario 2024-12-20 470 }
38dfb309423437 Kent Libetario 2024-12-20 471
38dfb309423437 Kent Libetario 2024-12-20 472 /**
38dfb309423437 Kent Libetario 2024-12-20 473 * @brief Set the undervoltage threshold of VM6 and VM7.
38dfb309423437 Kent Libetario 2024-12-20 474 * @return 0 in case of success, negative error code otherwise.
38dfb309423437 Kent Libetario 2024-12-20 475 */
38dfb309423437 Kent Libetario 2024-12-20 476 static int max42500_set_uv_thresh2(struct max42500_state *st,
38dfb309423437 Kent Libetario 2024-12-20 477 enum max42500_vm_input vm_in, u8 thresh)
38dfb309423437 Kent Libetario 2024-12-20 478 {
38dfb309423437 Kent Libetario 2024-12-20 479 u8 reg_addr;
38dfb309423437 Kent Libetario 2024-12-20 480
38dfb309423437 Kent Libetario 2024-12-20 481 if (thresh < MAX42500_MIN_THRESH_VM6_V7 ||
38dfb309423437 Kent Libetario 2024-12-20 @482 thresh > MAX42500_MAX_THRESH_VM6_V7)
38dfb309423437 Kent Libetario 2024-12-20 483 return -EINVAL;
38dfb309423437 Kent Libetario 2024-12-20 484
38dfb309423437 Kent Libetario 2024-12-20 485 switch (vm_in) {
38dfb309423437 Kent Libetario 2024-12-20 486 case MAX42500_VM6:
38dfb309423437 Kent Libetario 2024-12-20 487 reg_addr = MAX42500_REG_VINU6;
38dfb309423437 Kent Libetario 2024-12-20 488 break;
38dfb309423437 Kent Libetario 2024-12-20 489 case MAX42500_VM7:
38dfb309423437 Kent Libetario 2024-12-20 490 reg_addr = MAX42500_REG_VINU7;
38dfb309423437 Kent Libetario 2024-12-20 491 break;
38dfb309423437 Kent Libetario 2024-12-20 492 default:
38dfb309423437 Kent Libetario 2024-12-20 493 return -EINVAL;
38dfb309423437 Kent Libetario 2024-12-20 494 }
38dfb309423437 Kent Libetario 2024-12-20 495
38dfb309423437 Kent Libetario 2024-12-20 496 st->uv_thresh2[vm_in] = thresh;
38dfb309423437 Kent Libetario 2024-12-20 497 return max42500_reg_write(st, reg_addr, thresh);
38dfb309423437 Kent Libetario 2024-12-20 498 }
38dfb309423437 Kent Libetario 2024-12-20 499
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 0/2] Add driver for MAX42500 @ 2024-12-20 1:20 Kent Libetario 2024-12-20 1:20 ` [PATCH 2/2] drivers: hwmon: add driver for max42500 Kent Libetario 0 siblings, 1 reply; 6+ messages in thread From: Kent Libetario @ 2024-12-20 1:20 UTC (permalink / raw) To: linux-hwmon, devicetree, linux-kernel Cc: Jean Delvare, Guenter Roeck, Rob Herring, Krzysztof Kozlowski, Conor Dooley This patch series introduces a new hardware monitoring (hwmon) driver for the MAX42500 hardware. The driver is a SoC power-system monitor with up to seven voltage monitor inputs. Each input has programmable OV/UV thresholds over the full temperature range and support DVS through the I2C interface. The MAX42500 contains a programmable flexible power sequence recorder (FPSR) and a programmable challenge/response watchdog with a configurable RESET output. These are also accessible through the I2C interface. The patches are organized as follows: 1. Patch 1/2: Add documentation for devicetree bindings of MAX42500 driver. 2. Patch 2/2: Add the core implementation of the MAX42500 hwmon driver. The driver has been tested on the raspberry pi-4 platform. Kent Libetario (2): dt-bindings: hwmon: add adi,max42500.yaml drivers: hwmon: add driver for max42500 .../bindings/hwmon/adi,max42500.yaml | 42 + MAINTAINERS | 7 + drivers/hwmon/Kconfig | 13 + drivers/hwmon/Makefile | 1 + drivers/hwmon/max42500.c | 1049 +++++++++++++++++ 5 files changed, 1112 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/adi,max42500.yaml create mode 100644 drivers/hwmon/max42500.c base-commit: de076198d1e4934c5fc17aa52d5f1884f469ce1a -- 2.25.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] drivers: hwmon: add driver for max42500 2024-12-20 1:20 [PATCH 0/2] Add driver for MAX42500 Kent Libetario @ 2024-12-20 1:20 ` Kent Libetario 2024-12-21 5:01 ` kernel test robot 2024-12-21 18:08 ` Guenter Roeck 0 siblings, 2 replies; 6+ messages in thread From: Kent Libetario @ 2024-12-20 1:20 UTC (permalink / raw) To: linux-hwmon, devicetree, linux-kernel Cc: Jean Delvare, Guenter Roeck, Rob Herring, Krzysztof Kozlowski, Conor Dooley Add the core implementation of the MAX42500 hwmon driver. Signed-off-by: Kent Libetario <Kent.Libetario@analog.com> --- MAINTAINERS | 1 + drivers/hwmon/Kconfig | 13 + drivers/hwmon/Makefile | 1 + drivers/hwmon/max42500.c | 1049 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 1064 insertions(+) create mode 100644 drivers/hwmon/max42500.c diff --git a/MAINTAINERS b/MAINTAINERS index d1703b4834c8..434191e16dd5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14051,6 +14051,7 @@ M: Kent Libetario <kent.libetario@analog.com> L: linux-hwmon@vger.kernel.org S: Supported F: Documentation/devicetree/bindings/hwmon/adi,max42500.yaml +F: drivers/hwmon/max42500.c MAX6650 HARDWARE MONITOR AND FAN CONTROLLER DRIVER L: linux-hwmon@vger.kernel.org diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index dd376602f3f1..ec0d7aad7789 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1220,6 +1220,19 @@ config MAX31827 This driver can also be built as a module. If so, the module will be called max31827. +config SENSORS_MAX42500 + tristate "MAX42500 Industrial Power System Monitor Family" + depends on I2C + select REGMAP_I2C + help + If you say yes here you get support for MAX42500 SoC power-system monitor + with up to seven voltage monitor. The driver also contains a programmable + challenge/response watchdog, which is accessible through the I2C interface, + along with a configurable RESET output. + + This driver can also be built as a module. If so, the module + will be called max42500. + config SENSORS_MAX6620 tristate "Maxim MAX6620 fan controller" depends on I2C diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index b827b92f2a78..d27d8fc01141 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -152,6 +152,7 @@ obj-$(CONFIG_SENSORS_MAX197) += max197.o obj-$(CONFIG_SENSORS_MAX31722) += max31722.o obj-$(CONFIG_SENSORS_MAX31730) += max31730.o obj-$(CONFIG_SENSORS_MAX31760) += max31760.o +obj-$(CONFIG_SENSORS_MAX42500) += max42500.o obj-$(CONFIG_SENSORS_MAX6620) += max6620.o obj-$(CONFIG_SENSORS_MAX6621) += max6621.o obj-$(CONFIG_SENSORS_MAX6639) += max6639.o diff --git a/drivers/hwmon/max42500.c b/drivers/hwmon/max42500.c new file mode 100644 index 000000000000..23b90c766767 --- /dev/null +++ b/drivers/hwmon/max42500.c @@ -0,0 +1,1049 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * MAX42500 - Industrial Power System Monitor + * + * Copyright 2024 Analog Devices Inc. + */ + +#include <linux/init.h> +#include <linux/jiffies.h> +#include <linux/slab.h> +#include <linux/crc8.h> +#include <linux/bitfield.h> +#include <linux/bitops.h> +#include <linux/err.h> +#include <linux/gpio.h> +#include <linux/gpio/driver.h> +#include <linux/hwmon.h> +#include <linux/i2c.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/property.h> +#include <linux/regmap.h> + +/* Implements Polynomial X^8 + X^2 + X^1 +1 */ +#define CRC8_PEC 0x07 + +DECLARE_CRC8_TABLE(max42500_crc8); + +#define MAX42500_REG_ID 0x00 +#define MAX42500_REG_CONFIG1 0x01 +#define MAX42500_REG_CONFIG2 0x02 +#define MAX42500_REG_VMON 0x03 +#define MAX42500_REG_RSTMAP 0x04 +#define MAX42500_REG_STATOV 0x05 +#define MAX42500_REG_STATUV 0x06 +#define MAX42500_REG_STATOFF 0x07 +#define MAX42500_REG_VIN1 0x08 +#define MAX42500_REG_VIN2 0x09 +#define MAX42500_REG_VIN3 0x0A +#define MAX42500_REG_VIN4 0x0B +#define MAX42500_REG_VIN5 0x0C +#define MAX42500_REG_VINO6 0x0D +#define MAX42500_REG_VINU6 0x0E +#define MAX42500_REG_VINO7 0x0F +#define MAX42500_REG_VINU7 0x10 +#define MAX42500_REG_OVUV1 0x11 +#define MAX42500_REG_OVUV2 0x12 +#define MAX42500_REG_OVUV3 0x13 +#define MAX42500_REG_OVUV4 0x14 +#define MAX42500_REG_OVUV5 0x15 +#define MAX42500_REG_FPSSTAT1 0x16 +#define MAX42500_REG_FPSCFG1 0x17 +#define MAX42500_REG_UTIME1 0x18 +#define MAX42500_REG_UTIME2 0x19 +#define MAX42500_REG_UTIME3 0x1A +#define MAX42500_REG_UTIME4 0x1B +#define MAX42500_REG_UTIME5 0x1C +#define MAX42500_REG_UTIME6 0x1D +#define MAX42500_REG_UTIME7 0x1E +#define MAX42500_REG_DTIME1 0x1F +#define MAX42500_REG_DTIME2 0x20 +#define MAX42500_REG_DTIME3 0x21 +#define MAX42500_REG_DTIME4 0x22 +#define MAX42500_REG_DTIME5 0x23 +#define MAX42500_REG_DTIME6 0x24 +#define MAX42500_REG_DTIME7 0x25 +#define MAX42500_REG_WDSTAT 0x26 +#define MAX42500_REG_WDCDIV 0x27 +#define MAX42500_REG_WDCFG1 0x28 +#define MAX42500_REG_WDCFG2 0x29 +#define MAX42500_REG_WDKEY 0x2A +#define MAX42500_REG_WDLOCK 0x2B +#define MAX42500_REG_RSTCTRL 0x2C +#define MAX42500_REG_CID 0x2D + +/** X is set based on the pull configuration of the ADDR pin */ +#define MAX42500_ADDR(x) (0x28 + (x)) +#define MAX42500_SILICON_ID (0x30) +#define MAX42500_I2C_WR_FRAME_SIZE (4) +#define MAX42500_I2C_RD_FRAME_SIZE (5) + +/** MAX42500 Nominal voltage computation */ +#define MAX42500_VNOM_MAX_VM1_VM4 0xFF // 3.6875v +#define MAX42500_VNOM_MAX_VM5 0xFF // 5.6v +#define MAX42500_MIN_VNOM 0x00 // 0.5v +#define MAX42500_VNOM_STEP_VM1_VM4 0x01 // 0.0125v +#define MAX42500_VNOM_STEP_VM5 0x01 // 0.02v + +/** MAX42500 Undervoltage/Overvoltage maximum and minimum thresholds*/ +#define MAX42500_MAX_THRESH_VM1_VM5 0x0F // OV = 110.0%, UV = 90.0% +#define MAX42500_MIN_THRESH_VM1_VM5 0x00 // OV = 102.5%, UV = 97.5% +#define MAX42500_MAX_THRESH_VM6_V7 0xFF // 1.775v +#define MAX42500_MIN_THRESH_VM6_V7 0x00 // 0.5v + +/* CONFIG1 bit masks */ +#define MAX42500_CONFIG1_PECE_MASK BIT(0) +#define MAX42500_CONFIG1_MBST_MASK BIT(1) +#define MAX42500_CONFIG1_RR_MASK BIT(2) + +/* VMON bit masks */ +#define MAX42500_VMON_IN_MASK(bit) BIT(bit) +#define MAX42500_VMON_VMPD_MASK BIT(7) + +/* RSTMAP bit masks */ +#define MAX42500_RSTMAP_IN_MASK(bit) BIT(bit) +#define MAX42500_RSTMAP_PARM_MASK BIT(7) + +/* WDCDIV bit masks */ +#define MAX42500_WDCDIV_SWW_MASK BIT(6) +#define MAX42500_WDCDIV_WDIC_MASK (0x3F) + +/* WDCFG2 bit masks */ +#define MAX42500_WDCFG2_WDEN_MASK BIT(3) +#define MAX42500_WDCFG2_1UP_MASK (0x7) + +/* WDLOCK bit masks */ +#define MAX42500_WDLOCK_LOCK_MASK BIT(0) + +/* RSTCTRL bit masks */ +#define MAX42500_RSTCTRL_MR1_MASK BIT(2) +#define MAX42500_RSTCTRL_RHLD_MASK (0x3) + +/* MAX42500 device status */ +enum max42500_status { + MAX42500_STATUS_OFF, + MAX42500_STATUS_SLEEP, + MAX42500_STATUS_ON, + MAX42500_STATUS_MAX +}; + +/* MAX42500 voltage monitor input */ +enum max42500_vm_input { + MAX42500_VM1, + MAX42500_VM2, + MAX42500_VM3, + MAX42500_VM4, + MAX42500_VM5, + MAX42500_VM6, + MAX42500_VM7, + MAX42500_VM_MAX +}; + +/* MAX42500 comparator status */ +enum max42500_comp_stat { + MAX42500_COMP_STAT_OFF, + MAX42500_COMP_STAT_UV, + MAX42500_COMP_STAT_OV, + MAX42500_COMP_STAT_MAX +}; + +/* MAX42500 watchdog mode */ +enum max42500_wd_mode { + MAX42500_WD_MODE_CH_RESP, + MAX42500_WD_MODE_SIMPLE, + MAX42500_WD_MODE_MAX +}; + +/* MAX42500 reset hold/active timeout time. */ +enum max42500_wd_rhld { + MAX42500_WD_RHOLD_0_MS, + MAX42500_WD_RHOLD_8_MS, + MAX42500_WD_RHOLD_16_MS, + MAX42500_WD_RHOLD_32_MS, + MAX42500_WD_RHOLD_MAX +}; + +struct max42500_config { + /* Packet error checking enable */ + u8 pece; + /* Enabled voltage monitor inputs */ + u8 vmon_en; + /* Voltage monitor power down enable */ + u8 vmon_vmpd; + /* Enabled voltage monitor reset mapping */ + u8 reset_map; + /* Watchdog mode */ + enum max42500_wd_mode wd_mode; + /* Watchdog clock div */ + u8 wd_cdiv; + /* Watchdog close window */ + u8 wd_close; + /* Watchdog open window */ + u8 wd_open; + /* Watchdog first update window */ + u8 wd_1ud; + /* Watchdog enable */ + u8 wd_en; +}; + +struct max42500_state { + struct i2c_client *client; + struct regmap *regmap; + struct max42500_config *config; + long pwrup_stamp[MAX42500_VM_MAX]; + long pwrdn_stamp[MAX42500_VM_MAX]; + u8 ov_thresh1[MAX42500_VM_MAX - 2]; + u8 ov_thresh2[MAX42500_VM_MAX - 5]; + u8 uv_thresh1[MAX42500_VM_MAX - 2]; + u8 uv_thresh2[MAX42500_VM_MAX - 5]; + u8 nominal_volt[MAX42500_VM_MAX - 2]; + u8 comp_status[MAX42500_VM_MAX * MAX42500_COMP_STAT_MAX]; +}; + +/************************ Functions Definitions **************************/ +/** + * @brief Read a raw value from a register. + * @return 0 in case of success, error code otherwise. + */ +static int max42500_reg_read(struct max42500_state *st, + u8 reg_addr, u8 *reg_data) +{ + int ret; + u8 i2c_data[MAX42500_I2C_RD_FRAME_SIZE] = {0}; + u8 bytes_num; + u8 pece_value; + + /* PEC is computed over entire I2C frame from first START condition */ + i2c_data[0] = (st->client->addr << 1); + i2c_data[1] = reg_addr; + i2c_data[2] = (st->client->addr << 1) | 0x1; + + /* I2C write target address */ + bytes_num = 1; + + ret = regmap_bulk_write(st->regmap, reg_addr, &i2c_data[1], bytes_num); + if (ret) + return ret; + + /* Change byte count if PECE is enabled (1-byte data. 1-byte PEC) */ + bytes_num = (st->config->pece) ? 2 : bytes_num; + + ret = regmap_bulk_read(st->regmap, reg_addr, &i2c_data[3], bytes_num); + if (ret) + return ret; + + if (st->config->pece) { + /* Compute CRC over entire I2C frame */ + pece_value = crc8(max42500_crc8, i2c_data, + (MAX42500_I2C_RD_FRAME_SIZE - 1), 0); + + if (i2c_data[4] != pece_value) + return -EIO; + } + + *reg_data = i2c_data[3]; + + return 0; +} + +/** + * @brief Write a raw value to a register. + * @return 0 in case of success, negative error code otherwise. + */ +static int max42500_reg_write(struct max42500_state *st, + u8 reg_addr, u8 data) +{ + u8 i2c_data[MAX42500_I2C_WR_FRAME_SIZE] = {0}; + u8 bytes_num; + u8 pece_value; + + bytes_num = (st->config->pece) ? (MAX42500_I2C_WR_FRAME_SIZE - 1) : 2; + i2c_data[0] = (st->client->addr << 1); + i2c_data[1] = reg_addr; + i2c_data[2] = (u8)(data & 0xFF); + + pece_value = 0; + if (st->config->pece) + pece_value = crc8(max42500_crc8, i2c_data, bytes_num, 0); + + i2c_data[0] = i2c_data[1]; + i2c_data[1] = i2c_data[2]; + i2c_data[2] = pece_value; + + return regmap_bulk_write(st->regmap, reg_addr, i2c_data, bytes_num); +} + +/** + * @brief Update a register's value based on a mask. + * @return 0 in case of success, negative error code otherwise. + */ +static int max42500_reg_update(struct max42500_state *st, + u8 reg_addr, u8 mask, u8 data) +{ + int ret; + u8 reg_data; + + ret = max42500_reg_read(st, reg_addr, ®_data); + if (ret) + return ret; + + reg_data &= ~mask; + reg_data |= mask & data; + + return max42500_reg_write(st, reg_addr, reg_data); +} + +/** + * @brief Set nominal voltage for VM1 to VM5. + * @return 0 in case of success, negative error code otherwise. + */ +static int max42500_set_nominal_voltage(struct max42500_state *st, + enum max42500_vm_input vm_in, u8 voltage) +{ + u8 reg_addr; + + switch (vm_in) { + case MAX42500_VM1: + case MAX42500_VM2: + case MAX42500_VM3: + case MAX42500_VM4: + if (voltage < MAX42500_MIN_VNOM || + voltage > MAX42500_VNOM_MAX_VM1_VM4) + return -EINVAL; + reg_addr = MAX42500_REG_VIN1 + vm_in; + break; + case MAX42500_VM5: + if (voltage < MAX42500_MIN_VNOM || + voltage > MAX42500_VNOM_MAX_VM5) + return -EINVAL; + reg_addr = MAX42500_REG_VIN5; + break; + default: + return -EINVAL; + } + + st->nominal_volt[vm_in] = voltage; + return max42500_reg_write(st, reg_addr, voltage); +} + +/** + * @brief Get the status of the voltage monitor input. + * @return 0 in case of success, negative error code otherwise. + */ +static int max42500_get_comp_status(struct max42500_state *st, + u8 vm_in, u8 *status) +{ + int ret; + u8 reg_addr; + u8 vm_in_status; + + switch (vm_in % MAX42500_COMP_STAT_MAX) { + case MAX42500_COMP_STAT_OFF: + reg_addr = MAX42500_REG_STATOFF; + break; + case MAX42500_COMP_STAT_UV: + reg_addr = MAX42500_REG_STATUV; + break; + case MAX42500_COMP_STAT_OV: + reg_addr = MAX42500_REG_STATOV; + break; + default: + return -EINVAL; + } + + ret = max42500_reg_read(st, reg_addr, &vm_in_status); + if (ret) + return ret; + + switch (vm_in % MAX42500_VM_MAX) { + case MAX42500_VM1: + *status = (u8)FIELD_GET(BIT(MAX42500_VM1), vm_in_status); + break; + case MAX42500_VM2: + *status = (u8)FIELD_GET(BIT(MAX42500_VM2), vm_in_status); + break; + case MAX42500_VM3: + *status = (u8)FIELD_GET(BIT(MAX42500_VM3), vm_in_status); + break; + case MAX42500_VM4: + *status = (u8)FIELD_GET(BIT(MAX42500_VM4), vm_in_status); + break; + case MAX42500_VM5: + *status = (u8)FIELD_GET(BIT(MAX42500_VM5), vm_in_status); + break; + case MAX42500_VM6: + *status = (u8)FIELD_GET(BIT(MAX42500_VM6), vm_in_status); + break; + case MAX42500_VM7: + *status = (u8)FIELD_GET(BIT(MAX42500_VM7), vm_in_status); + break; + default: + return -EINVAL; + } + st->comp_status[vm_in] = *status; + + return 0; +} + +/** + * @brief Set the overvoltage threshold of VM1 to VM5. + * @return 0 in case of success, negative error code otherwise. + */ +static int max42500_set_ov_thresh1(struct max42500_state *st, + enum max42500_vm_input vm_in, u8 thresh) +{ + if (thresh < MAX42500_MIN_THRESH_VM1_VM5 || + thresh > MAX42500_MAX_THRESH_VM1_VM5) + return -EINVAL; + + switch (vm_in) { + case MAX42500_VM1: + case MAX42500_VM2: + case MAX42500_VM3: + case MAX42500_VM4: + case MAX42500_VM5: + st->ov_thresh1[vm_in] = thresh; + return max42500_reg_update(st, + MAX42500_REG_OVUV1 + vm_in, + GENMASK(7, 4), + FIELD_PREP(GENMASK(7, 4), + thresh)); + default: + return -EINVAL; + } +} + +/** + * @brief Set the overvoltage threshold of VM6 and VM7. + * @return 0 in case of success, negative error code otherwise. + */ +static int max42500_set_ov_thresh2(struct max42500_state *st, + enum max42500_vm_input vm_in, u8 thresh) +{ + u8 reg_addr; + + if (thresh < MAX42500_MIN_THRESH_VM6_V7 || + thresh > MAX42500_MAX_THRESH_VM6_V7) + return -EINVAL; + + switch (vm_in) { + case MAX42500_VM6: + reg_addr = MAX42500_REG_VINO6; + break; + case MAX42500_VM7: + reg_addr = MAX42500_REG_VINO7; + break; + default: + return -EINVAL; + } + + st->ov_thresh2[vm_in] = thresh; + return max42500_reg_write(st, reg_addr, thresh); +} + +/** + * @brief Set the undervoltage threshold of VM1 to VM5. + * @return 0 in case of success, negative error code otherwise. + */ +static int max42500_set_uv_thresh1(struct max42500_state *st, + enum max42500_vm_input vm_in, u8 thresh) +{ + if (thresh < MAX42500_MIN_THRESH_VM1_VM5 || + thresh > MAX42500_MAX_THRESH_VM1_VM5) + return -EINVAL; + + switch (vm_in) { + case MAX42500_VM1: + case MAX42500_VM2: + case MAX42500_VM3: + case MAX42500_VM4: + case MAX42500_VM5: + st->uv_thresh1[vm_in] = thresh; + return max42500_reg_update(st, + MAX42500_REG_OVUV1 + vm_in, + GENMASK(3, 0), + thresh); + default: + return -EINVAL; + } +} + +/** + * @brief Set the undervoltage threshold of VM6 and VM7. + * @return 0 in case of success, negative error code otherwise. + */ +static int max42500_set_uv_thresh2(struct max42500_state *st, + enum max42500_vm_input vm_in, u8 thresh) +{ + u8 reg_addr; + + if (thresh < MAX42500_MIN_THRESH_VM6_V7 || + thresh > MAX42500_MAX_THRESH_VM6_V7) + return -EINVAL; + + switch (vm_in) { + case MAX42500_VM6: + reg_addr = MAX42500_REG_VINU6; + break; + case MAX42500_VM7: + reg_addr = MAX42500_REG_VINU7; + break; + default: + return -EINVAL; + } + + st->uv_thresh2[vm_in] = thresh; + return max42500_reg_write(st, reg_addr, thresh); +} + +/** + * @brief Get the FPS clock divider value. + * @return 0 in case of success, negative error code otherwise. + */ +static int max42500_get_fps_clk_div(struct max42500_state *st, + u8 *fps_clk_div) +{ + int ret; + u8 reg_val; + + ret = max42500_reg_read(st, MAX42500_REG_FPSCFG1, ®_val); + if (ret) + return ret; + + *fps_clk_div = (u8)FIELD_GET(GENMASK(2, 0), reg_val); + + return 0; +} + +/** + * @brief Get power-up timestamp for a specified voltage monitor input. + * @return 0 in case of success, negative error code otherwise. + */ +static int max42500_get_power_up_timestamp(struct max42500_state *st, + enum max42500_vm_input vm_in, long *timestamp) +{ + int ret; + u8 reg_val; + u8 fps_clk_div; + + ret = max42500_reg_read(st, MAX42500_REG_UTIME1 + vm_in, ®_val); + if (ret) + return ret; + + /* Check if the input voltage rose above the UV threshold */ + if (reg_val == 0) { + /* Input voltage never rose above UV threshold*/ + *timestamp = 0; + return 0; + } + + ret = max42500_get_fps_clk_div(st, &fps_clk_div); + if (ret) + return ret; + + *timestamp = (reg_val - 1) * 25 * (1 << fps_clk_div); + st->pwrup_stamp[vm_in] = *timestamp; + return 0; +} + +/** + * @brief Get power-down timestamp for a specified voltage monitor input. + * @return 0 in case of success, negative error code otherwise. + */ +static int max42500_get_power_down_timestamp(struct max42500_state *st, + enum max42500_vm_input vm_in, long *timestamp) +{ + int ret; + u8 reg_val; + u8 fps_clk_div; + + ret = max42500_reg_read(st, MAX42500_REG_DTIME1 + vm_in, ®_val); + if (ret) + return ret; + + /* Check if the input voltage fell below the OFF threshold */ + if (reg_val == 0) { + /* Input voltage never fell below OFF threshold */ + *timestamp = 0; + return 0; + } + + ret = max42500_get_fps_clk_div(st, &fps_clk_div); + if (ret) + return ret; + + *timestamp = (reg_val - 1) * 25 * (1 << fps_clk_div); + st->pwrdn_stamp[vm_in] = *timestamp; + return 0; +} + +/** + * @brief Enable/Disable watchdog + * @return 0 in case of success, error code otherwise + */ +static int max42500_set_watchdog_enable(struct max42500_state *st, + bool wd_enable) +{ + int ret; + u8 reg_val; + + ret = max42500_reg_read(st, MAX42500_REG_WDCFG2, ®_val); + if (ret) + return ret; + + if (wd_enable) + reg_val |= BIT(3); + else + reg_val &= ~BIT(3); + + return max42500_reg_write(st, MAX42500_REG_WDCFG2, reg_val); +} + +/** + * @brief 8-bit watchdog key computation. + * @return 0 in case of success, negative error code otherwise. + */ +static int max42500_get_watchdog_key(struct max42500_state *st, + u8 *new_wd_key) +{ + int ret; + u8 curr_wd_key; + + ret = max42500_reg_read(st, MAX42500_REG_WDKEY, &curr_wd_key); + if (ret) + return ret; + + /* Calculate the new bit using the LFSR polynomial */ + u8 new_bit = ((curr_wd_key >> 7) ^ + (curr_wd_key >> 5) ^ + (curr_wd_key >> 4) ^ + (curr_wd_key >> 3)) & 0x01; + + /* Shift existing bits upwards to MSb and insert the new bit as LSb */ + *new_wd_key = (curr_wd_key << 1) | new_bit; + + return 0; +} + +/** + * @brief Update the watchdog key based on the mode and current value. + * @return 0 in case of success, error code otherwise. + */ +static int max42500_set_watchdog_key(struct max42500_state *st) +{ + int ret; + u8 reg_val; + u8 wd_key; + u8 wd_mode; + + ret = max42500_reg_read(st, MAX42500_REG_WDKEY, &wd_key); + if (ret) + return ret; + + ret = max42500_reg_read(st, MAX42500_REG_WDCDIV, ®_val); + if (ret) + return ret; + + wd_mode = (u8)FIELD_GET(BIT(6), reg_val); + + /* Compute new watchdog key for challenge/response mode */ + if (wd_mode == MAX42500_WD_MODE_CH_RESP) + max42500_get_watchdog_key(st, &wd_key); + + return max42500_reg_write(st, MAX42500_REG_WDKEY, wd_key); +} + +/** @brief Set watchdog reset hold time + * @return 0 in case of success, error code otherwise + */ +static int max42500_set_watchdog_rhld(struct max42500_state *st, u8 rhld) +{ + return max42500_reg_update(st, + MAX42500_REG_RSTCTRL, + GENMASK(1, 0), + rhld); +} + +static umode_t max42500_is_visible(const void *data, + enum hwmon_sensor_types type, u32 attr, int channel) +{ + const struct max42500_state *st = data; + + switch (type) { + case hwmon_chip: + switch (attr) { + case hwmon_chip_in_reset_history: + if (st->pwrup_stamp[channel]) + return 0444; + break; + case hwmon_chip_curr_reset_history: + if (st->pwrdn_stamp[channel]) + return 0444; + break; + case hwmon_chip_register_tz: + return 0200; + case hwmon_chip_alarms: + case hwmon_chip_update_interval: + case hwmon_chip_temp_reset_history: + case hwmon_chip_power_reset_history: + return 0644; + } + break; + case hwmon_in: + switch (attr) { + case hwmon_in_input: + if (st->nominal_volt[channel]) + return 0644; + break; + case hwmon_in_lowest: + if (st->ov_thresh1[channel]) + return 0644; + break; + case hwmon_in_highest: + if (st->ov_thresh2[channel]) + return 0644; + break; + case hwmon_in_min: + if (st->uv_thresh1[channel]) + return 0644; + break; + case hwmon_in_max: + if (st->uv_thresh2[channel]) + return 0644; + break; + case hwmon_in_label: + if (st->comp_status[channel]) + return 0444; + break; + } + break; + default: + break; + } + + return 0; +} + +static int max42500_read_in(struct device *dev, u32 attr, int channel, + long *value) +{ + struct max42500_state *st = dev_get_drvdata(dev); + + switch (attr) { + case hwmon_in_label: + return max42500_get_comp_status(st, channel, (u8 *)value); + default: + return -EOPNOTSUPP; + } +} + +static int max42500_read_chip(struct device *dev, u32 attr, int channel, + long *value) +{ + struct max42500_state *st = dev_get_drvdata(dev); + + switch (attr) { + case hwmon_chip_in_reset_history: + return max42500_get_power_up_timestamp(st, channel, value); + case hwmon_chip_curr_reset_history: + return max42500_get_power_down_timestamp(st, channel, value); + case hwmon_chip_power_reset_history: + return max42500_get_watchdog_key(st, (u8 *)value); + case hwmon_chip_update_interval: + return max42500_get_fps_clk_div(st, (u8 *)value); + default: + return -EOPNOTSUPP; + } +} + +static int max42500_read(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long *value) +{ + switch (type) { + case hwmon_chip: + return max42500_read_chip(dev, attr, channel, value); + case hwmon_in: + return max42500_read_in(dev, attr, channel, value); + default: + return -EOPNOTSUPP; + } +} + +static int max42500_write_in(struct device *dev, u32 attr, int channel, + long value) +{ + struct max42500_state *st = dev_get_drvdata(dev); + + switch (attr) { + case hwmon_in_input: + return max42500_set_nominal_voltage(st, channel, (u8)value); + case hwmon_in_min: + return max42500_set_uv_thresh1(st, channel, (u8)value); + case hwmon_in_max: + return max42500_set_uv_thresh2(st, channel, (u8)value); + case hwmon_in_lowest: + return max42500_set_ov_thresh1(st, channel, (u8)value); + case hwmon_in_highest: + return max42500_set_ov_thresh2(st, channel, (u8)value); + default: + return -EOPNOTSUPP; + } +} + +static int max42500_write_chip(struct device *dev, u32 attr, int channel, + long value) +{ + struct max42500_state *st = dev_get_drvdata(dev); + + switch (attr) { + case hwmon_chip_temp_reset_history: + return max42500_set_watchdog_rhld(st, (u8)value); + case hwmon_chip_register_tz: + return max42500_set_watchdog_key(st); + case hwmon_chip_alarms: + return max42500_set_watchdog_enable(st, (bool)value); + default: + return -EOPNOTSUPP; + } +} + +static int max42500_write(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long value) +{ + switch (type) { + case hwmon_chip: + return max42500_write_chip(dev, attr, channel, value); + case hwmon_in: + return max42500_write_in(dev, attr, channel, value); + default: + return -EOPNOTSUPP; + } +} + +static const struct max42500_config max42500_init = { + .pece = true, + .vmon_en = true, + .vmon_vmpd = true, + .reset_map = (MAX42500_RSTMAP_IN_MASK(MAX42500_VM2) | + MAX42500_RSTMAP_IN_MASK(MAX42500_VM3) | + MAX42500_RSTMAP_IN_MASK(MAX42500_VM4) | + MAX42500_RSTMAP_PARM_MASK), + .wd_mode = MAX42500_WD_MODE_SIMPLE, + .wd_cdiv = 0x0, + .wd_close = 0x0, + .wd_open = 0x0, + .wd_1ud = 0x0, + .wd_en = true, +}; + +static int max42500_init_client(struct max42500_state *st, + struct device *dev) +{ + int err; + u8 device_id; + + if (!st || !dev) + return -EINVAL; + + crc8_populate_msb(max42500_crc8, CRC8_PEC); + + /* Check device silicon ID */ + err = max42500_reg_read(st, MAX42500_REG_ID, &device_id); + if (err) + return err; + + if (device_id != MAX42500_SILICON_ID) + return -ENODEV; + + /* Configure PEC */ + err = max42500_reg_update(st, + MAX42500_REG_CONFIG1, + BIT(0), + max42500_init.pece); + if (err) + return err; + + /* Set PEC enable/disable for subsequent register access */ + st->config->pece = max42500_init.pece; + + /* Enable voltage monitor inputs */ + err = max42500_reg_update(st, + MAX42500_REG_VMON, + GENMASK(6, 0), + max42500_init.vmon_en); + if (err) + return err; + + /* Enable voltage monitor power-down */ + err = max42500_reg_update(st, + MAX42500_REG_VMON, + BIT(7), + max42500_init.vmon_vmpd); + if (err) + return err; + + /* Enable input OV/UV mapping to reset pin */ + err = max42500_reg_update(st, + MAX42500_REG_RSTMAP, + GENMASK(6, 0), + max42500_init.reset_map); + if (err) + return err; + + /* Set watchdog mode */ + err = max42500_reg_update(st, + MAX42500_REG_WDCDIV, + BIT(6), + max42500_init.wd_mode << 6); + if (err) + return err; + + /* Set watchdog clock div */ + err = max42500_reg_update(st, + MAX42500_REG_WDCDIV, + GENMASK(5, 0), + max42500_init.wd_cdiv); + if (err) + return err; + + /* Set watchdog open window */ + err = max42500_reg_update(st, + MAX42500_REG_WDCFG1, + GENMASK(3, 0), + max42500_init.wd_open); + if (err) + return err; + + /* Set watchdog close window */ + err = max42500_reg_update(st, + MAX42500_REG_WDCFG1, + GENMASK(7, 4), + max42500_init.wd_close); + if (err) + return err; + + /* Set watchdog first update window */ + err = max42500_reg_update(st, + MAX42500_REG_WDCFG2, + GENMASK(2, 0), + max42500_init.wd_1ud); + if (err) + return err; + + /* Set watchdog enable */ + err = max42500_reg_update(st, + MAX42500_REG_WDCFG2, + BIT(3), + max42500_init.wd_en); + if (err) + return err; + + /* Update parameters */ + st->config->vmon_en = max42500_init.vmon_en; + st->config->vmon_vmpd = max42500_init.vmon_vmpd; + st->config->reset_map = max42500_init.reset_map; + st->config->wd_mode = max42500_init.wd_mode; + st->config->wd_cdiv = max42500_init.wd_cdiv; + st->config->wd_open = max42500_init.wd_open; + st->config->wd_close = max42500_init.wd_close; + st->config->wd_1ud = max42500_init.wd_1ud; + st->config->wd_en = max42500_init.wd_en; + + return 0; +} + +static const struct hwmon_ops max42500_hwmon_ops = { + .is_visible = max42500_is_visible, + .read = max42500_read, + .write = max42500_write, +}; + +static const struct hwmon_channel_info *max42500_info[] = { + HWMON_CHANNEL_INFO(chip, + HWMON_C_IN_RESET_HISTORY | HWMON_C_CURR_RESET_HISTORY | + HWMON_C_TEMP_RESET_HISTORY | HWMON_C_POWER_RESET_HISTORY | + HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL | HWMON_C_ALARMS, + HWMON_C_IN_RESET_HISTORY | HWMON_C_CURR_RESET_HISTORY, + HWMON_C_IN_RESET_HISTORY | HWMON_C_CURR_RESET_HISTORY, + HWMON_C_IN_RESET_HISTORY | HWMON_C_CURR_RESET_HISTORY, + HWMON_C_IN_RESET_HISTORY | HWMON_C_CURR_RESET_HISTORY, + HWMON_C_IN_RESET_HISTORY | HWMON_C_CURR_RESET_HISTORY, + HWMON_C_IN_RESET_HISTORY | HWMON_C_CURR_RESET_HISTORY), + HWMON_CHANNEL_INFO(in, + HWMON_I_LABEL | HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_MIN | + HWMON_I_HIGHEST | HWMON_I_MAX, + HWMON_I_LABEL | HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_MIN | + HWMON_I_HIGHEST | HWMON_I_MAX, + HWMON_I_LABEL | HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_MIN, + HWMON_I_LABEL | HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_MIN, + HWMON_I_LABEL | HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_MIN, + HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, + HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, + HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, + HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL), + NULL +}; + +static const struct hwmon_chip_info max42500_chip_info = { + .ops = &max42500_hwmon_ops, + .info = max42500_info, +}; + +static const struct regmap_config max42500_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .max_register = 0x2D +}; + +static int max42500_probe(struct i2c_client *client) +{ + struct device *hwmon_dev; + struct max42500_state *st; + int err; + + st = devm_kzalloc(&client->dev, sizeof(*st), GFP_KERNEL); + if (!st) + return -ENOMEM; + + st->client = client; + st->regmap = devm_regmap_init_i2c(client, &max42500_regmap_config); + if (IS_ERR(st->regmap)) + return PTR_ERR(st->regmap); + + err = max42500_init_client(st, &client->dev); + if (err) + return err; + + hwmon_dev = devm_hwmon_device_register_with_info(&client->dev, + client->name, st, &max42500_chip_info, NULL); + + return PTR_ERR_OR_ZERO(hwmon_dev); +} + +static const struct of_device_id max42500_of_match[] = { + { .compatible = "adi,max42500" }, + { } +}; +MODULE_DEVICE_TABLE(of, max42500_of_match); + +static const struct i2c_device_id max42500_id[] = { + { "max42500", 0 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, max42500_id); + +static struct i2c_driver max42500_driver = { + .driver = { + .name = "max42500", + .of_match_table = max42500_of_match, + }, + .probe = max42500_probe, + .id_table = max42500_id, +}; + +module_i2c_driver(max42500_driver); + +MODULE_AUTHOR("Kent Libetario <kent.libetario@analog.com>"); +MODULE_DESCRIPTION("Hwmon driver for MAX42500"); +MODULE_LICENSE("GPL"); -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drivers: hwmon: add driver for max42500 2024-12-20 1:20 ` [PATCH 2/2] drivers: hwmon: add driver for max42500 Kent Libetario @ 2024-12-21 5:01 ` kernel test robot 2024-12-21 18:08 ` Guenter Roeck 1 sibling, 0 replies; 6+ messages in thread From: kernel test robot @ 2024-12-21 5:01 UTC (permalink / raw) To: Kent Libetario, linux-hwmon, devicetree, linux-kernel Cc: oe-kbuild-all, Jean Delvare, Guenter Roeck, Rob Herring, Krzysztof Kozlowski, Conor Dooley Hi Kent, kernel test robot noticed the following build warnings: [auto build test WARNING on de076198d1e4934c5fc17aa52d5f1884f469ce1a] url: https://github.com/intel-lab-lkp/linux/commits/Kent-Libetario/dt-bindings-hwmon-add-adi-max42500-yaml/20241220-092728 base: de076198d1e4934c5fc17aa52d5f1884f469ce1a patch link: https://lore.kernel.org/r/20241220012003.9568-3-Kent.Libetario%40analog.com patch subject: [PATCH 2/2] drivers: hwmon: add driver for max42500 config: x86_64-randconfig-r072-20241221 (https://download.01.org/0day-ci/archive/20241221/202412211209.zqypWgoc-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241221/202412211209.zqypWgoc-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/202412211209.zqypWgoc-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/hwmon/max42500.c:206: warning: Cannot understand * @brief Read a raw value from a register. on line 206 - I thought it was a doc line >> drivers/hwmon/max42500.c:251: warning: Cannot understand * @brief Write a raw value to a register. on line 251 - I thought it was a doc line >> drivers/hwmon/max42500.c:278: warning: Cannot understand * @brief Update a register's value based on a mask. on line 278 - I thought it was a doc line >> drivers/hwmon/max42500.c:298: warning: Cannot understand * @brief Set nominal voltage for VM1 to VM5. on line 298 - I thought it was a doc line >> drivers/hwmon/max42500.c:331: warning: Cannot understand * @brief Get the status of the voltage monitor input. on line 331 - I thought it was a doc line >> drivers/hwmon/max42500.c:390: warning: Cannot understand * @brief Set the overvoltage threshold of VM1 to VM5. on line 390 - I thought it was a doc line >> drivers/hwmon/max42500.c:418: warning: Cannot understand * @brief Set the overvoltage threshold of VM6 and VM7. on line 418 - I thought it was a doc line >> drivers/hwmon/max42500.c:446: warning: Cannot understand * @brief Set the undervoltage threshold of VM1 to VM5. on line 446 - I thought it was a doc line >> drivers/hwmon/max42500.c:473: warning: Cannot understand * @brief Set the undervoltage threshold of VM6 and VM7. on line 473 - I thought it was a doc line >> drivers/hwmon/max42500.c:501: warning: Cannot understand * @brief Get the FPS clock divider value. on line 501 - I thought it was a doc line >> drivers/hwmon/max42500.c:520: warning: Cannot understand * @brief Get power-up timestamp for a specified voltage monitor input. on line 520 - I thought it was a doc line >> drivers/hwmon/max42500.c:551: warning: Cannot understand * @brief Get power-down timestamp for a specified voltage monitor input. on line 551 - I thought it was a doc line >> drivers/hwmon/max42500.c:582: warning: Cannot understand * @brief Enable/Disable watchdog on line 582 - I thought it was a doc line >> drivers/hwmon/max42500.c:604: warning: Cannot understand * @brief 8-bit watchdog key computation. on line 604 - I thought it was a doc line >> drivers/hwmon/max42500.c:630: warning: Cannot understand * @brief Update the watchdog key based on the mode and current value. on line 630 - I thought it was a doc line vim +206 drivers/hwmon/max42500.c 203 204 /************************ Functions Definitions **************************/ 205 /** > 206 * @brief Read a raw value from a register. 207 * @return 0 in case of success, error code otherwise. 208 */ 209 static int max42500_reg_read(struct max42500_state *st, 210 u8 reg_addr, u8 *reg_data) 211 { 212 int ret; 213 u8 i2c_data[MAX42500_I2C_RD_FRAME_SIZE] = {0}; 214 u8 bytes_num; 215 u8 pece_value; 216 217 /* PEC is computed over entire I2C frame from first START condition */ 218 i2c_data[0] = (st->client->addr << 1); 219 i2c_data[1] = reg_addr; 220 i2c_data[2] = (st->client->addr << 1) | 0x1; 221 222 /* I2C write target address */ 223 bytes_num = 1; 224 225 ret = regmap_bulk_write(st->regmap, reg_addr, &i2c_data[1], bytes_num); 226 if (ret) 227 return ret; 228 229 /* Change byte count if PECE is enabled (1-byte data. 1-byte PEC) */ 230 bytes_num = (st->config->pece) ? 2 : bytes_num; 231 232 ret = regmap_bulk_read(st->regmap, reg_addr, &i2c_data[3], bytes_num); 233 if (ret) 234 return ret; 235 236 if (st->config->pece) { 237 /* Compute CRC over entire I2C frame */ 238 pece_value = crc8(max42500_crc8, i2c_data, 239 (MAX42500_I2C_RD_FRAME_SIZE - 1), 0); 240 241 if (i2c_data[4] != pece_value) 242 return -EIO; 243 } 244 245 *reg_data = i2c_data[3]; 246 247 return 0; 248 } 249 250 /** > 251 * @brief Write a raw value to a register. 252 * @return 0 in case of success, negative error code otherwise. 253 */ 254 static int max42500_reg_write(struct max42500_state *st, 255 u8 reg_addr, u8 data) 256 { 257 u8 i2c_data[MAX42500_I2C_WR_FRAME_SIZE] = {0}; 258 u8 bytes_num; 259 u8 pece_value; 260 261 bytes_num = (st->config->pece) ? (MAX42500_I2C_WR_FRAME_SIZE - 1) : 2; 262 i2c_data[0] = (st->client->addr << 1); 263 i2c_data[1] = reg_addr; 264 i2c_data[2] = (u8)(data & 0xFF); 265 266 pece_value = 0; 267 if (st->config->pece) 268 pece_value = crc8(max42500_crc8, i2c_data, bytes_num, 0); 269 270 i2c_data[0] = i2c_data[1]; 271 i2c_data[1] = i2c_data[2]; 272 i2c_data[2] = pece_value; 273 274 return regmap_bulk_write(st->regmap, reg_addr, i2c_data, bytes_num); 275 } 276 277 /** > 278 * @brief Update a register's value based on a mask. 279 * @return 0 in case of success, negative error code otherwise. 280 */ 281 static int max42500_reg_update(struct max42500_state *st, 282 u8 reg_addr, u8 mask, u8 data) 283 { 284 int ret; 285 u8 reg_data; 286 287 ret = max42500_reg_read(st, reg_addr, ®_data); 288 if (ret) 289 return ret; 290 291 reg_data &= ~mask; 292 reg_data |= mask & data; 293 294 return max42500_reg_write(st, reg_addr, reg_data); 295 } 296 297 /** > 298 * @brief Set nominal voltage for VM1 to VM5. 299 * @return 0 in case of success, negative error code otherwise. 300 */ 301 static int max42500_set_nominal_voltage(struct max42500_state *st, 302 enum max42500_vm_input vm_in, u8 voltage) 303 { 304 u8 reg_addr; 305 306 switch (vm_in) { 307 case MAX42500_VM1: 308 case MAX42500_VM2: 309 case MAX42500_VM3: 310 case MAX42500_VM4: 311 if (voltage < MAX42500_MIN_VNOM || 312 voltage > MAX42500_VNOM_MAX_VM1_VM4) 313 return -EINVAL; 314 reg_addr = MAX42500_REG_VIN1 + vm_in; 315 break; 316 case MAX42500_VM5: 317 if (voltage < MAX42500_MIN_VNOM || 318 voltage > MAX42500_VNOM_MAX_VM5) 319 return -EINVAL; 320 reg_addr = MAX42500_REG_VIN5; 321 break; 322 default: 323 return -EINVAL; 324 } 325 326 st->nominal_volt[vm_in] = voltage; 327 return max42500_reg_write(st, reg_addr, voltage); 328 } 329 330 /** > 331 * @brief Get the status of the voltage monitor input. 332 * @return 0 in case of success, negative error code otherwise. 333 */ 334 static int max42500_get_comp_status(struct max42500_state *st, 335 u8 vm_in, u8 *status) 336 { 337 int ret; 338 u8 reg_addr; 339 u8 vm_in_status; 340 341 switch (vm_in % MAX42500_COMP_STAT_MAX) { 342 case MAX42500_COMP_STAT_OFF: 343 reg_addr = MAX42500_REG_STATOFF; 344 break; 345 case MAX42500_COMP_STAT_UV: 346 reg_addr = MAX42500_REG_STATUV; 347 break; 348 case MAX42500_COMP_STAT_OV: 349 reg_addr = MAX42500_REG_STATOV; 350 break; 351 default: 352 return -EINVAL; 353 } 354 355 ret = max42500_reg_read(st, reg_addr, &vm_in_status); 356 if (ret) 357 return ret; 358 359 switch (vm_in % MAX42500_VM_MAX) { 360 case MAX42500_VM1: 361 *status = (u8)FIELD_GET(BIT(MAX42500_VM1), vm_in_status); 362 break; 363 case MAX42500_VM2: 364 *status = (u8)FIELD_GET(BIT(MAX42500_VM2), vm_in_status); 365 break; 366 case MAX42500_VM3: 367 *status = (u8)FIELD_GET(BIT(MAX42500_VM3), vm_in_status); 368 break; 369 case MAX42500_VM4: 370 *status = (u8)FIELD_GET(BIT(MAX42500_VM4), vm_in_status); 371 break; 372 case MAX42500_VM5: 373 *status = (u8)FIELD_GET(BIT(MAX42500_VM5), vm_in_status); 374 break; 375 case MAX42500_VM6: 376 *status = (u8)FIELD_GET(BIT(MAX42500_VM6), vm_in_status); 377 break; 378 case MAX42500_VM7: 379 *status = (u8)FIELD_GET(BIT(MAX42500_VM7), vm_in_status); 380 break; 381 default: 382 return -EINVAL; 383 } 384 st->comp_status[vm_in] = *status; 385 386 return 0; 387 } 388 389 /** > 390 * @brief Set the overvoltage threshold of VM1 to VM5. 391 * @return 0 in case of success, negative error code otherwise. 392 */ 393 static int max42500_set_ov_thresh1(struct max42500_state *st, 394 enum max42500_vm_input vm_in, u8 thresh) 395 { 396 if (thresh < MAX42500_MIN_THRESH_VM1_VM5 || 397 thresh > MAX42500_MAX_THRESH_VM1_VM5) 398 return -EINVAL; 399 400 switch (vm_in) { 401 case MAX42500_VM1: 402 case MAX42500_VM2: 403 case MAX42500_VM3: 404 case MAX42500_VM4: 405 case MAX42500_VM5: 406 st->ov_thresh1[vm_in] = thresh; 407 return max42500_reg_update(st, 408 MAX42500_REG_OVUV1 + vm_in, 409 GENMASK(7, 4), 410 FIELD_PREP(GENMASK(7, 4), 411 thresh)); 412 default: 413 return -EINVAL; 414 } 415 } 416 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drivers: hwmon: add driver for max42500 2024-12-20 1:20 ` [PATCH 2/2] drivers: hwmon: add driver for max42500 Kent Libetario 2024-12-21 5:01 ` kernel test robot @ 2024-12-21 18:08 ` Guenter Roeck 2025-03-10 9:05 ` Libetario, Kent 1 sibling, 1 reply; 6+ messages in thread From: Guenter Roeck @ 2024-12-21 18:08 UTC (permalink / raw) To: Kent Libetario Cc: linux-hwmon, devicetree, linux-kernel, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Conor Dooley On Fri, Dec 20, 2024 at 09:20:03AM +0800, Kent Libetario wrote: > Add the core implementation of the MAX42500 hwmon driver. > Please have a look into Documentation/process/submitting-patches.rst and follow the guidance about describing patches. > Signed-off-by: Kent Libetario <Kent.Libetario@analog.com> > --- > MAINTAINERS | 1 + > drivers/hwmon/Kconfig | 13 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/max42500.c | 1049 ++++++++++++++++++++++++++++++++++++++ Documentation missing. There is a large number of checkpatch CHECK messages, almost all about multi-line alignment. Indeed, when looking through the code, multi-line aligment seems arbitrary. Please fix. PEC support should be implemented through the i2c controller driver. Please see other drivers such as lm90.c or max31827.c for examples how to do that. Some more comments inline. The review is partial. The major problem I see is that there are not just a few but _lots_ of ABI violations. This is unacceptable. Please rework the driver and follow the hwmon ABI for all sysfs attributes. Guenter > 4 files changed, 1064 insertions(+) > create mode 100644 drivers/hwmon/max42500.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index d1703b4834c8..434191e16dd5 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -14051,6 +14051,7 @@ M: Kent Libetario <kent.libetario@analog.com> > L: linux-hwmon@vger.kernel.org > S: Supported > F: Documentation/devicetree/bindings/hwmon/adi,max42500.yaml > +F: drivers/hwmon/max42500.c > > MAX6650 HARDWARE MONITOR AND FAN CONTROLLER DRIVER > L: linux-hwmon@vger.kernel.org > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index dd376602f3f1..ec0d7aad7789 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1220,6 +1220,19 @@ config MAX31827 > This driver can also be built as a module. If so, the module > will be called max31827. > > +config SENSORS_MAX42500 > + tristate "MAX42500 Industrial Power System Monitor Family" > + depends on I2C > + select REGMAP_I2C > + help > + If you say yes here you get support for MAX42500 SoC power-system monitor > + with up to seven voltage monitor. The driver also contains a programmable > + challenge/response watchdog, which is accessible through the I2C interface, > + along with a configurable RESET output. > + > + This driver can also be built as a module. If so, the module > + will be called max42500. > + > config SENSORS_MAX6620 > tristate "Maxim MAX6620 fan controller" > depends on I2C > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index b827b92f2a78..d27d8fc01141 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -152,6 +152,7 @@ obj-$(CONFIG_SENSORS_MAX197) += max197.o > obj-$(CONFIG_SENSORS_MAX31722) += max31722.o > obj-$(CONFIG_SENSORS_MAX31730) += max31730.o > obj-$(CONFIG_SENSORS_MAX31760) += max31760.o > +obj-$(CONFIG_SENSORS_MAX42500) += max42500.o > obj-$(CONFIG_SENSORS_MAX6620) += max6620.o > obj-$(CONFIG_SENSORS_MAX6621) += max6621.o > obj-$(CONFIG_SENSORS_MAX6639) += max6639.o > diff --git a/drivers/hwmon/max42500.c b/drivers/hwmon/max42500.c > new file mode 100644 > index 000000000000..23b90c766767 > --- /dev/null > +++ b/drivers/hwmon/max42500.c > @@ -0,0 +1,1049 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * MAX42500 - Industrial Power System Monitor > + * > + * Copyright 2024 Analog Devices Inc. > + */ > + > +#include <linux/init.h> > +#include <linux/jiffies.h> > +#include <linux/slab.h> > +#include <linux/crc8.h> > +#include <linux/bitfield.h> > +#include <linux/bitops.h> > +#include <linux/err.h> > +#include <linux/gpio.h> > +#include <linux/gpio/driver.h> > +#include <linux/hwmon.h> > +#include <linux/i2c.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/property.h> > +#include <linux/regmap.h> > + > +/* Implements Polynomial X^8 + X^2 + X^1 +1 */ > +#define CRC8_PEC 0x07 > + > +DECLARE_CRC8_TABLE(max42500_crc8); > + > +#define MAX42500_REG_ID 0x00 > +#define MAX42500_REG_CONFIG1 0x01 > +#define MAX42500_REG_CONFIG2 0x02 > +#define MAX42500_REG_VMON 0x03 > +#define MAX42500_REG_RSTMAP 0x04 > +#define MAX42500_REG_STATOV 0x05 > +#define MAX42500_REG_STATUV 0x06 > +#define MAX42500_REG_STATOFF 0x07 > +#define MAX42500_REG_VIN1 0x08 > +#define MAX42500_REG_VIN2 0x09 > +#define MAX42500_REG_VIN3 0x0A > +#define MAX42500_REG_VIN4 0x0B > +#define MAX42500_REG_VIN5 0x0C > +#define MAX42500_REG_VINO6 0x0D > +#define MAX42500_REG_VINU6 0x0E > +#define MAX42500_REG_VINO7 0x0F > +#define MAX42500_REG_VINU7 0x10 > +#define MAX42500_REG_OVUV1 0x11 > +#define MAX42500_REG_OVUV2 0x12 > +#define MAX42500_REG_OVUV3 0x13 > +#define MAX42500_REG_OVUV4 0x14 > +#define MAX42500_REG_OVUV5 0x15 > +#define MAX42500_REG_FPSSTAT1 0x16 > +#define MAX42500_REG_FPSCFG1 0x17 > +#define MAX42500_REG_UTIME1 0x18 > +#define MAX42500_REG_UTIME2 0x19 > +#define MAX42500_REG_UTIME3 0x1A > +#define MAX42500_REG_UTIME4 0x1B > +#define MAX42500_REG_UTIME5 0x1C > +#define MAX42500_REG_UTIME6 0x1D > +#define MAX42500_REG_UTIME7 0x1E > +#define MAX42500_REG_DTIME1 0x1F > +#define MAX42500_REG_DTIME2 0x20 > +#define MAX42500_REG_DTIME3 0x21 > +#define MAX42500_REG_DTIME4 0x22 > +#define MAX42500_REG_DTIME5 0x23 > +#define MAX42500_REG_DTIME6 0x24 > +#define MAX42500_REG_DTIME7 0x25 > +#define MAX42500_REG_WDSTAT 0x26 > +#define MAX42500_REG_WDCDIV 0x27 > +#define MAX42500_REG_WDCFG1 0x28 > +#define MAX42500_REG_WDCFG2 0x29 > +#define MAX42500_REG_WDKEY 0x2A > +#define MAX42500_REG_WDLOCK 0x2B > +#define MAX42500_REG_RSTCTRL 0x2C > +#define MAX42500_REG_CID 0x2D > + > +/** X is set based on the pull configuration of the ADDR pin */ > +#define MAX42500_ADDR(x) (0x28 + (x)) > +#define MAX42500_SILICON_ID (0x30) > +#define MAX42500_I2C_WR_FRAME_SIZE (4) > +#define MAX42500_I2C_RD_FRAME_SIZE (5) > + > +/** MAX42500 Nominal voltage computation */ > +#define MAX42500_VNOM_MAX_VM1_VM4 0xFF // 3.6875v > +#define MAX42500_VNOM_MAX_VM5 0xFF // 5.6v > +#define MAX42500_MIN_VNOM 0x00 // 0.5v > +#define MAX42500_VNOM_STEP_VM1_VM4 0x01 // 0.0125v > +#define MAX42500_VNOM_STEP_VM5 0x01 // 0.02v > + > +/** MAX42500 Undervoltage/Overvoltage maximum and minimum thresholds*/ > +#define MAX42500_MAX_THRESH_VM1_VM5 0x0F // OV = 110.0%, UV = 90.0% > +#define MAX42500_MIN_THRESH_VM1_VM5 0x00 // OV = 102.5%, UV = 97.5% > +#define MAX42500_MAX_THRESH_VM6_V7 0xFF // 1.775v > +#define MAX42500_MIN_THRESH_VM6_V7 0x00 // 0.5v > + > +/* CONFIG1 bit masks */ > +#define MAX42500_CONFIG1_PECE_MASK BIT(0) > +#define MAX42500_CONFIG1_MBST_MASK BIT(1) > +#define MAX42500_CONFIG1_RR_MASK BIT(2) > + > +/* VMON bit masks */ > +#define MAX42500_VMON_IN_MASK(bit) BIT(bit) > +#define MAX42500_VMON_VMPD_MASK BIT(7) > + > +/* RSTMAP bit masks */ > +#define MAX42500_RSTMAP_IN_MASK(bit) BIT(bit) > +#define MAX42500_RSTMAP_PARM_MASK BIT(7) > + > +/* WDCDIV bit masks */ > +#define MAX42500_WDCDIV_SWW_MASK BIT(6) > +#define MAX42500_WDCDIV_WDIC_MASK (0x3F) > + > +/* WDCFG2 bit masks */ > +#define MAX42500_WDCFG2_WDEN_MASK BIT(3) > +#define MAX42500_WDCFG2_1UP_MASK (0x7) > + > +/* WDLOCK bit masks */ > +#define MAX42500_WDLOCK_LOCK_MASK BIT(0) > + > +/* RSTCTRL bit masks */ > +#define MAX42500_RSTCTRL_MR1_MASK BIT(2) > +#define MAX42500_RSTCTRL_RHLD_MASK (0x3) > + > +/* MAX42500 device status */ > +enum max42500_status { > + MAX42500_STATUS_OFF, > + MAX42500_STATUS_SLEEP, > + MAX42500_STATUS_ON, > + MAX42500_STATUS_MAX > +}; > + > +/* MAX42500 voltage monitor input */ > +enum max42500_vm_input { > + MAX42500_VM1, > + MAX42500_VM2, > + MAX42500_VM3, > + MAX42500_VM4, > + MAX42500_VM5, > + MAX42500_VM6, > + MAX42500_VM7, > + MAX42500_VM_MAX > +}; > + > +/* MAX42500 comparator status */ > +enum max42500_comp_stat { > + MAX42500_COMP_STAT_OFF, > + MAX42500_COMP_STAT_UV, > + MAX42500_COMP_STAT_OV, > + MAX42500_COMP_STAT_MAX > +}; > + > +/* MAX42500 watchdog mode */ > +enum max42500_wd_mode { > + MAX42500_WD_MODE_CH_RESP, > + MAX42500_WD_MODE_SIMPLE, > + MAX42500_WD_MODE_MAX > +}; > + > +/* MAX42500 reset hold/active timeout time. */ > +enum max42500_wd_rhld { > + MAX42500_WD_RHOLD_0_MS, > + MAX42500_WD_RHOLD_8_MS, > + MAX42500_WD_RHOLD_16_MS, > + MAX42500_WD_RHOLD_32_MS, > + MAX42500_WD_RHOLD_MAX > +}; > + > +struct max42500_config { > + /* Packet error checking enable */ > + u8 pece; > + /* Enabled voltage monitor inputs */ > + u8 vmon_en; > + /* Voltage monitor power down enable */ > + u8 vmon_vmpd; > + /* Enabled voltage monitor reset mapping */ > + u8 reset_map; > + /* Watchdog mode */ > + enum max42500_wd_mode wd_mode; > + /* Watchdog clock div */ > + u8 wd_cdiv; > + /* Watchdog close window */ > + u8 wd_close; > + /* Watchdog open window */ > + u8 wd_open; > + /* Watchdog first update window */ > + u8 wd_1ud; > + /* Watchdog enable */ > + u8 wd_en; > +}; > + > +struct max42500_state { > + struct i2c_client *client; > + struct regmap *regmap; > + struct max42500_config *config; > + long pwrup_stamp[MAX42500_VM_MAX]; > + long pwrdn_stamp[MAX42500_VM_MAX]; > + u8 ov_thresh1[MAX42500_VM_MAX - 2]; > + u8 ov_thresh2[MAX42500_VM_MAX - 5]; > + u8 uv_thresh1[MAX42500_VM_MAX - 2]; > + u8 uv_thresh2[MAX42500_VM_MAX - 5]; > + u8 nominal_volt[MAX42500_VM_MAX - 2]; > + u8 comp_status[MAX42500_VM_MAX * MAX42500_COMP_STAT_MAX]; > +}; > + > +/************************ Functions Definitions **************************/ > +/** > + * @brief Read a raw value from a register. > + * @return 0 in case of success, error code otherwise. > + */ > +static int max42500_reg_read(struct max42500_state *st, > + u8 reg_addr, u8 *reg_data) > +{ > + int ret; > + u8 i2c_data[MAX42500_I2C_RD_FRAME_SIZE] = {0}; > + u8 bytes_num; > + u8 pece_value; > + > + /* PEC is computed over entire I2C frame from first START condition */ > + i2c_data[0] = (st->client->addr << 1); > + i2c_data[1] = reg_addr; > + i2c_data[2] = (st->client->addr << 1) | 0x1; > + > + /* I2C write target address */ > + bytes_num = 1; > + > + ret = regmap_bulk_write(st->regmap, reg_addr, &i2c_data[1], bytes_num); > + if (ret) > + return ret; > + > + /* Change byte count if PECE is enabled (1-byte data. 1-byte PEC) */ > + bytes_num = (st->config->pece) ? 2 : bytes_num; > + > + ret = regmap_bulk_read(st->regmap, reg_addr, &i2c_data[3], bytes_num); > + if (ret) > + return ret; > + > + if (st->config->pece) { > + /* Compute CRC over entire I2C frame */ > + pece_value = crc8(max42500_crc8, i2c_data, > + (MAX42500_I2C_RD_FRAME_SIZE - 1), 0); > + > + if (i2c_data[4] != pece_value) > + return -EIO; > + } > + > + *reg_data = i2c_data[3]; > + > + return 0; This seems to re-implement PEC support in the driver. As mentioned, PEC support should be handled in the i2c controller driver. Also, _if_ it is for some reasons necessary to implement chip access functions, those should be implemented as regmap bus. The functional part of the driver should just use regmap functions to access the chip. > +} > + > +/** > + * @brief Write a raw value to a register. > + * @return 0 in case of success, negative error code otherwise. > + */ > +static int max42500_reg_write(struct max42500_state *st, > + u8 reg_addr, u8 data) > +{ > + u8 i2c_data[MAX42500_I2C_WR_FRAME_SIZE] = {0}; > + u8 bytes_num; > + u8 pece_value; > + > + bytes_num = (st->config->pece) ? (MAX42500_I2C_WR_FRAME_SIZE - 1) : 2; > + i2c_data[0] = (st->client->addr << 1); > + i2c_data[1] = reg_addr; > + i2c_data[2] = (u8)(data & 0xFF); > + > + pece_value = 0; > + if (st->config->pece) > + pece_value = crc8(max42500_crc8, i2c_data, bytes_num, 0); > + > + i2c_data[0] = i2c_data[1]; > + i2c_data[1] = i2c_data[2]; > + i2c_data[2] = pece_value; > + > + return regmap_bulk_write(st->regmap, reg_addr, i2c_data, bytes_num); > +} > + > +/** > + * @brief Update a register's value based on a mask. > + * @return 0 in case of success, negative error code otherwise. Please fix. > + */ > +static int max42500_reg_update(struct max42500_state *st, > + u8 reg_addr, u8 mask, u8 data) > +{ > + int ret; > + u8 reg_data; > + > + ret = max42500_reg_read(st, reg_addr, ®_data); > + if (ret) > + return ret; > + > + reg_data &= ~mask; > + reg_data |= mask & data; > + > + return max42500_reg_write(st, reg_addr, reg_data); > +} > + > +/** > + * @brief Set nominal voltage for VM1 to VM5. > + * @return 0 in case of success, negative error code otherwise. > + */ > +static int max42500_set_nominal_voltage(struct max42500_state *st, > + enum max42500_vm_input vm_in, u8 voltage) > +{ > + u8 reg_addr; > + > + switch (vm_in) { > + case MAX42500_VM1: > + case MAX42500_VM2: > + case MAX42500_VM3: > + case MAX42500_VM4: > + if (voltage < MAX42500_MIN_VNOM || > + voltage > MAX42500_VNOM_MAX_VM1_VM4) > + return -EINVAL; The driver should use clamp_val() for value boundary control. > + reg_addr = MAX42500_REG_VIN1 + vm_in; > + break; > + case MAX42500_VM5: > + if (voltage < MAX42500_MIN_VNOM || > + voltage > MAX42500_VNOM_MAX_VM5) > + return -EINVAL; > + reg_addr = MAX42500_REG_VIN5; > + break; > + default: > + return -EINVAL; > + } > + > + st->nominal_volt[vm_in] = voltage; I fail to understand how this is supposed to work. The locally cached values are set (only) here, with a sysfs attribute. During probe they are not set, meaning the input attribute won't be available because it is only created if nominal_volt is set which is not the case at that time. That doesn't make sense. There needs to be some default. Also, the whole point of using regmap is that regmap provides caching. The code should use that facility instead of caching locally. > + return max42500_reg_write(st, reg_addr, voltage); > +} > + > +/** > + * @brief Get the status of the voltage monitor input. > + * @return 0 in case of success, negative error code otherwise. > + */ > +static int max42500_get_comp_status(struct max42500_state *st, > + u8 vm_in, u8 *status) > +{ > + int ret; > + u8 reg_addr; > + u8 vm_in_status; > + > + switch (vm_in % MAX42500_COMP_STAT_MAX) { > + case MAX42500_COMP_STAT_OFF: > + reg_addr = MAX42500_REG_STATOFF; > + break; > + case MAX42500_COMP_STAT_UV: > + reg_addr = MAX42500_REG_STATUV; > + break; > + case MAX42500_COMP_STAT_OV: > + reg_addr = MAX42500_REG_STATOV; > + break; > + default: > + return -EINVAL; > + } > + > + ret = max42500_reg_read(st, reg_addr, &vm_in_status); > + if (ret) > + return ret; > + > + switch (vm_in % MAX42500_VM_MAX) { > + case MAX42500_VM1: > + *status = (u8)FIELD_GET(BIT(MAX42500_VM1), vm_in_status); > + break; > + case MAX42500_VM2: > + *status = (u8)FIELD_GET(BIT(MAX42500_VM2), vm_in_status); > + break; > + case MAX42500_VM3: > + *status = (u8)FIELD_GET(BIT(MAX42500_VM3), vm_in_status); > + break; > + case MAX42500_VM4: > + *status = (u8)FIELD_GET(BIT(MAX42500_VM4), vm_in_status); > + break; > + case MAX42500_VM5: > + *status = (u8)FIELD_GET(BIT(MAX42500_VM5), vm_in_status); > + break; > + case MAX42500_VM6: > + *status = (u8)FIELD_GET(BIT(MAX42500_VM6), vm_in_status); > + break; > + case MAX42500_VM7: > + *status = (u8)FIELD_GET(BIT(MAX42500_VM7), vm_in_status); > + break; > + default: > + return -EINVAL; > + } > + st->comp_status[vm_in] = *status; > + > + return 0; > +} > + > +/** > + * @brief Set the overvoltage threshold of VM1 to VM5. > + * @return 0 in case of success, negative error code otherwise. > + */ > +static int max42500_set_ov_thresh1(struct max42500_state *st, > + enum max42500_vm_input vm_in, u8 thresh) > +{ > + if (thresh < MAX42500_MIN_THRESH_VM1_VM5 || > + thresh > MAX42500_MAX_THRESH_VM1_VM5) > + return -EINVAL; > + > + switch (vm_in) { > + case MAX42500_VM1: > + case MAX42500_VM2: > + case MAX42500_VM3: > + case MAX42500_VM4: > + case MAX42500_VM5: > + st->ov_thresh1[vm_in] = thresh; > + return max42500_reg_update(st, > + MAX42500_REG_OVUV1 + vm_in, > + GENMASK(7, 4), > + FIELD_PREP(GENMASK(7, 4), > + thresh)); > + default: > + return -EINVAL; > + } > +} > + > +/** > + * @brief Set the overvoltage threshold of VM6 and VM7. > + * @return 0 in case of success, negative error code otherwise. > + */ > +static int max42500_set_ov_thresh2(struct max42500_state *st, > + enum max42500_vm_input vm_in, u8 thresh) > +{ > + u8 reg_addr; > + > + if (thresh < MAX42500_MIN_THRESH_VM6_V7 || > + thresh > MAX42500_MAX_THRESH_VM6_V7) > + return -EINVAL; > + > + switch (vm_in) { > + case MAX42500_VM6: > + reg_addr = MAX42500_REG_VINO6; > + break; > + case MAX42500_VM7: > + reg_addr = MAX42500_REG_VINO7; > + break; > + default: > + return -EINVAL; > + } > + > + st->ov_thresh2[vm_in] = thresh; > + return max42500_reg_write(st, reg_addr, thresh); > +} > + > +/** > + * @brief Set the undervoltage threshold of VM1 to VM5. > + * @return 0 in case of success, negative error code otherwise. > + */ > +static int max42500_set_uv_thresh1(struct max42500_state *st, > + enum max42500_vm_input vm_in, u8 thresh) > +{ > + if (thresh < MAX42500_MIN_THRESH_VM1_VM5 || > + thresh > MAX42500_MAX_THRESH_VM1_VM5) > + return -EINVAL; > + > + switch (vm_in) { > + case MAX42500_VM1: > + case MAX42500_VM2: > + case MAX42500_VM3: > + case MAX42500_VM4: > + case MAX42500_VM5: > + st->uv_thresh1[vm_in] = thresh; > + return max42500_reg_update(st, > + MAX42500_REG_OVUV1 + vm_in, > + GENMASK(3, 0), > + thresh); > + default: > + return -EINVAL; > + } > +} > + > +/** > + * @brief Set the undervoltage threshold of VM6 and VM7. > + * @return 0 in case of success, negative error code otherwise. > + */ > +static int max42500_set_uv_thresh2(struct max42500_state *st, > + enum max42500_vm_input vm_in, u8 thresh) > +{ > + u8 reg_addr; > + > + if (thresh < MAX42500_MIN_THRESH_VM6_V7 || > + thresh > MAX42500_MAX_THRESH_VM6_V7) > + return -EINVAL; > + > + switch (vm_in) { > + case MAX42500_VM6: > + reg_addr = MAX42500_REG_VINU6; > + break; > + case MAX42500_VM7: > + reg_addr = MAX42500_REG_VINU7; > + break; > + default: > + return -EINVAL; > + } > + > + st->uv_thresh2[vm_in] = thresh; > + return max42500_reg_write(st, reg_addr, thresh); > +} > + > +/** > + * @brief Get the FPS clock divider value. > + * @return 0 in case of success, negative error code otherwise. > + */ > +static int max42500_get_fps_clk_div(struct max42500_state *st, > + u8 *fps_clk_div) > +{ > + int ret; > + u8 reg_val; > + > + ret = max42500_reg_read(st, MAX42500_REG_FPSCFG1, ®_val); > + if (ret) > + return ret; > + > + *fps_clk_div = (u8)FIELD_GET(GENMASK(2, 0), reg_val); > + > + return 0; > +} > + > +/** > + * @brief Get power-up timestamp for a specified voltage monitor input. > + * @return 0 in case of success, negative error code otherwise. > + */ > +static int max42500_get_power_up_timestamp(struct max42500_state *st, > + enum max42500_vm_input vm_in, long *timestamp) > +{ > + int ret; > + u8 reg_val; > + u8 fps_clk_div; > + > + ret = max42500_reg_read(st, MAX42500_REG_UTIME1 + vm_in, ®_val); > + if (ret) > + return ret; > + > + /* Check if the input voltage rose above the UV threshold */ > + if (reg_val == 0) { > + /* Input voltage never rose above UV threshold*/ > + *timestamp = 0; > + return 0; > + } > + > + ret = max42500_get_fps_clk_div(st, &fps_clk_div); > + if (ret) > + return ret; > + > + *timestamp = (reg_val - 1) * 25 * (1 << fps_clk_div); > + st->pwrup_stamp[vm_in] = *timestamp; > + return 0; > +} > + > +/** > + * @brief Get power-down timestamp for a specified voltage monitor input. > + * @return 0 in case of success, negative error code otherwise. > + */ > +static int max42500_get_power_down_timestamp(struct max42500_state *st, > + enum max42500_vm_input vm_in, long *timestamp) > +{ > + int ret; > + u8 reg_val; > + u8 fps_clk_div; > + > + ret = max42500_reg_read(st, MAX42500_REG_DTIME1 + vm_in, ®_val); > + if (ret) > + return ret; > + > + /* Check if the input voltage fell below the OFF threshold */ > + if (reg_val == 0) { > + /* Input voltage never fell below OFF threshold */ > + *timestamp = 0; > + return 0; > + } > + > + ret = max42500_get_fps_clk_div(st, &fps_clk_div); > + if (ret) > + return ret; > + > + *timestamp = (reg_val - 1) * 25 * (1 << fps_clk_div); > + st->pwrdn_stamp[vm_in] = *timestamp; > + return 0; > +} > + > +/** > + * @brief Enable/Disable watchdog > + * @return 0 in case of success, error code otherwise > + */ > +static int max42500_set_watchdog_enable(struct max42500_state *st, > + bool wd_enable) > +{ > + int ret; > + u8 reg_val; > + > + ret = max42500_reg_read(st, MAX42500_REG_WDCFG2, ®_val); > + if (ret) > + return ret; > + > + if (wd_enable) > + reg_val |= BIT(3); > + else > + reg_val &= ~BIT(3); > + > + return max42500_reg_write(st, MAX42500_REG_WDCFG2, reg_val); > +} > + > +/** > + * @brief 8-bit watchdog key computation. > + * @return 0 in case of success, negative error code otherwise. > + */ > +static int max42500_get_watchdog_key(struct max42500_state *st, > + u8 *new_wd_key) > +{ > + int ret; > + u8 curr_wd_key; > + > + ret = max42500_reg_read(st, MAX42500_REG_WDKEY, &curr_wd_key); > + if (ret) > + return ret; > + > + /* Calculate the new bit using the LFSR polynomial */ > + u8 new_bit = ((curr_wd_key >> 7) ^ > + (curr_wd_key >> 5) ^ > + (curr_wd_key >> 4) ^ > + (curr_wd_key >> 3)) & 0x01; > + > + /* Shift existing bits upwards to MSb and insert the new bit as LSb */ > + *new_wd_key = (curr_wd_key << 1) | new_bit; > + > + return 0; > +} > + > +/** > + * @brief Update the watchdog key based on the mode and current value. > + * @return 0 in case of success, error code otherwise. > + */ > +static int max42500_set_watchdog_key(struct max42500_state *st) > +{ > + int ret; > + u8 reg_val; > + u8 wd_key; > + u8 wd_mode; > + > + ret = max42500_reg_read(st, MAX42500_REG_WDKEY, &wd_key); > + if (ret) > + return ret; > + > + ret = max42500_reg_read(st, MAX42500_REG_WDCDIV, ®_val); > + if (ret) > + return ret; > + > + wd_mode = (u8)FIELD_GET(BIT(6), reg_val); > + > + /* Compute new watchdog key for challenge/response mode */ > + if (wd_mode == MAX42500_WD_MODE_CH_RESP) > + max42500_get_watchdog_key(st, &wd_key); > + > + return max42500_reg_write(st, MAX42500_REG_WDKEY, wd_key); > +} > + > +/** @brief Set watchdog reset hold time > + * @return 0 in case of success, error code otherwise > + */ > +static int max42500_set_watchdog_rhld(struct max42500_state *st, u8 rhld) > +{ > + return max42500_reg_update(st, > + MAX42500_REG_RSTCTRL, > + GENMASK(1, 0), > + rhld); > +} > + > +static umode_t max42500_is_visible(const void *data, > + enum hwmon_sensor_types type, u32 attr, int channel) > +{ > + const struct max42500_state *st = data; > + > + switch (type) { > + case hwmon_chip: > + switch (attr) { > + case hwmon_chip_in_reset_history: > + if (st->pwrup_stamp[channel]) > + return 0444; > + break; > + case hwmon_chip_curr_reset_history: > + if (st->pwrdn_stamp[channel]) > + return 0444; > + break; > + case hwmon_chip_register_tz: > + return 0200; > + case hwmon_chip_alarms: > + case hwmon_chip_update_interval: > + case hwmon_chip_temp_reset_history: > + case hwmon_chip_power_reset_history: > + return 0644; > + } > + break; > + case hwmon_in: > + switch (attr) { > + case hwmon_in_input: > + if (st->nominal_volt[channel]) > + return 0644; > + break; > + case hwmon_in_lowest: > + if (st->ov_thresh1[channel]) > + return 0644; > + break; > + case hwmon_in_highest: > + if (st->ov_thresh2[channel]) > + return 0644; > + break; > + case hwmon_in_min: > + if (st->uv_thresh1[channel]) > + return 0644; > + break; > + case hwmon_in_max: > + if (st->uv_thresh2[channel]) > + return 0644; > + break; > + case hwmon_in_label: > + if (st->comp_status[channel]) > + return 0444; > + break; > + } > + break; > + default: > + break; > + } > + > + return 0; > +} > + > +static int max42500_read_in(struct device *dev, u32 attr, int channel, > + long *value) > +{ > + struct max42500_state *st = dev_get_drvdata(dev); > + > + switch (attr) { > + case hwmon_in_label: > + return max42500_get_comp_status(st, channel, (u8 *)value); The label atribute is supposed to return a text. > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static int max42500_read_chip(struct device *dev, u32 attr, int channel, > + long *value) > +{ > + struct max42500_state *st = dev_get_drvdata(dev); > + > + switch (attr) { > + case hwmon_chip_in_reset_history: > + return max42500_get_power_up_timestamp(st, channel, value); > + case hwmon_chip_curr_reset_history: > + return max42500_get_power_down_timestamp(st, channel, value); > + case hwmon_chip_power_reset_history: > + return max42500_get_watchdog_key(st, (u8 *)value); > + case hwmon_chip_update_interval: > + return max42500_get_fps_clk_div(st, (u8 *)value); More ABI violations. > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static int max42500_read(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long *value) > +{ > + switch (type) { > + case hwmon_chip: > + return max42500_read_chip(dev, attr, channel, value); > + case hwmon_in: > + return max42500_read_in(dev, attr, channel, value); > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static int max42500_write_in(struct device *dev, u32 attr, int channel, > + long value) > +{ > + struct max42500_state *st = dev_get_drvdata(dev); > + > + switch (attr) { > + case hwmon_in_input: > + return max42500_set_nominal_voltage(st, channel, (u8)value); This is an ABI violation: inX_input attributes return voltages provided by the chip, and are not writeable. Looking into the datasheet, this has to be set using the min/max attributes. > + case hwmon_in_min: > + return max42500_set_uv_thresh1(st, channel, (u8)value); This is a severe ABI violation. First, again, "(u8)value" is unacceptable. Second, the ABI states that this is a value in mV, not a percentage. I don't know why the chip uses a concept of "nominal voltage" and percentages of that for thresholds. I undersatand that this makes working with the chip difficult, but that does not mean that ABI violations are acceptable. AFAICS the chip doesn't even report the current voltages, so any inX_input attributes must not be provided. The limit attributes must set limits in mV. > + case hwmon_in_max: > + return max42500_set_uv_thresh2(st, channel, (u8)value); > + case hwmon_in_lowest: > + return max42500_set_ov_thresh1(st, channel, (u8)value); > + case hwmon_in_highest: > + return max42500_set_ov_thresh2(st, channel, (u8)value); This looks like another ABI violation. _lowest and _highest_ attributes report historic values. > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static int max42500_write_chip(struct device *dev, u32 attr, int channel, > + long value) > +{ > + struct max42500_state *st = dev_get_drvdata(dev); > + > + switch (attr) { > + case hwmon_chip_temp_reset_history: > + return max42500_set_watchdog_rhld(st, (u8)value); > + case hwmon_chip_register_tz: > + return max42500_set_watchdog_key(st); > + case hwmon_chip_alarms: > + return max42500_set_watchdog_enable(st, (bool)value); > + default: The above all seem to abouse the ABI and need documentation and explanation to even be able to evaluate. On top of that, cutting off values such as by "(u8)value" is unacceptable; writing 256 would have the same result as writing 0. > + return -EOPNOTSUPP; > + } > +} > + > +static int max42500_write(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long value) > +{ > + switch (type) { > + case hwmon_chip: > + return max42500_write_chip(dev, attr, channel, value); > + case hwmon_in: > + return max42500_write_in(dev, attr, channel, value); > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static const struct max42500_config max42500_init = { > + .pece = true, > + .vmon_en = true, > + .vmon_vmpd = true, > + .reset_map = (MAX42500_RSTMAP_IN_MASK(MAX42500_VM2) | > + MAX42500_RSTMAP_IN_MASK(MAX42500_VM3) | > + MAX42500_RSTMAP_IN_MASK(MAX42500_VM4) | > + MAX42500_RSTMAP_PARM_MASK), > + .wd_mode = MAX42500_WD_MODE_SIMPLE, > + .wd_cdiv = 0x0, > + .wd_close = 0x0, > + .wd_open = 0x0, > + .wd_1ud = 0x0, > + .wd_en = true, > +}; > + > +static int max42500_init_client(struct max42500_state *st, > + struct device *dev) > +{ > + int err; > + u8 device_id; > + > + if (!st || !dev) > + return -EINVAL; How would any of those ever be NULL ? Please drop such unnecessary checks. > + > + crc8_populate_msb(max42500_crc8, CRC8_PEC); > + > + /* Check device silicon ID */ > + err = max42500_reg_read(st, MAX42500_REG_ID, &device_id); > + if (err) > + return err; > + > + if (device_id != MAX42500_SILICON_ID) > + return -ENODEV; > + > + /* Configure PEC */ > + err = max42500_reg_update(st, > + MAX42500_REG_CONFIG1, > + BIT(0), > + max42500_init.pece); > + if (err) > + return err; > + > + /* Set PEC enable/disable for subsequent register access */ > + st->config->pece = max42500_init.pece; > + > + /* Enable voltage monitor inputs */ > + err = max42500_reg_update(st, > + MAX42500_REG_VMON, > + GENMASK(6, 0), > + max42500_init.vmon_en); > + if (err) > + return err; > + > + /* Enable voltage monitor power-down */ > + err = max42500_reg_update(st, > + MAX42500_REG_VMON, > + BIT(7), > + max42500_init.vmon_vmpd); > + if (err) > + return err; > + > + /* Enable input OV/UV mapping to reset pin */ > + err = max42500_reg_update(st, > + MAX42500_REG_RSTMAP, > + GENMASK(6, 0), > + max42500_init.reset_map); > + if (err) > + return err; > + > + /* Set watchdog mode */ > + err = max42500_reg_update(st, > + MAX42500_REG_WDCDIV, > + BIT(6), > + max42500_init.wd_mode << 6); > + if (err) > + return err; > + > + /* Set watchdog clock div */ > + err = max42500_reg_update(st, > + MAX42500_REG_WDCDIV, > + GENMASK(5, 0), > + max42500_init.wd_cdiv); > + if (err) > + return err; > + > + /* Set watchdog open window */ > + err = max42500_reg_update(st, > + MAX42500_REG_WDCFG1, > + GENMASK(3, 0), > + max42500_init.wd_open); > + if (err) > + return err; > + > + /* Set watchdog close window */ > + err = max42500_reg_update(st, > + MAX42500_REG_WDCFG1, > + GENMASK(7, 4), > + max42500_init.wd_close); > + if (err) > + return err; > + > + /* Set watchdog first update window */ > + err = max42500_reg_update(st, > + MAX42500_REG_WDCFG2, > + GENMASK(2, 0), > + max42500_init.wd_1ud); > + if (err) > + return err; > + > + /* Set watchdog enable */ > + err = max42500_reg_update(st, > + MAX42500_REG_WDCFG2, > + BIT(3), > + max42500_init.wd_en); > + if (err) > + return err; > + > + /* Update parameters */ > + st->config->vmon_en = max42500_init.vmon_en; > + st->config->vmon_vmpd = max42500_init.vmon_vmpd; > + st->config->reset_map = max42500_init.reset_map; > + st->config->wd_mode = max42500_init.wd_mode; > + st->config->wd_cdiv = max42500_init.wd_cdiv; > + st->config->wd_open = max42500_init.wd_open; > + st->config->wd_close = max42500_init.wd_close; > + st->config->wd_1ud = max42500_init.wd_1ud; > + st->config->wd_en = max42500_init.wd_en; Why not just setting a pointer to max42500_init ? > + > + return 0; > +} > + > +static const struct hwmon_ops max42500_hwmon_ops = { > + .is_visible = max42500_is_visible, > + .read = max42500_read, > + .write = max42500_write, > +}; > + > +static const struct hwmon_channel_info *max42500_info[] = { > + HWMON_CHANNEL_INFO(chip, > + HWMON_C_IN_RESET_HISTORY | HWMON_C_CURR_RESET_HISTORY | > + HWMON_C_TEMP_RESET_HISTORY | HWMON_C_POWER_RESET_HISTORY | > + HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL | HWMON_C_ALARMS, > + HWMON_C_IN_RESET_HISTORY | HWMON_C_CURR_RESET_HISTORY, > + HWMON_C_IN_RESET_HISTORY | HWMON_C_CURR_RESET_HISTORY, > + HWMON_C_IN_RESET_HISTORY | HWMON_C_CURR_RESET_HISTORY, > + HWMON_C_IN_RESET_HISTORY | HWMON_C_CURR_RESET_HISTORY, > + HWMON_C_IN_RESET_HISTORY | HWMON_C_CURR_RESET_HISTORY, > + HWMON_C_IN_RESET_HISTORY | HWMON_C_CURR_RESET_HISTORY), > + HWMON_CHANNEL_INFO(in, > + HWMON_I_LABEL | HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_MIN | > + HWMON_I_HIGHEST | HWMON_I_MAX, > + HWMON_I_LABEL | HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_MIN | > + HWMON_I_HIGHEST | HWMON_I_MAX, > + HWMON_I_LABEL | HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_MIN, > + HWMON_I_LABEL | HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_MIN, > + HWMON_I_LABEL | HWMON_I_INPUT | HWMON_I_LOWEST | HWMON_I_MIN, > + HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, > + HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, > + HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, > + HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL), > + NULL > +}; > + > +static const struct hwmon_chip_info max42500_chip_info = { > + .ops = &max42500_hwmon_ops, > + .info = max42500_info, > +}; > + > +static const struct regmap_config max42500_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = 0x2D > +}; > + > +static int max42500_probe(struct i2c_client *client) > +{ > + struct device *hwmon_dev; > + struct max42500_state *st; > + int err; > + > + st = devm_kzalloc(&client->dev, sizeof(*st), GFP_KERNEL); > + if (!st) > + return -ENOMEM; > + > + st->client = client; > + st->regmap = devm_regmap_init_i2c(client, &max42500_regmap_config); > + if (IS_ERR(st->regmap)) > + return PTR_ERR(st->regmap); > + > + err = max42500_init_client(st, &client->dev); > + if (err) > + return err; > + > + hwmon_dev = devm_hwmon_device_register_with_info(&client->dev, > + client->name, st, &max42500_chip_info, NULL); > + > + return PTR_ERR_OR_ZERO(hwmon_dev); > +} > + > +static const struct of_device_id max42500_of_match[] = { > + { .compatible = "adi,max42500" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, max42500_of_match); > + > +static const struct i2c_device_id max42500_id[] = { > + { "max42500", 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, max42500_id); > + > +static struct i2c_driver max42500_driver = { > + .driver = { > + .name = "max42500", > + .of_match_table = max42500_of_match, > + }, > + .probe = max42500_probe, > + .id_table = max42500_id, > +}; > + > +module_i2c_driver(max42500_driver); > + > +MODULE_AUTHOR("Kent Libetario <kent.libetario@analog.com>"); > +MODULE_DESCRIPTION("Hwmon driver for MAX42500"); > +MODULE_LICENSE("GPL"); ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 2/2] drivers: hwmon: add driver for max42500 2024-12-21 18:08 ` Guenter Roeck @ 2025-03-10 9:05 ` Libetario, Kent 0 siblings, 0 replies; 6+ messages in thread From: Libetario, Kent @ 2025-03-10 9:05 UTC (permalink / raw) To: Guenter Roeck Cc: linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Conor Dooley > -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: Sunday, December 22, 2024 2:09 AM > To: Libetario, Kent <Kent.Libetario@analog.com> > Cc: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; Jean Delvare <jdelvare@suse.com>; Rob Herring > <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley > <conor+dt@kernel.org> > Subject: Re: [PATCH 2/2] drivers: hwmon: add driver for max42500 > > [External] > > On Fri, Dec 20, 2024 at 09:20:03AM +0800, Kent Libetario wrote: > > Add the core implementation of the MAX42500 hwmon driver. > > > > Please have a look into Documentation/process/submitting-patches.rst > and follow the guidance about describing patches. > Yes, I will look into the documentation and update the patch descriptions. > > Signed-off-by: Kent Libetario <Kent.Libetario@analog.com> > > --- > > MAINTAINERS | 1 + > > drivers/hwmon/Kconfig | 13 + > > drivers/hwmon/Makefile | 1 + > > drivers/hwmon/max42500.c | 1049 > > ++++++++++++++++++++++++++++++++++++++ > > Documentation missing. > > There is a large number of checkpatch CHECK messages, almost all about multi- > line alignment. Indeed, when looking through the code, multi-line aligment > seems arbitrary. Please fix. > > PEC support should be implemented through the i2c controller driver. > Please see other drivers such as lm90.c or max31827.c for examples how to do > that. > > Some more comments inline. The review is partial. The major problem I see is > that there are not just a few but _lots_ of ABI violations. > This is unacceptable. Please rework the driver and follow the hwmon ABI for all > sysfs attributes. > > Guenter > Yes, I have reworked the driver and that is why it took me sometime to reply from your review comments due to a large revision. I have fixed the alignment format issues updated the code and added the documentation as you instructed and from the driver references you provided. I have initiated also to divide the drivers into three, including hwmon driver. The other 2 drivers are the mfd and watchdog drivers using their own ABI sysfs attributes standards. Also, I have used debugfs for the non-standard attributes in all of three drivers that are not defined in their ABI sysfs document. Thanks, Kent > > 4 files changed, 1064 insertions(+) > > create mode 100644 drivers/hwmon/max42500.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS index > > d1703b4834c8..434191e16dd5 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -14051,6 +14051,7 @@ M: Kent Libetario <kent.libetario@analog.com> > > L: linux-hwmon@vger.kernel.org > > S: Supported > > F: Documentation/devicetree/bindings/hwmon/adi,max42500.yaml > > +F: drivers/hwmon/max42500.c > > > > MAX6650 HARDWARE MONITOR AND FAN CONTROLLER DRIVER > > L: linux-hwmon@vger.kernel.org > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index > > dd376602f3f1..ec0d7aad7789 100644 > > --- a/drivers/hwmon/Kconfig > > +++ b/drivers/hwmon/Kconfig > > @@ -1220,6 +1220,19 @@ config MAX31827 > > This driver can also be built as a module. If so, the module > > will be called max31827. > > > > +config SENSORS_MAX42500 > > + tristate "MAX42500 Industrial Power System Monitor Family" > > + depends on I2C > > + select REGMAP_I2C > > + help > > + If you say yes here you get support for MAX42500 SoC power-system > monitor > > + with up to seven voltage monitor. The driver also contains a > programmable > > + challenge/response watchdog, which is accessible through the I2C > interface, > > + along with a configurable RESET output. > > + > > + This driver can also be built as a module. If so, the module > > + will be called max42500. > > + > > config SENSORS_MAX6620 > > tristate "Maxim MAX6620 fan controller" > > depends on I2C > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index > > b827b92f2a78..d27d8fc01141 100644 > > --- a/drivers/hwmon/Makefile > > +++ b/drivers/hwmon/Makefile > > @@ -152,6 +152,7 @@ obj-$(CONFIG_SENSORS_MAX197) += max197.o > > obj-$(CONFIG_SENSORS_MAX31722) += max31722.o > > obj-$(CONFIG_SENSORS_MAX31730) += max31730.o > > obj-$(CONFIG_SENSORS_MAX31760) += max31760.o > > +obj-$(CONFIG_SENSORS_MAX42500) += max42500.o > > obj-$(CONFIG_SENSORS_MAX6620) += max6620.o > > obj-$(CONFIG_SENSORS_MAX6621) += max6621.o > > obj-$(CONFIG_SENSORS_MAX6639) += max6639.o > > diff --git a/drivers/hwmon/max42500.c b/drivers/hwmon/max42500.c new > > file mode 100644 index 000000000000..23b90c766767 > > --- /dev/null > > +++ b/drivers/hwmon/max42500.c > > @@ -0,0 +1,1049 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * MAX42500 - Industrial Power System Monitor > > + * > > + * Copyright 2024 Analog Devices Inc. > > + */ > > + > > +#include <linux/init.h> > > +#include <linux/jiffies.h> > > +#include <linux/slab.h> > > +#include <linux/crc8.h> > > +#include <linux/bitfield.h> > > +#include <linux/bitops.h> > > +#include <linux/err.h> > > +#include <linux/gpio.h> > > +#include <linux/gpio/driver.h> > > +#include <linux/hwmon.h> > > +#include <linux/i2c.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/property.h> > > +#include <linux/regmap.h> > > + > > +/* Implements Polynomial X^8 + X^2 + X^1 +1 */ > > +#define CRC8_PEC 0x07 > > + > > +DECLARE_CRC8_TABLE(max42500_crc8); > > + > > +#define MAX42500_REG_ID 0x00 > > +#define MAX42500_REG_CONFIG1 0x01 > > +#define MAX42500_REG_CONFIG2 0x02 > > +#define MAX42500_REG_VMON 0x03 > > +#define MAX42500_REG_RSTMAP 0x04 > > +#define MAX42500_REG_STATOV 0x05 > > +#define MAX42500_REG_STATUV 0x06 > > +#define MAX42500_REG_STATOFF 0x07 > > +#define MAX42500_REG_VIN1 0x08 > > +#define MAX42500_REG_VIN2 0x09 > > +#define MAX42500_REG_VIN3 0x0A > > +#define MAX42500_REG_VIN4 0x0B > > +#define MAX42500_REG_VIN5 0x0C > > +#define MAX42500_REG_VINO6 0x0D > > +#define MAX42500_REG_VINU6 0x0E > > +#define MAX42500_REG_VINO7 0x0F > > +#define MAX42500_REG_VINU7 0x10 > > +#define MAX42500_REG_OVUV1 0x11 > > +#define MAX42500_REG_OVUV2 0x12 > > +#define MAX42500_REG_OVUV3 0x13 > > +#define MAX42500_REG_OVUV4 0x14 > > +#define MAX42500_REG_OVUV5 0x15 > > +#define MAX42500_REG_FPSSTAT1 0x16 > > +#define MAX42500_REG_FPSCFG1 0x17 > > +#define MAX42500_REG_UTIME1 0x18 > > +#define MAX42500_REG_UTIME2 0x19 > > +#define MAX42500_REG_UTIME3 0x1A > > +#define MAX42500_REG_UTIME4 0x1B > > +#define MAX42500_REG_UTIME5 0x1C > > +#define MAX42500_REG_UTIME6 0x1D > > +#define MAX42500_REG_UTIME7 0x1E > > +#define MAX42500_REG_DTIME1 0x1F > > +#define MAX42500_REG_DTIME2 0x20 > > +#define MAX42500_REG_DTIME3 0x21 > > +#define MAX42500_REG_DTIME4 0x22 > > +#define MAX42500_REG_DTIME5 0x23 > > +#define MAX42500_REG_DTIME6 0x24 > > +#define MAX42500_REG_DTIME7 0x25 > > +#define MAX42500_REG_WDSTAT 0x26 > > +#define MAX42500_REG_WDCDIV 0x27 > > +#define MAX42500_REG_WDCFG1 0x28 > > +#define MAX42500_REG_WDCFG2 0x29 > > +#define MAX42500_REG_WDKEY 0x2A > > +#define MAX42500_REG_WDLOCK 0x2B > > +#define MAX42500_REG_RSTCTRL 0x2C > > +#define MAX42500_REG_CID 0x2D > > + > > +/** X is set based on the pull configuration of the ADDR pin */ > > +#define MAX42500_ADDR(x) (0x28 + (x)) > > +#define MAX42500_SILICON_ID (0x30) > > +#define MAX42500_I2C_WR_FRAME_SIZE (4) > > +#define MAX42500_I2C_RD_FRAME_SIZE (5) > > + > > +/** MAX42500 Nominal voltage computation */ > > +#define MAX42500_VNOM_MAX_VM1_VM4 0xFF // 3.6875v > > +#define MAX42500_VNOM_MAX_VM5 0xFF // 5.6v > > +#define MAX42500_MIN_VNOM 0x00 // 0.5v > > +#define MAX42500_VNOM_STEP_VM1_VM4 0x01 // 0.0125v > > +#define MAX42500_VNOM_STEP_VM5 0x01 // 0.02v > > + > > +/** MAX42500 Undervoltage/Overvoltage maximum and minimum > thresholds*/ > > +#define MAX42500_MAX_THRESH_VM1_VM5 0x0F // OV = 110.0%, > UV = 90.0% > > +#define MAX42500_MIN_THRESH_VM1_VM5 0x00 // OV = 102.5%, > UV = 97.5% > > +#define MAX42500_MAX_THRESH_VM6_V7 0xFF // 1.775v > > +#define MAX42500_MIN_THRESH_VM6_V7 0x00 // 0.5v > > + > > +/* CONFIG1 bit masks */ > > +#define MAX42500_CONFIG1_PECE_MASK BIT(0) > > +#define MAX42500_CONFIG1_MBST_MASK BIT(1) > > +#define MAX42500_CONFIG1_RR_MASK BIT(2) > > + > > +/* VMON bit masks */ > > +#define MAX42500_VMON_IN_MASK(bit) BIT(bit) > > +#define MAX42500_VMON_VMPD_MASK BIT(7) > > + > > +/* RSTMAP bit masks */ > > +#define MAX42500_RSTMAP_IN_MASK(bit) BIT(bit) > > +#define MAX42500_RSTMAP_PARM_MASK BIT(7) > > + > > +/* WDCDIV bit masks */ > > +#define MAX42500_WDCDIV_SWW_MASK BIT(6) > > +#define MAX42500_WDCDIV_WDIC_MASK (0x3F) > > + > > +/* WDCFG2 bit masks */ > > +#define MAX42500_WDCFG2_WDEN_MASK BIT(3) > > +#define MAX42500_WDCFG2_1UP_MASK (0x7) > > + > > +/* WDLOCK bit masks */ > > +#define MAX42500_WDLOCK_LOCK_MASK BIT(0) > > + > > +/* RSTCTRL bit masks */ > > +#define MAX42500_RSTCTRL_MR1_MASK BIT(2) > > +#define MAX42500_RSTCTRL_RHLD_MASK (0x3) > > + > > +/* MAX42500 device status */ > > +enum max42500_status { > > + MAX42500_STATUS_OFF, > > + MAX42500_STATUS_SLEEP, > > + MAX42500_STATUS_ON, > > + MAX42500_STATUS_MAX > > +}; > > + > > +/* MAX42500 voltage monitor input */ > > +enum max42500_vm_input { > > + MAX42500_VM1, > > + MAX42500_VM2, > > + MAX42500_VM3, > > + MAX42500_VM4, > > + MAX42500_VM5, > > + MAX42500_VM6, > > + MAX42500_VM7, > > + MAX42500_VM_MAX > > +}; > > + > > +/* MAX42500 comparator status */ > > +enum max42500_comp_stat { > > + MAX42500_COMP_STAT_OFF, > > + MAX42500_COMP_STAT_UV, > > + MAX42500_COMP_STAT_OV, > > + MAX42500_COMP_STAT_MAX > > +}; > > + > > +/* MAX42500 watchdog mode */ > > +enum max42500_wd_mode { > > + MAX42500_WD_MODE_CH_RESP, > > + MAX42500_WD_MODE_SIMPLE, > > + MAX42500_WD_MODE_MAX > > +}; > > + > > +/* MAX42500 reset hold/active timeout time. */ enum max42500_wd_rhld > > +{ > > + MAX42500_WD_RHOLD_0_MS, > > + MAX42500_WD_RHOLD_8_MS, > > + MAX42500_WD_RHOLD_16_MS, > > + MAX42500_WD_RHOLD_32_MS, > > + MAX42500_WD_RHOLD_MAX > > +}; > > + > > +struct max42500_config { > > + /* Packet error checking enable */ > > + u8 pece; > > + /* Enabled voltage monitor inputs */ > > + u8 vmon_en; > > + /* Voltage monitor power down enable */ > > + u8 vmon_vmpd; > > + /* Enabled voltage monitor reset mapping */ > > + u8 reset_map; > > + /* Watchdog mode */ > > + enum max42500_wd_mode wd_mode; > > + /* Watchdog clock div */ > > + u8 wd_cdiv; > > + /* Watchdog close window */ > > + u8 wd_close; > > + /* Watchdog open window */ > > + u8 wd_open; > > + /* Watchdog first update window */ > > + u8 wd_1ud; > > + /* Watchdog enable */ > > + u8 wd_en; > > +}; > > + > > +struct max42500_state { > > + struct i2c_client *client; > > + struct regmap *regmap; > > + struct max42500_config *config; > > + long pwrup_stamp[MAX42500_VM_MAX]; > > + long pwrdn_stamp[MAX42500_VM_MAX]; > > + u8 ov_thresh1[MAX42500_VM_MAX - 2]; > > + u8 ov_thresh2[MAX42500_VM_MAX - 5]; > > + u8 uv_thresh1[MAX42500_VM_MAX - 2]; > > + u8 uv_thresh2[MAX42500_VM_MAX - 5]; > > + u8 nominal_volt[MAX42500_VM_MAX - 2]; > > + u8 comp_status[MAX42500_VM_MAX * MAX42500_COMP_STAT_MAX]; }; > > + > > +/************************ Functions Definitions > > +**************************/ > > +/** > > + * @brief Read a raw value from a register. > > + * @return 0 in case of success, error code otherwise. > > + */ > > +static int max42500_reg_read(struct max42500_state *st, > > + u8 reg_addr, u8 > *reg_data) > > +{ > > + int ret; > > + u8 i2c_data[MAX42500_I2C_RD_FRAME_SIZE] = {0}; > > + u8 bytes_num; > > + u8 pece_value; > > + > > + /* PEC is computed over entire I2C frame from first START condition */ > > + i2c_data[0] = (st->client->addr << 1); > > + i2c_data[1] = reg_addr; > > + i2c_data[2] = (st->client->addr << 1) | 0x1; > > + > > + /* I2C write target address */ > > + bytes_num = 1; > > + > > + ret = regmap_bulk_write(st->regmap, reg_addr, &i2c_data[1], > bytes_num); > > + if (ret) > > + return ret; > > + > > + /* Change byte count if PECE is enabled (1-byte data. 1-byte PEC) */ > > + bytes_num = (st->config->pece) ? 2 : bytes_num; > > + > > + ret = regmap_bulk_read(st->regmap, reg_addr, &i2c_data[3], > bytes_num); > > + if (ret) > > + return ret; > > + > > + if (st->config->pece) { > > + /* Compute CRC over entire I2C frame */ > > + pece_value = crc8(max42500_crc8, i2c_data, > > + > (MAX42500_I2C_RD_FRAME_SIZE - 1), 0); > > + > > + if (i2c_data[4] != pece_value) > > + return -EIO; > > + } > > + > > + *reg_data = i2c_data[3]; > > + > > + return 0; > > This seems to re-implement PEC support in the driver. As mentioned, PEC > support should be handled in the i2c controller driver. > Yes, I will update the code to reflect you instructions to use the i2c driver's PEC implementation. > Also, _if_ it is for some reasons necessary to implement chip access functions, > those should be implemented as regmap bus. The functional part of the driver > should just use regmap functions to access the chip. > Yes, I agree and will update the code to use regmap functions as you instructed. > > +} > > + > > +/** > > + * @brief Write a raw value to a register. > > + * @return 0 in case of success, negative error code otherwise. > > + */ > > +static int max42500_reg_write(struct max42500_state *st, > > + u8 reg_addr, u8 data) > > +{ > > + u8 i2c_data[MAX42500_I2C_WR_FRAME_SIZE] = {0}; > > + u8 bytes_num; > > + u8 pece_value; > > + > > + bytes_num = (st->config->pece) ? (MAX42500_I2C_WR_FRAME_SIZE - 1) : > 2; > > + i2c_data[0] = (st->client->addr << 1); > > + i2c_data[1] = reg_addr; > > + i2c_data[2] = (u8)(data & 0xFF); > > + > > + pece_value = 0; > > + if (st->config->pece) > > + pece_value = crc8(max42500_crc8, i2c_data, bytes_num, 0); > > + > > + i2c_data[0] = i2c_data[1]; > > + i2c_data[1] = i2c_data[2]; > > + i2c_data[2] = pece_value; > > + > > + return regmap_bulk_write(st->regmap, reg_addr, i2c_data, bytes_num); > > +} > > + > > +/** > > + * @brief Update a register's value based on a mask. > > + * @return 0 in case of success, negative error code otherwise. > > Please fix. > Yes, will do. > > + */ > > +static int max42500_reg_update(struct max42500_state *st, > > + u8 reg_addr, u8 mask, > u8 data) { > > + int ret; > > + u8 reg_data; > > + > > + ret = max42500_reg_read(st, reg_addr, ®_data); > > + if (ret) > > + return ret; > > + > > + reg_data &= ~mask; > > + reg_data |= mask & data; > > + > > + return max42500_reg_write(st, reg_addr, reg_data); } > > + > > +/** > > + * @brief Set nominal voltage for VM1 to VM5. > > + * @return 0 in case of success, negative error code otherwise. > > + */ > > +static int max42500_set_nominal_voltage(struct max42500_state *st, > > + enum max42500_vm_input vm_in, u8 voltage) { > > + u8 reg_addr; > > + > > + switch (vm_in) { > > + case MAX42500_VM1: > > + case MAX42500_VM2: > > + case MAX42500_VM3: > > + case MAX42500_VM4: > > + if (voltage < MAX42500_MIN_VNOM || > > + voltage > MAX42500_VNOM_MAX_VM1_VM4) > > + return -EINVAL; > > The driver should use clamp_val() for value boundary control. > Yes, I will update the code according to your instructions. > > + reg_addr = MAX42500_REG_VIN1 + vm_in; > > + break; > > + case MAX42500_VM5: > > + if (voltage < MAX42500_MIN_VNOM || > > + voltage > MAX42500_VNOM_MAX_VM5) > > + return -EINVAL; > > + reg_addr = MAX42500_REG_VIN5; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + st->nominal_volt[vm_in] = voltage; > > I fail to understand how this is supposed to work. The locally cached values are > set (only) here, with a sysfs attribute. During probe they are not set, meaning > the input attribute won't be available because it is only created if nominal_volt > is set which is not the case at that time. That doesn't make sense. There needs > to be some default. > I will update the code to get first the latest value from the userspace by the sysfs attributes or from the device registers, since the device itself already has default OTP initial values setting. > Also, the whole point of using regmap is that regmap provides caching. The code > should use that facility instead of caching locally. > Yes, I will update the code to reflect the proper usage of regmap according to your instructions. > > + return max42500_reg_write(st, reg_addr, voltage); } > > + > > +/** > > + * @brief Get the status of the voltage monitor input. > > + * @return 0 in case of success, negative error code otherwise. > > + */ > > +static int max42500_get_comp_status(struct max42500_state *st, > > + u8 vm_in, u8 > *status) > > +{ > > + int ret; > > + u8 reg_addr; > > + u8 vm_in_status; > > + > > + switch (vm_in % MAX42500_COMP_STAT_MAX) { > > + case MAX42500_COMP_STAT_OFF: > > + reg_addr = MAX42500_REG_STATOFF; > > + break; > > + case MAX42500_COMP_STAT_UV: > > + reg_addr = MAX42500_REG_STATUV; > > + break; > > + case MAX42500_COMP_STAT_OV: > > + reg_addr = MAX42500_REG_STATOV; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + ret = max42500_reg_read(st, reg_addr, &vm_in_status); > > + if (ret) > > + return ret; > > + > > + switch (vm_in % MAX42500_VM_MAX) { > > + case MAX42500_VM1: > > + *status = (u8)FIELD_GET(BIT(MAX42500_VM1), vm_in_status); > > + break; > > + case MAX42500_VM2: > > + *status = (u8)FIELD_GET(BIT(MAX42500_VM2), vm_in_status); > > + break; > > + case MAX42500_VM3: > > + *status = (u8)FIELD_GET(BIT(MAX42500_VM3), vm_in_status); > > + break; > > + case MAX42500_VM4: > > + *status = (u8)FIELD_GET(BIT(MAX42500_VM4), vm_in_status); > > + break; > > + case MAX42500_VM5: > > + *status = (u8)FIELD_GET(BIT(MAX42500_VM5), vm_in_status); > > + break; > > + case MAX42500_VM6: > > + *status = (u8)FIELD_GET(BIT(MAX42500_VM6), vm_in_status); > > + break; > > + case MAX42500_VM7: > > + *status = (u8)FIELD_GET(BIT(MAX42500_VM7), vm_in_status); > > + break; > > + default: > > + return -EINVAL; > > + } > > + st->comp_status[vm_in] = *status; > > + > > + return 0; > > +} > > + > > +/** > > + * @brief Set the overvoltage threshold of VM1 to VM5. > > + * @return 0 in case of success, negative error code otherwise. > > + */ > > +static int max42500_set_ov_thresh1(struct max42500_state *st, > > + enum max42500_vm_input vm_in, u8 thresh) { > > + if (thresh < MAX42500_MIN_THRESH_VM1_VM5 || > > + thresh > MAX42500_MAX_THRESH_VM1_VM5) > > + return -EINVAL; > > + > > + switch (vm_in) { > > + case MAX42500_VM1: > > + case MAX42500_VM2: > > + case MAX42500_VM3: > > + case MAX42500_VM4: > > + case MAX42500_VM5: > > + st->ov_thresh1[vm_in] = thresh; > > + return max42500_reg_update(st, > > + > MAX42500_REG_OVUV1 + vm_in, > > + GENMASK(7, 4), > > + > FIELD_PREP(GENMASK(7, 4), > > + thresh)); > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +/** > > + * @brief Set the overvoltage threshold of VM6 and VM7. > > + * @return 0 in case of success, negative error code otherwise. > > + */ > > +static int max42500_set_ov_thresh2(struct max42500_state *st, > > + enum max42500_vm_input vm_in, u8 thresh) { > > + u8 reg_addr; > > + > > + if (thresh < MAX42500_MIN_THRESH_VM6_V7 || > > + thresh > MAX42500_MAX_THRESH_VM6_V7) > > + return -EINVAL; > > + > > + switch (vm_in) { > > + case MAX42500_VM6: > > + reg_addr = MAX42500_REG_VINO6; > > + break; > > + case MAX42500_VM7: > > + reg_addr = MAX42500_REG_VINO7; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + st->ov_thresh2[vm_in] = thresh; > > + return max42500_reg_write(st, reg_addr, thresh); } > > + > > +/** > > + * @brief Set the undervoltage threshold of VM1 to VM5. > > + * @return 0 in case of success, negative error code otherwise. > > + */ > > +static int max42500_set_uv_thresh1(struct max42500_state *st, > > + enum max42500_vm_input vm_in, u8 thresh) { > > + if (thresh < MAX42500_MIN_THRESH_VM1_VM5 || > > + thresh > MAX42500_MAX_THRESH_VM1_VM5) > > + return -EINVAL; > > + > > + switch (vm_in) { > > + case MAX42500_VM1: > > + case MAX42500_VM2: > > + case MAX42500_VM3: > > + case MAX42500_VM4: > > + case MAX42500_VM5: > > + st->uv_thresh1[vm_in] = thresh; > > + return max42500_reg_update(st, > > + MAX42500_REG_OVUV1 + vm_in, > > + GENMASK(3, 0), > > + thresh); > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +/** > > + * @brief Set the undervoltage threshold of VM6 and VM7. > > + * @return 0 in case of success, negative error code otherwise. > > + */ > > +static int max42500_set_uv_thresh2(struct max42500_state *st, > > + enum max42500_vm_input vm_in, u8 thresh) { > > + u8 reg_addr; > > + > > + if (thresh < MAX42500_MIN_THRESH_VM6_V7 || > > + thresh > MAX42500_MAX_THRESH_VM6_V7) > > + return -EINVAL; > > + > > + switch (vm_in) { > > + case MAX42500_VM6: > > + reg_addr = MAX42500_REG_VINU6; > > + break; > > + case MAX42500_VM7: > > + reg_addr = MAX42500_REG_VINU7; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + st->uv_thresh2[vm_in] = thresh; > > + return max42500_reg_write(st, reg_addr, thresh); } > > + > > +/** > > + * @brief Get the FPS clock divider value. > > + * @return 0 in case of success, negative error code otherwise. > > + */ > > +static int max42500_get_fps_clk_div(struct max42500_state *st, > > + u8 *fps_clk_div) > > +{ > > + int ret; > > + u8 reg_val; > > + > > + ret = max42500_reg_read(st, MAX42500_REG_FPSCFG1, ®_val); > > + if (ret) > > + return ret; > > + > > + *fps_clk_div = (u8)FIELD_GET(GENMASK(2, 0), reg_val); > > + > > + return 0; > > +} > > + > > +/** > > + * @brief Get power-up timestamp for a specified voltage monitor input. > > + * @return 0 in case of success, negative error code otherwise. > > + */ > > +static int max42500_get_power_up_timestamp(struct max42500_state *st, > > + enum max42500_vm_input vm_in, long *timestamp) { > > + int ret; > > + u8 reg_val; > > + u8 fps_clk_div; > > + > > + ret = max42500_reg_read(st, MAX42500_REG_UTIME1 + vm_in, > ®_val); > > + if (ret) > > + return ret; > > + > > + /* Check if the input voltage rose above the UV threshold */ > > + if (reg_val == 0) { > > + /* Input voltage never rose above UV threshold*/ > > + *timestamp = 0; > > + return 0; > > + } > > + > > + ret = max42500_get_fps_clk_div(st, &fps_clk_div); > > + if (ret) > > + return ret; > > + > > + *timestamp = (reg_val - 1) * 25 * (1 << fps_clk_div); > > + st->pwrup_stamp[vm_in] = *timestamp; > > + return 0; > > +} > > + > > +/** > > + * @brief Get power-down timestamp for a specified voltage monitor input. > > + * @return 0 in case of success, negative error code otherwise. > > + */ > > +static int max42500_get_power_down_timestamp(struct max42500_state *st, > > + enum max42500_vm_input vm_in, long *timestamp) { > > + int ret; > > + u8 reg_val; > > + u8 fps_clk_div; > > + > > + ret = max42500_reg_read(st, MAX42500_REG_DTIME1 + vm_in, > ®_val); > > + if (ret) > > + return ret; > > + > > + /* Check if the input voltage fell below the OFF threshold */ > > + if (reg_val == 0) { > > + /* Input voltage never fell below OFF threshold */ > > + *timestamp = 0; > > + return 0; > > + } > > + > > + ret = max42500_get_fps_clk_div(st, &fps_clk_div); > > + if (ret) > > + return ret; > > + > > + *timestamp = (reg_val - 1) * 25 * (1 << fps_clk_div); > > + st->pwrdn_stamp[vm_in] = *timestamp; > > + return 0; > > +} > > + > > +/** > > + * @brief Enable/Disable watchdog > > + * @return 0 in case of success, error code otherwise */ static int > > +max42500_set_watchdog_enable(struct max42500_state *st, > > + bool > wd_enable) > > +{ > > + int ret; > > + u8 reg_val; > > + > > + ret = max42500_reg_read(st, MAX42500_REG_WDCFG2, ®_val); > > + if (ret) > > + return ret; > > + > > + if (wd_enable) > > + reg_val |= BIT(3); > > + else > > + reg_val &= ~BIT(3); > > + > > + return max42500_reg_write(st, MAX42500_REG_WDCFG2, reg_val); } > > + > > +/** > > + * @brief 8-bit watchdog key computation. > > + * @return 0 in case of success, negative error code otherwise. > > + */ > > +static int max42500_get_watchdog_key(struct max42500_state *st, > > + u8 > *new_wd_key) > > +{ > > + int ret; > > + u8 curr_wd_key; > > + > > + ret = max42500_reg_read(st, MAX42500_REG_WDKEY, &curr_wd_key); > > + if (ret) > > + return ret; > > + > > + /* Calculate the new bit using the LFSR polynomial */ > > + u8 new_bit = ((curr_wd_key >> 7) ^ > > + (curr_wd_key >> 5) ^ > > + (curr_wd_key >> 4) ^ > > + (curr_wd_key >> 3)) & 0x01; > > + > > + /* Shift existing bits upwards to MSb and insert the new bit as LSb */ > > + *new_wd_key = (curr_wd_key << 1) | new_bit; > > + > > + return 0; > > +} > > + > > +/** > > + * @brief Update the watchdog key based on the mode and current value. > > + * @return 0 in case of success, error code otherwise. > > + */ > > +static int max42500_set_watchdog_key(struct max42500_state *st) { > > + int ret; > > + u8 reg_val; > > + u8 wd_key; > > + u8 wd_mode; > > + > > + ret = max42500_reg_read(st, MAX42500_REG_WDKEY, &wd_key); > > + if (ret) > > + return ret; > > + > > + ret = max42500_reg_read(st, MAX42500_REG_WDCDIV, ®_val); > > + if (ret) > > + return ret; > > + > > + wd_mode = (u8)FIELD_GET(BIT(6), reg_val); > > + > > + /* Compute new watchdog key for challenge/response mode */ > > + if (wd_mode == MAX42500_WD_MODE_CH_RESP) > > + max42500_get_watchdog_key(st, &wd_key); > > + > > + return max42500_reg_write(st, MAX42500_REG_WDKEY, wd_key); } > > + > > +/** @brief Set watchdog reset hold time > > + * @return 0 in case of success, error code otherwise */ static int > > +max42500_set_watchdog_rhld(struct max42500_state *st, u8 rhld) { > > + return max42500_reg_update(st, > > + > MAX42500_REG_RSTCTRL, > > + GENMASK(1, 0), > > + rhld); > > +} > > + > > +static umode_t max42500_is_visible(const void *data, > > + enum hwmon_sensor_types type, u32 attr, int channel) { > > + const struct max42500_state *st = data; > > + > > + switch (type) { > > + case hwmon_chip: > > + switch (attr) { > > + case hwmon_chip_in_reset_history: > > + if (st->pwrup_stamp[channel]) > > + return 0444; > > + break; > > + case hwmon_chip_curr_reset_history: > > + if (st->pwrdn_stamp[channel]) > > + return 0444; > > + break; > > + case hwmon_chip_register_tz: > > + return 0200; > > + case hwmon_chip_alarms: > > + case hwmon_chip_update_interval: > > + case hwmon_chip_temp_reset_history: > > + case hwmon_chip_power_reset_history: > > + return 0644; > > + } > > + break; > > + case hwmon_in: > > + switch (attr) { > > + case hwmon_in_input: > > + if (st->nominal_volt[channel]) > > + return 0644; > > + break; > > + case hwmon_in_lowest: > > + if (st->ov_thresh1[channel]) > > + return 0644; > > + break; > > + case hwmon_in_highest: > > + if (st->ov_thresh2[channel]) > > + return 0644; > > + break; > > + case hwmon_in_min: > > + if (st->uv_thresh1[channel]) > > + return 0644; > > + break; > > + case hwmon_in_max: > > + if (st->uv_thresh2[channel]) > > + return 0644; > > + break; > > + case hwmon_in_label: > > + if (st->comp_status[channel]) > > + return 0444; > > + break; > > + } > > + break; > > + default: > > + break; > > + } > > + > > + return 0; > > +} > > + > > +static int max42500_read_in(struct device *dev, u32 attr, int channel, > > + long *value) > > +{ > > + struct max42500_state *st = dev_get_drvdata(dev); > > + > > + switch (attr) { > > + case hwmon_in_label: > > + return max42500_get_comp_status(st, channel, (u8 *)value); > > The label atribute is supposed to return a text. > Yes, I agree and will update the code according to the ABI sysfs standards. > > + default: > > + return -EOPNOTSUPP; > > + } > > +} > > + > > +static int max42500_read_chip(struct device *dev, u32 attr, int channel, > > + long *value) > > +{ > > + struct max42500_state *st = dev_get_drvdata(dev); > > + > > + switch (attr) { > > + case hwmon_chip_in_reset_history: > > + return max42500_get_power_up_timestamp(st, channel, value); > > + case hwmon_chip_curr_reset_history: > > + return max42500_get_power_down_timestamp(st, channel, > value); > > + case hwmon_chip_power_reset_history: > > + return max42500_get_watchdog_key(st, (u8 *)value); > > + case hwmon_chip_update_interval: > > + return max42500_get_fps_clk_div(st, (u8 *)value); > > More ABI violations. > Yes, I agree and will update the code according to the ABI sysfs standards. > > + default: > > + return -EOPNOTSUPP; > > + } > > +} > > + > > +static int max42500_read(struct device *dev, enum hwmon_sensor_types > type, > > + u32 attr, int channel, long > *value) { > > + switch (type) { > > + case hwmon_chip: > > + return max42500_read_chip(dev, attr, channel, value); > > + case hwmon_in: > > + return max42500_read_in(dev, attr, channel, value); > > + default: > > + return -EOPNOTSUPP; > > + } > > +} > > + > > +static int max42500_write_in(struct device *dev, u32 attr, int channel, > > + long value) > > +{ > > + struct max42500_state *st = dev_get_drvdata(dev); > > + > > + switch (attr) { > > + case hwmon_in_input: > > + return max42500_set_nominal_voltage(st, channel, (u8)value); > > This is an ABI violation: inX_input attributes return voltages provided by the > chip, and are not writeable. Looking into the datasheet, this has to be set using > the min/max attributes. > Yes, I agree and will update the code to reflect your instructions and add the ABI documentation to address the violations. > > + case hwmon_in_min: > > + return max42500_set_uv_thresh1(st, channel, (u8)value); > > This is a severe ABI violation. First, again, "(u8)value" is unacceptable. > Second, the ABI states that this is a value in mV, not a percentage. > I don't know why the chip uses a concept of "nominal voltage" and percentages > of that for thresholds. I undersatand that this makes working with the chip > difficult, but that does not mean that ABI violations are acceptable. > > AFAICS the chip doesn't even report the current voltages, so any inX_input > attributes must not be provided. The limit attributes must set limits in mV. > Yes, I agree and have a discussion with the hardware application engineers. I will update the code to reflect the appropriate attributes that can be used for the "nominal voltage" and the percentages for the OV/UV thresholds. For the limits, can we use uV to address the decimal part of mV, e.g. 12.5mV? > > + case hwmon_in_max: > > + return max42500_set_uv_thresh2(st, channel, (u8)value); > > + case hwmon_in_lowest: > > + return max42500_set_ov_thresh1(st, channel, (u8)value); > > + case hwmon_in_highest: > > + return max42500_set_ov_thresh2(st, channel, (u8)value); > > This looks like another ABI violation. _lowest and _highest_ attributes report > historic values. > Yes, I agree and will update the code and add ABI documentation to address the violations. > > + default: > > + return -EOPNOTSUPP; > > + } > > +} > > + > > +static int max42500_write_chip(struct device *dev, u32 attr, int channel, > > + long value) > > +{ > > + struct max42500_state *st = dev_get_drvdata(dev); > > + > > + switch (attr) { > > + case hwmon_chip_temp_reset_history: > > + return max42500_set_watchdog_rhld(st, (u8)value); > > + case hwmon_chip_register_tz: > > + return max42500_set_watchdog_key(st); > > + case hwmon_chip_alarms: > > + return max42500_set_watchdog_enable(st, (bool)value); > > + default: > > The above all seem to abouse the ABI and need documentation and explanation > to even be able to evaluate. > Yes, I will add an ABI documentation to explain the attributes. > On top of that, cutting off values such as by "(u8)value" is unacceptable; writing > 256 would have the same result as writing 0. > Yes, I agree and will update these using bit ops macros to handle the values. > > + return -EOPNOTSUPP; > > + } > > +} > > + > > +static int max42500_write(struct device *dev, enum hwmon_sensor_types > type, > > + u32 attr, int channel, long > value) { > > + switch (type) { > > + case hwmon_chip: > > + return max42500_write_chip(dev, attr, channel, value); > > + case hwmon_in: > > + return max42500_write_in(dev, attr, channel, value); > > + default: > > + return -EOPNOTSUPP; > > + } > > +} > > + > > +static const struct max42500_config max42500_init = { > > + .pece = true, > > + .vmon_en = true, > > + .vmon_vmpd = true, > > + .reset_map = (MAX42500_RSTMAP_IN_MASK(MAX42500_VM2) | > > + > MAX42500_RSTMAP_IN_MASK(MAX42500_VM3) | > > + > MAX42500_RSTMAP_IN_MASK(MAX42500_VM4) | > > + MAX42500_RSTMAP_PARM_MASK), > > + .wd_mode = MAX42500_WD_MODE_SIMPLE, > > + .wd_cdiv = 0x0, > > + .wd_close = 0x0, > > + .wd_open = 0x0, > > + .wd_1ud = 0x0, > > + .wd_en = true, > > +}; > > + > > +static int max42500_init_client(struct max42500_state *st, > > + struct device *dev) > > +{ > > + int err; > > + u8 device_id; > > + > > + if (!st || !dev) > > + return -EINVAL; > > How would any of those ever be NULL ? Please drop such unnecessary checks. Yes, I will update the code to reflect these changes. > > + > > + crc8_populate_msb(max42500_crc8, CRC8_PEC); > > + > > + /* Check device silicon ID */ > > + err = max42500_reg_read(st, MAX42500_REG_ID, &device_id); > > + if (err) > > + return err; > > + > > + if (device_id != MAX42500_SILICON_ID) > > + return -ENODEV; > > + > > + /* Configure PEC */ > > + err = max42500_reg_update(st, > > + MAX42500_REG_CONFIG1, > > + BIT(0), > > + max42500_init.pece); > > + if (err) > > + return err; > > + > > + /* Set PEC enable/disable for subsequent register access */ > > + st->config->pece = max42500_init.pece; > > + > > + /* Enable voltage monitor inputs */ > > + err = max42500_reg_update(st, > > + MAX42500_REG_VMON, > > + GENMASK(6, 0), > > + max42500_init.vmon_en); > > + if (err) > > + return err; > > + > > + /* Enable voltage monitor power-down */ > > + err = max42500_reg_update(st, > > + MAX42500_REG_VMON, > > + BIT(7), > > + max42500_init.vmon_vmpd); > > + if (err) > > + return err; > > + > > + /* Enable input OV/UV mapping to reset pin */ > > + err = max42500_reg_update(st, > > + MAX42500_REG_RSTMAP, > > + GENMASK(6, 0), > > + max42500_init.reset_map); > > + if (err) > > + return err; > > + > > + /* Set watchdog mode */ > > + err = max42500_reg_update(st, > > + MAX42500_REG_WDCDIV, > > + BIT(6), > > + max42500_init.wd_mode << 6); > > + if (err) > > + return err; > > + > > + /* Set watchdog clock div */ > > + err = max42500_reg_update(st, > > + MAX42500_REG_WDCDIV, > > + GENMASK(5, 0), > > + max42500_init.wd_cdiv); > > + if (err) > > + return err; > > + > > + /* Set watchdog open window */ > > + err = max42500_reg_update(st, > > + MAX42500_REG_WDCFG1, > > + GENMASK(3, 0), > > + max42500_init.wd_open); > > + if (err) > > + return err; > > + > > + /* Set watchdog close window */ > > + err = max42500_reg_update(st, > > + MAX42500_REG_WDCFG1, > > + GENMASK(7, 4), > > + max42500_init.wd_close); > > + if (err) > > + return err; > > + > > + /* Set watchdog first update window */ > > + err = max42500_reg_update(st, > > + MAX42500_REG_WDCFG2, > > + GENMASK(2, 0), > > + max42500_init.wd_1ud); > > + if (err) > > + return err; > > + > > + /* Set watchdog enable */ > > + err = max42500_reg_update(st, > > + MAX42500_REG_WDCFG2, > > + BIT(3), > > + max42500_init.wd_en); > > + if (err) > > + return err; > > + > > + /* Update parameters */ > > + st->config->vmon_en = max42500_init.vmon_en; > > + st->config->vmon_vmpd = max42500_init.vmon_vmpd; > > + st->config->reset_map = max42500_init.reset_map; > > + st->config->wd_mode = max42500_init.wd_mode; > > + st->config->wd_cdiv = max42500_init.wd_cdiv; > > + st->config->wd_open = max42500_init.wd_open; > > + st->config->wd_close = max42500_init.wd_close; > > + st->config->wd_1ud = max42500_init.wd_1ud; > > + st->config->wd_en = max42500_init.wd_en; > > Why not just setting a pointer to max42500_init ? > This is a relic from the bare metal driver code. I will remove this since the device itself already has default OTP initial values setting. > > + > > + return 0; > > +} > > + > > +static const struct hwmon_ops max42500_hwmon_ops = { > > + .is_visible = max42500_is_visible, > > + .read = max42500_read, > > + .write = max42500_write, > > +}; > > + > > +static const struct hwmon_channel_info *max42500_info[] = { > > + HWMON_CHANNEL_INFO(chip, > > + HWMON_C_IN_RESET_HISTORY | > HWMON_C_CURR_RESET_HISTORY | > > + HWMON_C_TEMP_RESET_HISTORY | > HWMON_C_POWER_RESET_HISTORY | > > + HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL > | HWMON_C_ALARMS, > > + HWMON_C_IN_RESET_HISTORY | > HWMON_C_CURR_RESET_HISTORY, > > + HWMON_C_IN_RESET_HISTORY | > HWMON_C_CURR_RESET_HISTORY, > > + HWMON_C_IN_RESET_HISTORY | > HWMON_C_CURR_RESET_HISTORY, > > + HWMON_C_IN_RESET_HISTORY | > HWMON_C_CURR_RESET_HISTORY, > > + HWMON_C_IN_RESET_HISTORY | > HWMON_C_CURR_RESET_HISTORY, > > + HWMON_C_IN_RESET_HISTORY | > HWMON_C_CURR_RESET_HISTORY), > > + HWMON_CHANNEL_INFO(in, > > + HWMON_I_LABEL | HWMON_I_INPUT | > HWMON_I_LOWEST | HWMON_I_MIN | > > + HWMON_I_HIGHEST | HWMON_I_MAX, > > + HWMON_I_LABEL | HWMON_I_INPUT | > HWMON_I_LOWEST | HWMON_I_MIN | > > + HWMON_I_HIGHEST | HWMON_I_MAX, > > + HWMON_I_LABEL | HWMON_I_INPUT | > HWMON_I_LOWEST | HWMON_I_MIN, > > + HWMON_I_LABEL | HWMON_I_INPUT | > HWMON_I_LOWEST | HWMON_I_MIN, > > + HWMON_I_LABEL | HWMON_I_INPUT | > HWMON_I_LOWEST | HWMON_I_MIN, > > + HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, > HWMON_I_LABEL, > > + HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, > HWMON_I_LABEL, > > + HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, > HWMON_I_LABEL, > > + HWMON_I_LABEL, HWMON_I_LABEL, HWMON_I_LABEL, > HWMON_I_LABEL), > > + NULL > > +}; > > + > > +static const struct hwmon_chip_info max42500_chip_info = { > > + .ops = &max42500_hwmon_ops, > > + .info = max42500_info, > > +}; > > + > > +static const struct regmap_config max42500_regmap_config = { > > + .reg_bits = 8, > > + .val_bits = 8, > > + .max_register = 0x2D > > +}; > > + > > +static int max42500_probe(struct i2c_client *client) { > > + struct device *hwmon_dev; > > + struct max42500_state *st; > > + int err; > > + > > + st = devm_kzalloc(&client->dev, sizeof(*st), GFP_KERNEL); > > + if (!st) > > + return -ENOMEM; > > + > > + st->client = client; > > + st->regmap = devm_regmap_init_i2c(client, &max42500_regmap_config); > > + if (IS_ERR(st->regmap)) > > + return PTR_ERR(st->regmap); > > + > > + err = max42500_init_client(st, &client->dev); > > + if (err) > > + return err; > > + > > + hwmon_dev = devm_hwmon_device_register_with_info(&client->dev, > > + client->name, st, &max42500_chip_info, > NULL); > > + > > + return PTR_ERR_OR_ZERO(hwmon_dev); > > +} > > + > > +static const struct of_device_id max42500_of_match[] = { > > + { .compatible = "adi,max42500" }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, max42500_of_match); > > + > > +static const struct i2c_device_id max42500_id[] = { > > + { "max42500", 0 }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(i2c, max42500_id); > > + > > +static struct i2c_driver max42500_driver = { > > + .driver = { > > + .name = "max42500", > > + .of_match_table = max42500_of_match, > > + }, > > + .probe = max42500_probe, > > + .id_table = max42500_id, > > +}; > > + > > +module_i2c_driver(max42500_driver); > > + > > +MODULE_AUTHOR("Kent Libetario <kent.libetario@analog.com>"); > > +MODULE_DESCRIPTION("Hwmon driver for MAX42500"); > > +MODULE_LICENSE("GPL"); ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-03-10 9:31 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-12-22 23:15 [PATCH 2/2] drivers: hwmon: add driver for max42500 kernel test robot -- strict thread matches above, loose matches on Subject: below -- 2024-12-21 22:16 kernel test robot 2024-12-20 1:20 [PATCH 0/2] Add driver for MAX42500 Kent Libetario 2024-12-20 1:20 ` [PATCH 2/2] drivers: hwmon: add driver for max42500 Kent Libetario 2024-12-21 5:01 ` kernel test robot 2024-12-21 18:08 ` Guenter Roeck 2025-03-10 9:05 ` Libetario, Kent
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.