All of lore.kernel.org
 help / color / mirror / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: "Václav Kubernát" <kubernat@cesnet.cz>,
	"Jean Delvare" <jdelvare@suse.com>,
	"Jonathan Corbet" <corbet@lwn.net>,
	linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap
Date: Wed, 19 May 2021 12:00:22 -0700	[thread overview]
Message-ID: <f34293db-7159-9022-1a8c-829310fe84e7@roeck-us.net> (raw)
In-Reply-To: <20210512013052.903297-1-kubernat@cesnet.cz>

On 5/11/21 6:30 PM, Václav Kubernát wrote:
> Converting the driver to use regmap makes it more generic. It also makes
> it a lot easier to debug through debugfs.
> 
> Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
> ---
>   drivers/hwmon/Kconfig    |   1 +
>   drivers/hwmon/max31790.c | 254 ++++++++++++++++++++-------------------
>   2 files changed, 133 insertions(+), 122 deletions(-)
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 54f04e61fb83..c2ec57672c4e 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1092,6 +1092,7 @@ config SENSORS_MAX6697
>   config SENSORS_MAX31790
>   	tristate "Maxim MAX31790 sensor chip"
>   	depends on I2C
> +	select REGMAP_I2C
>   	help
>   	  If you say yes here you get support for 6-Channel PWM-Output
>   	  Fan RPM Controller.
> diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
> index 2c6b333a28e9..e3765ce4444a 100644
> --- a/drivers/hwmon/max31790.c
> +++ b/drivers/hwmon/max31790.c
> @@ -12,6 +12,7 @@
>   #include <linux/init.h>
>   #include <linux/jiffies.h>
>   #include <linux/module.h>
> +#include <linux/regmap.h>
>   #include <linux/slab.h>
>   
>   /* MAX31790 registers */
> @@ -46,92 +47,53 @@
>   
>   #define NR_CHANNEL			6
>   
> +#define MAX31790_REG_USER_BYTE_67	0x67
> +
> +#define BULK_TO_U16(msb, lsb)		(((msb) << 8) + (lsb))
> +#define U16_MSB(num)			(((num) & 0xFF00) >> 8)
> +#define U16_LSB(num)			((num) & 0x00FF)
> +
> +static const struct regmap_range max31790_ro_range = {
> +	.range_min = MAX31790_REG_TACH_COUNT(0),
> +	.range_max = MAX31790_REG_PWMOUT(0) - 1,

This should be:
	.range_max = MAX31790_REG_PWM_DUTY_CYCLE(5) + 1,

> +};
> +
> +static const struct regmap_access_table max31790_wr_table = {
> +	.no_ranges = &max31790_ro_range,
> +	.n_no_ranges = 1,
> +};
> +
> +static const struct regmap_range max31790_volatile_ranges[] = {
> +	regmap_reg_range(MAX31790_REG_TACH_COUNT(0), MAX31790_REG_TACH_COUNT(12)),
> +	regmap_reg_range(MAX31790_REG_FAN_FAULT_STATUS2, MAX31790_REG_FAN_FAULT_STATUS1),

This should be:

         regmap_reg_range(MAX31790_REG_PWM_DUTY_CYCLE(0), MAX31790_REG_PWM_DUTY_CYCLE(5) + 1),
         regmap_reg_range(MAX31790_REG_TACH_COUNT(0), MAX31790_REG_TACH_COUNT(11) + 1),
         regmap_reg_range(MAX31790_REG_FAN_FAULT_STATUS2, MAX31790_REG_FAN_FAULT_STATUS1),

> +};
> +
> +static const struct regmap_access_table max31790_volatile_table = {
> +	.no_ranges = max31790_volatile_ranges,
> +	.n_no_ranges = 2,
> +	.n_yes_ranges = 0

         .yes_ranges = max31790_volatile_ranges,
         .n_yes_ranges = ARRAY_SIZE(max31790_volatile_ranges),

Initializing with 0 is not necessary for static variables.

> +};
> +

As I have already said and now confirmed, specifying volatile ranges as no_ranges
is wrong. This only "works" because the cache method is specified implicitly as
REGCACHE_NONE, ie it has no effect.

> +static const struct regmap_config max31790_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.reg_stride = 1,
> +	.max_register = MAX31790_REG_USER_BYTE_67,

Add:
	.num_reg_defaults_raw = MAX31790_REG_USER_BYTE_67 + 1,
	.cache_type = REGCACHE_FLAT,

for caching to really work.
	
> +	.wr_table = &max31790_wr_table,
> +	.volatile_table = &max31790_volatile_table
> +};
> +
>   /*
>    * Client data (each client gets its own)
>    */
>   struct max31790_data {
> -	struct i2c_client *client;
> +	struct regmap *regmap;
> +
>   	struct mutex update_lock;
> -	bool valid; /* zero until following fields are valid */
> -	unsigned long last_updated; /* in jiffies */
> -
> -	/* register values */
>   	u8 fan_config[NR_CHANNEL];
>   	u8 fan_dynamics[NR_CHANNEL];
> -	u16 fault_status;
> -	u16 tach[NR_CHANNEL * 2];
> -	u16 pwm[NR_CHANNEL];
> -	u16 target_count[NR_CHANNEL];
>   };
>   
> -static struct max31790_data *max31790_update_device(struct device *dev)
> -{
> -	struct max31790_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> -	struct max31790_data *ret = data;
> -	int i;
> -	int rv;
> -
> -	mutex_lock(&data->update_lock);
> -
> -	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> -		rv = i2c_smbus_read_byte_data(client,
> -				MAX31790_REG_FAN_FAULT_STATUS1);
> -		if (rv < 0)
> -			goto abort;
> -		data->fault_status = rv & 0x3F;
> -
> -		rv = i2c_smbus_read_byte_data(client,
> -				MAX31790_REG_FAN_FAULT_STATUS2);
> -		if (rv < 0)
> -			goto abort;
> -		data->fault_status |= (rv & 0x3F) << 6;
> -
> -		for (i = 0; i < NR_CHANNEL; i++) {
> -			rv = i2c_smbus_read_word_swapped(client,
> -					MAX31790_REG_TACH_COUNT(i));
> -			if (rv < 0)
> -				goto abort;
> -			data->tach[i] = rv;
> -
> -			if (data->fan_config[i]
> -			    & MAX31790_FAN_CFG_TACH_INPUT) {
> -				rv = i2c_smbus_read_word_swapped(client,
> -					MAX31790_REG_TACH_COUNT(NR_CHANNEL
> -								+ i));
> -				if (rv < 0)
> -					goto abort;
> -				data->tach[NR_CHANNEL + i] = rv;
> -			} else {
> -				rv = i2c_smbus_read_word_swapped(client,
> -						MAX31790_REG_PWMOUT(i));
> -				if (rv < 0)
> -					goto abort;
> -				data->pwm[i] = rv;
> -
> -				rv = i2c_smbus_read_word_swapped(client,
> -						MAX31790_REG_TARGET_COUNT(i));
> -				if (rv < 0)
> -					goto abort;
> -				data->target_count[i] = rv;
> -			}
> -		}
> -
> -		data->last_updated = jiffies;
> -		data->valid = true;
> -	}
> -	goto done;
> -
> -abort:
> -	data->valid = false;
> -	ret = ERR_PTR(rv);
> -
> -done:
> -	mutex_unlock(&data->update_lock);
> -
> -	return ret;
> -}
> -
>   static const u8 tach_period[8] = { 1, 2, 4, 8, 16, 32, 32, 32 };
>   
>   static u8 get_tach_period(u8 fan_dynamics)
> @@ -159,28 +121,75 @@ static u8 bits_for_tach_period(int rpm)
>   	return bits;
>   }
>   
> +static int read_reg_byte(struct regmap *regmap, u8 reg)
> +{
> +	int rv;
> +	int val;
> +
> +	rv = regmap_read(regmap, reg, &val);
> +	if (rv < 0)
> +		return rv;
> +
> +	return val;
> +}
> +
> +static int read_reg_word(struct regmap *regmap, u8 reg)
> +{
> +	int rv;
> +	u8 val_bulk[2];
> +
> +	rv = regmap_bulk_read(regmap, reg, val_bulk, 2);
> +	if (rv < 0)
> +		return rv;
> +
> +	return BULK_TO_U16(val_bulk[0], val_bulk[1]);
> +}
> +
> +static int write_reg_word(struct regmap *regmap, u8 reg, u16 val)
> +{
> +	u8 bulk_val[2];
> +
> +	bulk_val[0] = U16_MSB(val);
> +	bulk_val[1] = U16_LSB(val);
> +
> +	return regmap_bulk_write(regmap, reg, bulk_val, 2);
> +}
> +
>   static int max31790_read_fan(struct device *dev, u32 attr, int channel,
>   			     long *val)
>   {
> -	struct max31790_data *data = max31790_update_device(dev);
> -	int sr, rpm;
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> +	struct max31790_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	int tach, fault;
>   
>   	switch (attr) {
>   	case hwmon_fan_input:
> -		sr = get_tach_period(data->fan_dynamics[channel]);
> -		rpm = RPM_FROM_REG(data->tach[channel], sr);
> -		*val = rpm;
> +		tach = read_reg_word(regmap, MAX31790_REG_TACH_COUNT(channel));
> +		if (tach < 0)
> +			return tach;
> +
> +		*val = RPM_FROM_REG(tach, get_tach_period(data->fan_dynamics[channel]));
>   		return 0;
>   	case hwmon_fan_target:
> -		sr = get_tach_period(data->fan_dynamics[channel]);
> -		rpm = RPM_FROM_REG(data->target_count[channel], sr);
> -		*val = rpm;
> +		tach = read_reg_word(regmap, MAX31790_REG_TARGET_COUNT(channel));
> +		if (tach < 0)
> +			return tach;
> +
> +		*val = RPM_FROM_REG(tach, get_tach_period(data->fan_dynamics[channel]));
>   		return 0;
>   	case hwmon_fan_fault:
> -		*val = !!(data->fault_status & (1 << channel));
> +		if (channel > 6)
> +			fault = read_reg_byte(regmap, MAX31790_REG_FAN_FAULT_STATUS2);
> +		else
> +			fault = read_reg_byte(regmap, MAX31790_REG_FAN_FAULT_STATUS1);
> +
> +		if (fault < 0)
> +			return fault;
> +
> +		if (channel > 6)
> +			*val = !!(fault & (1 << (channel - 6)));
> +		else
> +			*val = !!(fault & (1 << channel));
>   		return 0;
>   	default:
>   		return -EOPNOTSUPP;
> @@ -191,7 +200,7 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
>   			      long val)
>   {
>   	struct max31790_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> +	struct regmap *regmap = data->regmap;
>   	int target_count;
>   	int err = 0;
>   	u8 bits;
> @@ -207,9 +216,10 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
>   			((data->fan_dynamics[channel] &
>   			  ~MAX31790_FAN_DYN_SR_MASK) |
>   			 (bits << MAX31790_FAN_DYN_SR_SHIFT));
> -		err = i2c_smbus_write_byte_data(client,
> -					MAX31790_REG_FAN_DYNAMICS(channel),
> -					data->fan_dynamics[channel]);
> +
> +		err = regmap_write(regmap,
> +				   MAX31790_REG_FAN_DYNAMICS(channel),
> +				   data->fan_dynamics[channel]);
>   		if (err < 0)
>   			break;
>   
> @@ -217,11 +227,11 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
>   		target_count = RPM_TO_REG(val, sr);
>   		target_count = clamp_val(target_count, 0x1, 0x7FF);
>   
> -		data->target_count[channel] = target_count << 5;
> +		target_count = target_count << 5;
>   
> -		err = i2c_smbus_write_word_swapped(client,
> -					MAX31790_REG_TARGET_COUNT(channel),
> -					data->target_count[channel]);
> +		err = write_reg_word(regmap,
> +				     MAX31790_REG_TARGET_COUNT(channel),
> +				     target_count);
>   		break;
>   	default:
>   		err = -EOPNOTSUPP;
> @@ -258,22 +268,22 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
>   static int max31790_read_pwm(struct device *dev, u32 attr, int channel,
>   			     long *val)
>   {
> -	struct max31790_data *data = max31790_update_device(dev);
> -	u8 fan_config;
> -
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
> -	fan_config = data->fan_config[channel];
> +	struct max31790_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	int read;
>   
>   	switch (attr) {
>   	case hwmon_pwm_input:
> -		*val = data->pwm[channel] >> 8;
> +		read = read_reg_word(regmap, MAX31790_REG_PWMOUT(channel));
> +		if (read < 0)
> +			return read;
> +
> +		*val = read >> 8;
>   		return 0;
>   	case hwmon_pwm_enable:
> -		if (fan_config & MAX31790_FAN_CFG_RPM_MODE)
> +		if (data->fan_config[channel] & MAX31790_FAN_CFG_RPM_MODE)
>   			*val = 2;
> -		else if (fan_config & MAX31790_FAN_CFG_TACH_INPUT_EN)
> +		else if (data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN)
>   			*val = 1;
>   		else
>   			*val = 0;
> @@ -287,7 +297,7 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
>   			      long val)
>   {
>   	struct max31790_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> +	struct regmap *regmap = data->regmap;
>   	u8 fan_config;
>   	int err = 0;
>   
> @@ -299,10 +309,7 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
>   			err = -EINVAL;
>   			break;
>   		}
> -		data->pwm[channel] = val << 8;
> -		err = i2c_smbus_write_word_swapped(client,
> -						   MAX31790_REG_PWMOUT(channel),
> -						   data->pwm[channel]);
> +		err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), val << 8);
>   		break;
>   	case hwmon_pwm_enable:
>   		fan_config = data->fan_config[channel];
> @@ -321,9 +328,9 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
>   			break;
>   		}
>   		data->fan_config[channel] = fan_config;
> -		err = i2c_smbus_write_byte_data(client,
> -					MAX31790_REG_FAN_CONFIG(channel),
> -					fan_config);
> +		err = regmap_write(regmap,
> +				   MAX31790_REG_FAN_CONFIG(channel),
> +				   fan_config);
>   		break;
>   	default:
>   		err = -EOPNOTSUPP;
> @@ -426,20 +433,18 @@ static const struct hwmon_chip_info max31790_chip_info = {
>   	.info = max31790_info,
>   };
>   
> -static int max31790_init_client(struct i2c_client *client,
> +static int max31790_init_client(struct regmap *regmap,
>   				struct max31790_data *data)
>   {
>   	int i, rv;
>   
>   	for (i = 0; i < NR_CHANNEL; i++) {
> -		rv = i2c_smbus_read_byte_data(client,
> -				MAX31790_REG_FAN_CONFIG(i));
> +		rv = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(i % NR_CHANNEL));
>   		if (rv < 0)
>   			return rv;
>   		data->fan_config[i] = rv;
>   
> -		rv = i2c_smbus_read_byte_data(client,
> -				MAX31790_REG_FAN_DYNAMICS(i));
> +		rv = read_reg_byte(regmap, MAX31790_REG_FAN_DYNAMICS(i));
>   		if (rv < 0)
>   			return rv;
>   		data->fan_dynamics[i] = rv;
> @@ -464,13 +469,18 @@ static int max31790_probe(struct i2c_client *client)
>   	if (!data)
>   		return -ENOMEM;
>   
> -	data->client = client;
>   	mutex_init(&data->update_lock);
>   
> +	data->regmap = devm_regmap_init_i2c(client, &max31790_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(dev, "failed to allocate register map\n");
> +		return PTR_ERR(data->regmap);
> +	}
> +
>   	/*
>   	 * Initialize the max31790 chip
>   	 */
> -	err = max31790_init_client(client, data);
> +	err = max31790_init_client(data->regmap, data);
>   	if (err)
>   		return err;
>   
> 


  parent reply	other threads:[~2021-05-19 19:00 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-12  1:30 [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap Václav Kubernát
2021-05-12  1:30 ` [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable Václav Kubernát
2021-05-12  1:32   ` Václav Kubernát
2021-05-18 14:59     ` Guenter Roeck
2021-05-19 23:11   ` Guenter Roeck
2021-05-12  1:30 ` [PATCH v5 3/5] hwmon: (max31790) Show 0 RPM/fault when input disabled Václav Kubernát
2021-05-12  1:30 ` [PATCH v5 4/5] hwmon: (max31790) Allow setting fan*_div Václav Kubernát
2021-05-12  1:30 ` [PATCH v5 5/5] hwmon: (max31790) Update documentation Václav Kubernát
2021-05-19 19:00 ` Guenter Roeck [this message]
2021-05-20  5:12 ` [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap Guenter Roeck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f34293db-7159-9022-1a8c-829310fe84e7@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=corbet@lwn.net \
    --cc=jdelvare@suse.com \
    --cc=kubernat@cesnet.cz \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.