Re: [PATCH 2/2] hwmon:(pmbus/xdp730) Add support for efuse xdp730

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] Usage of firmware-specific `of_property_read_u32` instead of firmware-agnostic `device_property_read_u32`.
- [Low] The `xdp730_info` template structure is not marked as `const`.
- [Low] Missing explicit `#include <linux/regulator/consumer.h>` for `devm_regulator_get_enable()`.
--

commit c84eadae1a13e6d451b4eb98ba41bc9f1c1c537c
Author: Ashish Yadav <ashish.yadav@infineon.com>

hwmon:(pmbus/xdp730) Add support for efuse xdp730

Add the pmbus driver for Infineon XDP730 Digital eFuse Controller.

> diff --git a/drivers/hwmon/pmbus/xdp730.c b/drivers/hwmon/pmbus/xdp730.c
> new file mode 100644
> index 0000000000000..ea8968de9f267
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/xdp730.c
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Hardware monitoring driver for Infineon XDP730 Digital eFuse Controller
> + *
> + * Copyright (c) 2026 Infineon Technologies. All rights reserved.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/of_device.h>
> +#include <linux/bitops.h>
> +#include <linux/math64.h>

Is it possible to explicitly include <linux/regulator/consumer.h>?

The driver invokes devm_regulator_get_enable() below but relies on indirect
inclusion, which might break if subsystem headers are refactored in the
future.

> +#include "pmbus.h"
> +
> +/*
> + * The IMON resistor required to generate the system overcurrent protection.
> + * Arbitrary default Rimon value: 2k Ohm
> + */
> +#define XDP730_DEFAULT_RIMON 2000000000 /* 2k ohm */
> +#define XDP730_TELEMETRY_AVG 0xE9
> +
> +static struct pmbus_driver_info xdp730_info = {

Could this template structure be marked as const?

It is copied per-instance during probe via devm_kmemdup(), so declaring it
const would move it to the read-only .rodata section and prevent accidental
modifications.

> +	.pages = 1,
> +	.format[PSC_VOLTAGE_IN] = direct,
> +	.format[PSC_VOLTAGE_OUT] = direct,

[ ... ]

> +	ret >>= 10; /* 10th bit of TELEMETRY_AVG REG for GIMON Value */
> +	ret &= GENMASK(0, 0);
> +	if (ret == 1)
> +		gimon = 20000; /* output gain 20 microA/A */
> +	else
> +		gimon = 10000; /* output gain 10 microA/A */
> +
> +	if (of_property_read_u32(client->dev.of_node,
> +				 "infineon,rimon-micro-ohms", &rimon))

Should this use device_property_read_u32(&client->dev, ...) instead?

Using of_property_read_u32() restricts the driver to Device Tree-based
systems. If the driver is instantiated via ACPI, client->dev.of_node will
be NULL, causing the read to silently fail and fall back to the default value.

device_property_read_u32() from <linux/property.h> would seamlessly support
DT, ACPI, and software nodes.

> +		rimon = XDP730_DEFAULT_RIMON; /* Default if not set via DT */
> +	if (rimon == 0)
> +		return -EINVAL;
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519075558.91466-1-Ashish.Yadav@infineon.com?part=2