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

[Marten Lindahl <martenli@axis.com>]

On Wed, Apr 27, 2022 at 03:26:56PM +0200, Guenter Roeck wrote:
> 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().

Ok, I will change the subject to your suggestion, and change those two
functions. Thanks!

Kind regards
Mårten
> 
> 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)