Re: [PATCH v2 2/2] hwmon: Add driver for TI INA232 Current and Power Monitor

[Krzysztof Kozlowski <krzk@kernel.org>]

On 10/01/2025 09:15, Leo Yang wrote:
> Support ina233 driver for Meta Yosemite V4.
> 
> Driver for Texas Instruments INA233 Current and Power Monitor
> With I2C-, SMBus-, and PMBus-Compatible Interface
> 
> Reported-by: kernel test robot <lkp@intel.com>

No, what did the robot report? Drop.

> Closes: https://lore.kernel.org/oe-kbuild-all/202501092213.X9mbPW5Q-lkp@intel.com/

Drop

> Closes: https://lore.kernel.org/oe-kbuild-all/202501061734.nPNdRKqO-lkp@intel.com/

Drop

> Signed-off-by: Leo Yang <Leo-Yang@quantatw.com>
> ---
>  Documentation/hwmon/ina233.rst |  77 ++++++++++++++
>  MAINTAINERS                    |   8 ++
>  drivers/hwmon/pmbus/Kconfig    |   9 ++
>  drivers/hwmon/pmbus/Makefile   |   1 +
>  drivers/hwmon/pmbus/ina233.c   | 184 +++++++++++++++++++++++++++++++++
>  5 files changed, 279 insertions(+)
>  create mode 100644 Documentation/hwmon/ina233.rst
>  create mode 100644 drivers/hwmon/pmbus/ina233.c
> 
> diff --git a/Documentation/hwmon/ina233.rst b/Documentation/hwmon/ina233.rst
> new file mode 100644
> index 000000000000..41537f89bed5
> --- /dev/null
> +++ b/Documentation/hwmon/ina233.rst
> @@ -0,0 +1,77 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver ina233
> +====================
> +
> +Supported chips:
> +
> +  * TI INA233
> +
> +    Prefix: 'ina233'
> +
> +  * Datasheet
> +
> +    Publicly available at the TI website : https://www.ti.com/lit/ds/symlink/ina233.pdf
> +
> +Author:
> +
> +	Leo Yang <Leo-Yang@quantatw.com>
> +
> +Usage Notes
> +-----------
> +
> +The shunt resistor value can be configured by a device tree property;
> +see Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml for details.
> +
> +
> +Description
> +-----------
> +
> +This driver supports hardware monitoring for TI INA233.
> +
> +The driver is a client driver to the core PMBus driver. Please see
> +Documentation/hwmon/pmbus.rst for details on PMBus client drivers.
> +
> +The driver provides the following attributes for input voltage:
> +
> +**in1_input**
> +
> +**in1_label**
> +
> +**in1_max**
> +
> +**in1_max_alarm**
> +
> +**in1_min**
> +
> +**in1_min_alarm**
> +
> +The driver provides the following attributes for shunt voltage:
> +
> +**in2_input**
> +
> +**in2_label**
> +
> +The driver provides the following attributes for output voltage:
> +
> +**in3_input**
> +
> +**in3_label**
> +
> +**in3_alarm**
> +
> +The driver provides the following attributes for output current:
> +
> +**curr1_input**
> +
> +**curr1_label**
> +
> +**curr1_max**
> +
> +**curr1_max_alarm**
> +
> +The driver provides the following attributes for input power:
> +
> +**power1_input**
> +
> +**power1_label**
> \ No newline at end of file

You still have patch warnings. I already commented on this, so you have
to fix it everywhere.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index c575de4903db..fde1713dff9d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11226,6 +11226,14 @@ L:	linux-fbdev@vger.kernel.org
>  S:	Orphan
>  F:	drivers/video/fbdev/imsttfb.c
>  
> +INA233 HARDWARE MONITOR DRIVER
> +M:	Leo Yang <Leo-Yang@quantatw.com>
> +M:	Leo Yang <leo.yang.sy0@gmail.com>

One email.

> +L:	linux-hwmon@vger.kernel.org
> +S:	Odd Fixes

Why would we like to have unmaintained driver? Odd fixes is candidate to
removal, so shall we accept it and remove immediately?


> +F:	Documentation/hwmon/ina233.rst
> +F:	drivers/hwmon/pmbus/ina233.c
> +

...

> +
> +	/* If INA233 skips current/power, shunt-resistor and current-lsb aren't needed.	*/
> +	/* read rshunt value (uOhm) */
> +	if (of_property_read_u32(client->dev.of_node, "shunt-resistor", &rshunt) < 0)
> +		rshunt = INA233_RSHUNT_DEFAULT;
> +
> +	/* read current_lsb value (uA) */
> +	if (of_property_read_u16(client->dev.of_node, "ti,current-lsb", &current_lsb) < 0)
> +		current_lsb = INA233_CURRENT_LSB_DEFAULT;
> +
> +	if (!rshunt || !current_lsb) {
> +		dev_err(&client->dev, "shunt-resistor and current-lsb cannot be zero.\n");

Then properties must have constraints in your schema.

> +		return -EINVAL;


Best regards,
Krzysztof