Re: [PATCH v3 1/3] hwmon: (pmbus) Use driver specific ops if they exist

Guenter Roeck
2022-04-27T13:27:32Z

On Wed, Apr 27, 2022 at 03:02:11PM +0200, Mårten Lindahl wrote:
> Pmbus drivers using the default pmbus_regulator_ops for the enable/
> disable/is_enabled functions will use the standard pmbus core functions
> pmbus_read/write_byte_data. This could potentially influence some
> specific regulator chips that for example need a time delay before each
> data access.
>
> Lets add support for drivers to use chip specific read/write operations
> when using the standard pmbus_regulator_ops.

The subject is misleading. It should be something like "introduce and
use write_byte_data callback". Also, existing code calling
pmbus_write_byte_data() should call _pmbus_write_byte_data() instead.
This applies to pmbus_update_fan() and pmbus_get_boolean().

Thanks,
Guenter

>
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> ---
> drivers/hwmon/pmbus/pmbus.h | 2 ++
> drivers/hwmon/pmbus/pmbus_core.c | 58 +++++++++++++++++++++-----------
> 2 files changed, 40 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index e74b6ef070f3..c031a9700ace 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -438,6 +438,8 @@ struct pmbus_driver_info {
> int (*read_byte_data)(struct i2c_client *client, int page, int reg);
> int (*read_word_data)(struct i2c_client *client, int page, int phase,
> int reg);
> + int (*write_byte_data)(struct i2c_client *client, int page, int reg,
> + u8 byte);
> int (*write_word_data)(struct i2c_client *client, int page, int reg,
> u16 word);
> int (*write_byte)(struct i2c_client *client, int page, u8 value);
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index b2618b1d529e..1b0728c3c7d8 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -384,25 +384,6 @@ int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg, u8 value)
> }
> EXPORT_SYMBOL_NS_GPL(pmbus_write_byte_data, PMBUS);
>
> -int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
> - u8 mask, u8 value)
> -{
> - unsigned int tmp;
> - int rv;
> -
> - rv = pmbus_read_byte_data(client, page, reg);
> - if (rv < 0)
> - return rv;
> -
> - tmp = (rv & ~mask) | (value & mask);
> -
> - if (tmp != rv)
> - rv = pmbus_write_byte_data(client, page, reg, tmp);
> -
> - return rv;
> -}
> -EXPORT_SYMBOL_NS_GPL(pmbus_update_byte_data, PMBUS);
> -
> /*
> * _pmbus_read_byte_data() is similar to pmbus_read_byte_data(), but checks if
> * a device specific mapping function exists and calls it if necessary.
> @@ -421,6 +402,43 @@ static int _pmbus_read_byte_data(struct i2c_client *client, int page, int reg)
> return pmbus_read_byte_data(client, page, reg);
> }
>
> +/*
> + * _pmbus_write_byte_data() is similar to pmbus_write_byte_data(), but checks if
> + * a device specific mapping function exists and calls it if necessary.
> + */
> +static int _pmbus_write_byte_data(struct i2c_client *client, int page, int reg, u8 value)
> +{
> + struct pmbus_data *data = i2c_get_clientdata(client);
> + const struct pmbus_driver_info *info = data->info;
> + int status;
> +
> + if (info->write_byte_data) {
> + status = info->write_byte_data(client, page, reg, value);
> + if (status != -ENODATA)
> + return status;
> + }
> + return pmbus_write_byte_data(client, page, reg, value);
> +}
> +
> +int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
> + u8 mask, u8 value)
> +{
> + unsigned int tmp;
> + int rv;
> +
> + rv = _pmbus_read_byte_data(client, page, reg);
> + if (rv < 0)
> + return rv;
> +
> + tmp = (rv & ~mask) | (value & mask);
> +
> + if (tmp != rv)
> + rv = _pmbus_write_byte_data(client, page, reg, tmp);
> +
> + return rv;
> +}
> +EXPORT_SYMBOL_NS_GPL(pmbus_update_byte_data, PMBUS);
> +
> static struct pmbus_sensor *pmbus_find_sensor(struct pmbus_data *data, int page,
> int reg)
> {
> @@ -2396,7 +2414,7 @@ static int pmbus_regulator_is_enabled(struct regulator_dev *rdev)
> int ret;
>
> mutex_lock(&data->update_lock);
> - ret = pmbus_read_byte_data(client, page, PMBUS_OPERATION);
> + ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
> mutex_unlock(&data->update_lock);
>
> if (ret < 0)