Linux-Hwmon Archive on lore.kernel.org
 help / color / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: Chu Lin <linchuyuan@google.com>
Cc: jdelvare@suse.com, linux-hwmon@vger.kernel.org,
	linux-kernel@vger.kernel.org, jasonling@google.com,
	belgaied@google.com, zhongqil@google.com
Subject: Re: [PATCH] hwmon:max6697: Allow max6581 to create tempX_offset attributes
Date: Thu, 2 Jul 2020 18:06:11 -0700
Message-ID: <20200703010611.GA100794@roeck-us.net> (raw)
In-Reply-To: <20200702014223.4096977-1-linchuyuan@google.com>

On Thu, Jul 02, 2020 at 01:42:23AM +0000, Chu Lin wrote:
> Per max6581, reg 4d and reg 4e is used for temperature read offset.
> This patch will let the user specify the temperature read offset for
> max6581. This patch is tested on max6581 and only applies to max6581.
> 
> Testing:
> echo 16250 > temp2_offset
> cat temp2_offset
> 16250
> 
> echo 17500 > temp3_offset
> cat temp3_offset
> 17500
> cat temp4_offset
> 0
> cat temp2_offset
> 17500
> 
> echo 0 > temp2_offset
> cat temp2_offset
> 0
> cat temp3_offset
> 17500
> 
> echo -0 > temp2_offset
> cat temp2_offset
> 0
> 
> echo -100000 > temp2_offset
> -sh: echo: write error: Invalid argument
> 
> cat temp2_input
> 37000
> 
> echo 10000 > temp2_offset
> cat temp2_input
> 47125
> 
> echo -2000 > temp2_offset
> cat temp2_input
> 34875
> 
> echo -0 > temp2_offset
> cat temp2_input
> 37000
> 
> Signed-off-by: Chu Lin <linchuyuan@google.com>

Please run "checkpatch --strict" and fix everything it reports.

> ---
>  drivers/hwmon/max6697.c | 129 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 125 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/max6697.c b/drivers/hwmon/max6697.c
> index 743752a2467a..b11fcb594807 100644
> --- a/drivers/hwmon/max6697.c
> +++ b/drivers/hwmon/max6697.c
> @@ -41,6 +41,14 @@ static const u8 MAX6697_REG_CRIT[] = {
>  #define MAX6697_MAP_BITS(reg)	((((reg) & 0x7e) >> 1) | \
>  				 (((reg) & 0x01) << 6) | ((reg) & 0x80))
>  
> +#define MAX6581_OFFSET_TO_MILLIC(reg)	(16000 * (((reg) & (1<<6)) >> 6) + \
> +					 8000 * (((reg) & (1<<5)) >> 5) + \
> +					 4000 * (((reg) & (1<<4)) >> 4) + \
> +					 2000 * (((reg) & (1<<3)) >> 3) + \
> +					 1000 * (((reg) & (1<<2)) >> 2) + \
> +					 500 * (((reg) & (1<<1)) >> 1) + \
> +					 250 * ((reg) & 1))

Why not just the following ?

#define MAX6581_OFFSET_TO_MILLIC(reg)	((reg) * 250)

> +
>  #define MAX6697_REG_STAT(n)		(0x44 + (n))
>  
>  #define MAX6697_REG_CONFIG		0x41
> @@ -56,6 +64,9 @@ static const u8 MAX6697_REG_CRIT[] = {
>  #define MAX6581_REG_IDEALITY_SELECT	0x4c
>  #define MAX6581_REG_OFFSET		0x4d
>  #define MAX6581_REG_OFFSET_SELECT	0x4e
> +#define MAX6581_OFFSET_MIN		-31750
> +#define MAX6581_OFFSET_MAX		-MAX6581_OFFSET_MIN
> +

Why the double negation ?

#define MAX6581_OFFSET_MIN		-31750
#define MAX6581_OFFSET_MAX		31750

or

#define MAX6581_OFFSET_MAX              31750
#define	MAX6581_OFFSET_MIN		-MAX6581_OFFSET_MAX
>  
>  #define MAX6697_CONV_TIME		156	/* ms per channel, worst case */
>  
> @@ -316,6 +327,94 @@ static ssize_t temp_store(struct device *dev,
>  	return ret < 0 ? ret : count;
>  }
>  
> +static ssize_t offset_store(struct device *dev,
> +			    struct device_attribute *devattr, const char *buf,
> +			    size_t count)
> +{
> +	long temp;
> +	u8 val, select;
> +	int i, temp_frac, temp_int, ret, index;
> +	bool sign;
> +	struct max6697_data *data;
> +
> +	index = to_sensor_dev_attr(devattr)->index;
> +	data = dev_get_drvdata(dev);
> +	select = i2c_smbus_read_byte_data(data->client,
> +					  MAX6581_REG_OFFSET_SELECT);
> +	if (select < 0)
> +		return -ENODATA;

Please return the error.

> +	ret = kstrtol(buf, 10, &temp);
> +	if (ret < 0)
> +		return ret;
> +	if (temp > MAX6581_OFFSET_MAX || temp < MAX6581_OFFSET_MIN)
> +		return -EINVAL;

Please use clamp_val() to limit the range. We don't expect users to know
chip limits / restrictions.

> +
> +	/* disable the offset for channel */
> +	if (temp == 0) {
> +		ret = i2c_smbus_write_byte_data(data->client,
> +						MAX6581_REG_OFFSET_SELECT,
> +						select & ~(1<<(index-1)));
> +		return ret < 0 ? ret : count;
> +	}
> +	sign = temp < 0 ? false : true;
> +	temp = abs(temp);
> +	temp_int = temp / 1000;
> +	temp_frac = temp % 1000;
> +	for (i = 4; i >= 0 && temp_int > 0; --i) {
> +		if (temp_int >= 1<<i) {
> +			temp_int -= 1<<i;
> +			val |= 1 << (i+2);
> +		}
> +	}
> +	/* special handle the fraction */
> +	if (temp_frac % 250 != 0)
> +		return -EINVAL;
> +	if (temp_frac == 750)
> +		val |= 0x3;
> +	else if (temp_frac == 500)
> +		val |= 0x2;
> +	else if (temp_frac)
> +		val |= 0x1;

This is way too complicated.

	sign = temp < 0;
	temp = clamp_val(abs(temp), 0, MAX6581_OFFSET_MAX);
	reg = DIV_ROUND_CLOSEST(temp, 250);
	if (sign)
		reg |= (1 << 7);

should accomplish the same.

> +	ret = i2c_smbus_write_byte_data(data->client,
> +					MAX6581_REG_OFFSET_SELECT,
> +					select | 1<<(index-1));
> +	if (ret < 0)
> +		return ret;
> +	ret = i2c_smbus_write_byte_data(data->client,
> +					MAX6581_REG_OFFSET,
> +					sign?val:(~val + 1));
> +	return ret < 0 ? ret : count;

Since this function requires multiple i2c accesses, it needs to be
mutex protected.


> +}
> +
> +static ssize_t offset_show(struct device *dev, struct device_attribute *devattr,
> +			   char *buf)
> +{
> +	int select, ret, index, temp;
> +	struct max6697_data *data;
> +
> +	index = to_sensor_dev_attr(devattr)->index;
> +	data = dev_get_drvdata(dev);
> +	select = i2c_smbus_read_byte_data(data->client,
> +				       MAX6581_REG_OFFSET_SELECT);
> +	if (select < 0)
> +		return -ENODATA;
> +	if (select & (1<<(index-1))) {
> +		ret = i2c_smbus_read_byte_data(data->client,
> +					       MAX6581_REG_OFFSET);
> +		if (ret < 0)
> +			return -ENODATA;
> +	} else {
> +		return sprintf(buf, "%d\n", 0);
> +	}
> +	if (1<<7 & ret) {

Not believer in Yoda Programming I am.

> +		ret = ~ret + 1;
> +		temp = -MAX6581_OFFSET_TO_MILLIC(ret);
> +	} else {
> +		temp = MAX6581_OFFSET_TO_MILLIC(ret);
> +	}

Negative vlaues should be handled in MAX6581_OFFSET_TO_MILLIC().
If necessary / too complex, make it a function.

> +	return sprintf(buf, "%d\n", temp);
> +}
> +
>  static SENSOR_DEVICE_ATTR_RO(temp1_input, temp_input, 0);
>  static SENSOR_DEVICE_ATTR_2_RW(temp1_max, temp, 0, MAX6697_TEMP_MAX);
>  static SENSOR_DEVICE_ATTR_2_RW(temp1_crit, temp, 0, MAX6697_TEMP_CRIT);
> @@ -374,6 +473,16 @@ static SENSOR_DEVICE_ATTR_RO(temp6_fault, alarm, 5);
>  static SENSOR_DEVICE_ATTR_RO(temp7_fault, alarm, 6);
>  static SENSOR_DEVICE_ATTR_RO(temp8_fault, alarm, 7);
>  
> +/* There is no offset for local temperature so starting from temp2 */

If so ...

> +static SENSOR_DEVICE_ATTR_RW(temp1_offset, offset, 0);

then why an attribute for temp1_offset ? Guess it is a dummy,
which is a waste. Just use dummy again.

> +static SENSOR_DEVICE_ATTR_RW(temp2_offset, offset, 1);
> +static SENSOR_DEVICE_ATTR_RW(temp3_offset, offset, 2);
> +static SENSOR_DEVICE_ATTR_RW(temp4_offset, offset, 3);
> +static SENSOR_DEVICE_ATTR_RW(temp5_offset, offset, 4);
> +static SENSOR_DEVICE_ATTR_RW(temp6_offset, offset, 5);
> +static SENSOR_DEVICE_ATTR_RW(temp7_offset, offset, 6);
> +static SENSOR_DEVICE_ATTR_RW(temp8_offset, offset, 7);
> +
>  static DEVICE_ATTR(dummy, 0, NULL, NULL);
>  
>  static umode_t max6697_is_visible(struct kobject *kobj, struct attribute *attr,
> @@ -382,8 +491,8 @@ static umode_t max6697_is_visible(struct kobject *kobj, struct attribute *attr,
>  	struct device *dev = container_of(kobj, struct device, kobj);
>  	struct max6697_data *data = dev_get_drvdata(dev);
>  	const struct max6697_chip_data *chip = data->chip;
> -	int channel = index / 6;	/* channel number */
> -	int nr = index % 6;		/* attribute index within channel */
> +	int channel = index / 8;	/* channel number */
> +	int nr = index % 7;		/* attribute index within channel */
>  
>  	if (channel >= chip->channels)
>  		return 0;
> @@ -392,6 +501,10 @@ static umode_t max6697_is_visible(struct kobject *kobj, struct attribute *attr,
>  		return 0;
>  	if (nr == 5 && !(chip->have_fault & (1 << channel)))
>  		return 0;
> +	/* offset reg is only supported on max6581 remote channels */
> +	if (nr == 6)
> +		if (data->type != max6581 || channel == 0)
> +			return 0;
>  
>  	return attr->mode;
>  }
> @@ -408,6 +521,7 @@ static struct attribute *max6697_attributes[] = {
>  	&sensor_dev_attr_temp1_crit.dev_attr.attr,
>  	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
>  	&dev_attr_dummy.attr,
> +	&sensor_dev_attr_temp1_offset.dev_attr.attr,
>  
>  	&sensor_dev_attr_temp2_input.dev_attr.attr,
>  	&sensor_dev_attr_temp2_max.dev_attr.attr,
> @@ -415,6 +529,7 @@ static struct attribute *max6697_attributes[] = {
>  	&sensor_dev_attr_temp2_crit.dev_attr.attr,
>  	&sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
>  	&sensor_dev_attr_temp2_fault.dev_attr.attr,
> +	&sensor_dev_attr_temp2_offset.dev_attr.attr,
>  
>  	&sensor_dev_attr_temp3_input.dev_attr.attr,
>  	&sensor_dev_attr_temp3_max.dev_attr.attr,
> @@ -422,6 +537,7 @@ static struct attribute *max6697_attributes[] = {
>  	&sensor_dev_attr_temp3_crit.dev_attr.attr,
>  	&sensor_dev_attr_temp3_crit_alarm.dev_attr.attr,
>  	&sensor_dev_attr_temp3_fault.dev_attr.attr,
> +	&sensor_dev_attr_temp3_offset.dev_attr.attr,
>  
>  	&sensor_dev_attr_temp4_input.dev_attr.attr,
>  	&sensor_dev_attr_temp4_max.dev_attr.attr,
> @@ -429,6 +545,7 @@ static struct attribute *max6697_attributes[] = {
>  	&sensor_dev_attr_temp4_crit.dev_attr.attr,
>  	&sensor_dev_attr_temp4_crit_alarm.dev_attr.attr,
>  	&sensor_dev_attr_temp4_fault.dev_attr.attr,
> +	&sensor_dev_attr_temp4_offset.dev_attr.attr,
>  
>  	&sensor_dev_attr_temp5_input.dev_attr.attr,
>  	&sensor_dev_attr_temp5_max.dev_attr.attr,
> @@ -436,6 +553,7 @@ static struct attribute *max6697_attributes[] = {
>  	&sensor_dev_attr_temp5_crit.dev_attr.attr,
>  	&sensor_dev_attr_temp5_crit_alarm.dev_attr.attr,
>  	&sensor_dev_attr_temp5_fault.dev_attr.attr,
> +	&sensor_dev_attr_temp5_offset.dev_attr.attr,
>  
>  	&sensor_dev_attr_temp6_input.dev_attr.attr,
>  	&sensor_dev_attr_temp6_max.dev_attr.attr,
> @@ -443,6 +561,7 @@ static struct attribute *max6697_attributes[] = {
>  	&sensor_dev_attr_temp6_crit.dev_attr.attr,
>  	&sensor_dev_attr_temp6_crit_alarm.dev_attr.attr,
>  	&sensor_dev_attr_temp6_fault.dev_attr.attr,
> +	&sensor_dev_attr_temp6_offset.dev_attr.attr,
>  
>  	&sensor_dev_attr_temp7_input.dev_attr.attr,
>  	&sensor_dev_attr_temp7_max.dev_attr.attr,
> @@ -450,6 +569,7 @@ static struct attribute *max6697_attributes[] = {
>  	&sensor_dev_attr_temp7_crit.dev_attr.attr,
>  	&sensor_dev_attr_temp7_crit_alarm.dev_attr.attr,
>  	&sensor_dev_attr_temp7_fault.dev_attr.attr,
> +	&sensor_dev_attr_temp7_offset.dev_attr.attr,
>  
>  	&sensor_dev_attr_temp8_input.dev_attr.attr,
>  	&sensor_dev_attr_temp8_max.dev_attr.attr,
> @@ -457,6 +577,7 @@ static struct attribute *max6697_attributes[] = {
>  	&sensor_dev_attr_temp8_crit.dev_attr.attr,
>  	&sensor_dev_attr_temp8_crit_alarm.dev_attr.attr,
>  	&sensor_dev_attr_temp8_fault.dev_attr.attr,
> +	&sensor_dev_attr_temp8_offset.dev_attr.attr,
>  	NULL
>  };
>  
> @@ -493,8 +614,8 @@ static void max6697_get_config_of(struct device_node *node,
>  	}
>  	prop = of_get_property(node, "transistor-ideality", &len);
>  	if (prop && len == 2 * sizeof(u32)) {
> -			pdata->ideality_mask = be32_to_cpu(prop[0]);
> -			pdata->ideality_value = be32_to_cpu(prop[1]);
> +		pdata->ideality_mask = be32_to_cpu(prop[0]);
> +		pdata->ideality_value = be32_to_cpu(prop[1]);
>  	}
>  }
>  

  parent reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-30 17:43 [PATCH] hwmon:max6697: Allow max6581 to set temperature read offset Chu Lin
2020-06-30 17:59 ` Guenter Roeck
2020-07-01 18:43 ` Chu Lin
2020-07-01 18:45 ` Chu Lin
2020-07-02  1:42 ` [PATCH] hwmon:max6697: Allow max6581 to create tempX_offset attributes Chu Lin
2020-07-02  5:14   ` Guenter Roeck
2020-07-02 16:38     ` Chu Lin
2020-07-03  1:06   ` Guenter Roeck [this message]
2020-07-06 17:54     ` [PATCH] hwmon:max6697: Allow max6581 to create tempX_offset Chu Lin
2020-07-06 19:32       ` 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=20200703010611.GA100794@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=belgaied@google.com \
    --cc=jasonling@google.com \
    --cc=jdelvare@suse.com \
    --cc=linchuyuan@google.com \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=zhongqil@google.com \
    /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

Linux-Hwmon Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hwmon/0 linux-hwmon/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hwmon linux-hwmon/ https://lore.kernel.org/linux-hwmon \
		linux-hwmon@vger.kernel.org
	public-inbox-index linux-hwmon

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hwmon


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git