Re: [PATCH v3 3/6] hwmon: (pmbus/pim4328) Add support for reading direct format coefficients

[Guenter Roeck]

On 5/24/21 8:02 AM, Erik Rosen wrote:
> Add support for reading and decoding direct format coefficients to
> the PMBus core driver. If the new flag PMBUS_USE_COEFFICIENTS_CMD
> is set, the driver will use the COEFFICIENTS register together with
> the information in the pmbus_sensor_attr structs to initialize
> relevant coefficients for the direct mode format.
>
> Signed-off-by: Erik Rosen <erik.rosen@xxxxxxxxxxxxx>
> ---
>   drivers/hwmon/pmbus/pmbus_core.c | 82 ++++++++++++++++++++++++++++++++
>   include/linux/pmbus.h            |  8 ++++
>   2 files changed, 90 insertions(+)
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 460cbfd716e4..32e29f6dee38 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2177,6 +2177,77 @@ static int pmbus_find_attributes(struct i2c_client *client,
>   	return ret;
>   }
>   
> +/*
> + * The pmbus_class_attr_map structure maps one sensor class to
> + * it's corresponding sensor attributes array.
> + */
> +struct pmbus_class_attr_map {
> +	enum pmbus_sensor_classes class;
> +	u8 nattr;

u8 doesn't save anything and on most architectures makes the code more
complicated. Just use int.

> +	const struct pmbus_sensor_attr *attr;
> +};
> +
> +static const struct pmbus_class_attr_map class_attr_map[] = {
> +	{
> +		.class = PSC_VOLTAGE_IN,
> +		.attr = voltage_attributes,
> +		.nattr = ARRAY_SIZE(voltage_attributes),
> +	}, {
> +		.class = PSC_VOLTAGE_OUT,
> +		.attr = voltage_attributes,
> +		.nattr = ARRAY_SIZE(voltage_attributes),
> +	}, {
> +		.class = PSC_CURRENT_IN,
> +		.attr = current_attributes,
> +		.nattr = ARRAY_SIZE(current_attributes),
> +	}, {
> +		.class = PSC_CURRENT_OUT,
> +		.attr = current_attributes,
> +		.nattr = ARRAY_SIZE(current_attributes),
> +	}, {
> +		.class = PSC_POWER,
> +		.attr = power_attributes,
> +		.nattr = ARRAY_SIZE(power_attributes),
> +	}, {
> +		.class = PSC_TEMPERATURE,
> +		.attr = temp_attributes,
> +		.nattr = ARRAY_SIZE(temp_attributes),
> +	}
> +};
> +
> +static int pmbus_init_coefficients(struct i2c_client *client,
> +				   struct pmbus_data *data)
> +{
> +	int i, n;
> +	int ret = 0;

Unnecessary initialization

> +	const struct pmbus_class_attr_map *map;
> +	const struct pmbus_sensor_attr *attr;
> +
> +	for (i = 0; i < ARRAY_SIZE(class_attr_map); i++) {
> +		map = &class_attr_map[i];
> +		if (data->info->format[map->class] != direct)
> +			continue;
> +		for (n = 0; n < map->nattr; n++) {
> +			attr = &map->attr[n];
> +			if (map->class != attr->class)
> +				continue;
> +			ret = pmbus_read_coefficients(client,
> +						      (struct pmbus_driver_info *)data->info,

This is not a good idea. data->info points to a constant. Is there a reason
to pass data instead of info as argument to this function ?

> +						      attr->class,
> +						      attr->reg);
> +			if (ret >= 0)
> +				break;
> +		}
> +		if (ret < 0) {
> +			dev_err(&client->dev, "No coefficients found for sensor class %d\n",
> +				map->class); > +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   /*
>    * Identify chip parameters.
>    * This function is called for all chips.
> @@ -2296,6 +2367,17 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
>   			return ret;
>   		}
>   	}
> +
> +	if (data->flags & PMBUS_USE_COEFFICIENTS_CMD) {
> +		if (!i2c_check_functionality(client->adapter,
> +					     I2C_FUNC_SMBUS_BLOCK_PROC_CALL))
> +			return -ENODEV;
> +
> +		ret = pmbus_init_coefficients(client, data);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>   	return 0;
>   }
>   
> diff --git a/include/linux/pmbus.h b/include/linux/pmbus.h
> index f720470b1bab..7fdc282dab5a 100644
> --- a/include/linux/pmbus.h
> +++ b/include/linux/pmbus.h
> @@ -52,6 +52,14 @@
>    */
>   #define PMBUS_NO_WRITE_PROTECT			BIT(4)
>   
> +/*
> + * PMBUS_USE_COEFFICIENTS_CMD
> + *
> + * When this flag is set the PMBus core driver will use the COEFFICIENTS
> + * register to initialize the coefficients for the direct mode format.
> + */
> +#define PMBUS_USE_COEFFICIENTS_CMD		BIT(5)
> +
>   struct pmbus_platform_data {
>   	u32 flags;		/* Device specific flags */
>