Re: [PATCH v2] hwmon: Add support for ina233

[Guenter Roeck <linux@roeck-us.net>]

On 5/14/24 02:22, Delphine CC Chiu wrote:
> Support ina233 with vshunt
> 
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@wiwynn.com>
> ---

I don't understand what you are doing here. The patch introducing ina233
support was not accepted, still waiting for you to apply the changes
I asked for. This is v2, which doesn't really add support for ina233
but adds a non-standard attribute on top of the not accepted patch.

As for reporting the shunt voltage, I don't mind, but it should be
reported as standard attribute. The pmbus core supports reporting
a "VMON" voltage (PMBUS_VIRT_READ_VMON) which you should use. I am not
going to accept a non-standard attribute to report a voltage.

Thanks,
Guenter

>   drivers/hwmon/pmbus/ina233.c | 37 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/ina233.c b/drivers/hwmon/pmbus/ina233.c
> index d5c7d7408ac3..33736e5049a9 100644
> --- a/drivers/hwmon/pmbus/ina233.c
> +++ b/drivers/hwmon/pmbus/ina233.c
> @@ -11,9 +11,43 @@
>   #include <linux/init.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
> +#include <linux/hwmon-sysfs.h>
>   #include "pmbus.h"
>   
>   #define MFR_CALIBRATION	0xd4
> +#define MFR_READ_VSHUNT 0xd1
> +
> +#define HIGHEST_BIT 15
> +
> +static ssize_t ina233_vshunt_show(struct device *dev,
> +				  struct device_attribute *devattr, char *buf)
> +{
> +	int shunt_volt;
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +
> +	shunt_volt = i2c_smbus_read_word_data(client, MFR_READ_VSHUNT);
> +	if (shunt_volt < 0)
> +		return shunt_volt;
> +
> +	// Is negative
> +	if (shunt_volt & (1 << HIGHEST_BIT))
> +		shunt_volt = ~(shunt_volt & ~(1 << HIGHEST_BIT)) + 1;
> +
> +	// This unit is mV.
> +	// LSB 2.5 μV is reference from spec 7.6.3.2 MFR_READ_VSHUNT
> +	int val = DIV_ROUND_UP(shunt_volt * 25, 10000);
> +
> +	return sysfs_emit(buf, "%d\n", val);
> +}
> +
> +static SENSOR_DEVICE_ATTR_RO(vshunt, ina233_vshunt, 1);
> +
> +static struct attribute *ina233_attrs[] = {
> +	&sensor_dev_attr_vshunt.dev_attr.attr,
> +	NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(ina233);
>   
>   struct pmbus_driver_info ina233_info = {
>   	.pages = 1,
> @@ -29,6 +63,7 @@ struct pmbus_driver_info ina233_info = {
>   	.m[PSC_VOLTAGE_OUT] = 8,
>   	.R[PSC_VOLTAGE_IN] = 2,
>   	.R[PSC_VOLTAGE_OUT] = 2,
> +	.groups = ina233_groups,
>   };
>   
>   static int ina233_probe(struct i2c_client *client)
> @@ -82,7 +117,7 @@ static struct i2c_driver ina233_driver = {
>   
>   module_i2c_driver(ina233_driver);
>   
> -MODULE_AUTHOR("Eli Huang <eli_huang@wiwynn.com>");
> +MODULE_AUTHOR("Delphine_CC_Chiu <Delphine_CC_Chiu@wiwynn.com>");
>   MODULE_DESCRIPTION("PMBus driver for Texas Instruments INA233 and compatible chips");
>   MODULE_LICENSE("GPL");
>   MODULE_IMPORT_NS(PMBUS);