Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1686060 > unrolled thread

[PATCH linux v2 0/3] Export 1-wire thermal sensors as hwmon device

Back to article view | Back to linux.kernel

Contents

  [PATCH linux v2 0/3] Export 1-wire thermal sensors as hwmon device Jaghathiswari Rankappagounder Natarajan <jaghu@google.com> - 2017-07-12 23:50 +0200
    [PATCH linux v2 3/3] drivers: w1: add hwmon temp support for w1_therm Jaghathiswari Rankappagounder Natarajan <jaghu@google.com> - 2017-07-12 23:50 +0200
      Re: [linux,v2,3/3] drivers: w1: add hwmon temp support for w1_therm Guenter Roeck <linux@roeck-us.net> - 2017-07-13 17:20 +0200
    [PATCH linux v2 2/3] drivers: w1: refactor w1_slave_show to make the temp reading functionality separate Jaghathiswari Rankappagounder Natarajan <jaghu@google.com> - 2017-07-12 23:50 +0200
    Re: [PATCH linux v2 0/3] Export 1-wire thermal sensors as hwmon device Evgeniy Polyakov <zbr@ioremap.net> - 2017-07-13 17:40 +0200
      Re: [PATCH linux v2 0/3] Export 1-wire thermal sensors as hwmon device Evgeniy Polyakov <zbr@ioremap.net> - 2017-07-13 17:40 +0200
        Re: [PATCH linux v2 0/3] Export 1-wire thermal sensors as hwmon device Jaghathiswari Rankappagounder Natarajan <jaghu@google.com> - 2017-07-17 18:10 +0200
          Re: [PATCH linux v2 0/3] Export 1-wire thermal sensors as hwmon  device Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-07-17 18:30 +0200

#1686060 — [PATCH linux v2 0/3] Export 1-wire thermal sensors as hwmon device

Our board has 4 DS18B20 1-wire temperature sensors. Each 1-wire bus and the
sensor under it is already configured against the Linux 1-wire driver
(called w1). They have a sysfs file(e.g.
/sys/bus/w1/devices/w1_bus_master1/28-000007704f4c/w1_slave) which, when read,
runs code to read the temperature. We'd like the temperatures to show up in
hwmon, so that the BMC IPMI sensor plumbing can forward those to host.

The changes in version 2 are mentioned in the individual patches.

This patchset is based on linux mainline version v4.10.

Tested:
Yes; On a board with 4 DS18B20 1-wire temperature sensors:
root@zaius:/sys/class/hwmon# ls
hwmon0  hwmon1  hwmon2  hwmon3  hwmon4  hwmon5
root@zaius:/sys/class/hwmon# cd hwmon0
root@zaius:/sys/class/hwmon/hwmon0# ls
device       name         subsystem    temp1_input  uevent
root@zaius:/sys/class/hwmon/hwmon0# cat temp1_input
24500
root@zaius:/sys/class/hwmon/hwmon0# cd ..
root@zaius:/sys/class/hwmon# cd hwmon1
root@zaius:/sys/class/hwmon/hwmon1# cat temp1_input
26562
root@zaius:/sys/class/hwmon/hwmon1# cd ..
root@zaius:/sys/class/hwmon# cd hwmon2
root@zaius:/sys/class/hwmon/hwmon2# cat temp1_input
27250
root@zaius:/sys/class/hwmon/hwmon2# cd ..
root@zaius:/sys/class/hwmon# cd hwmon3
root@zaius:/sys/class/hwmon/hwmon3# cat temp1_input
22250
root@zaius:/sys/class/hwmon/hwmon3#

Jaghathiswari Rankappagounder Natarajan (3):
  drivers: w1: add hwmon support structures
  drivers: w1: refactor w1_slave_show to make the temp reading
    functionality separate
  drivers: w1: add hwmon temp support for w1_therm

 drivers/w1/slaves/w1_therm.c | 197 ++++++++++++++++++++++++++++++++++++-------
 drivers/w1/w1.c              |  21 ++++-
 drivers/w1/w1.h              |   2 +
 drivers/w1/w1_family.h       |   2 +
 4 files changed, 191 insertions(+), 31 deletions(-)

--
2.13.2.932.g7449e964c-goog

[toc] | [next] | [standalone]

#1686063 — [PATCH linux v2 3/3] drivers: w1: add hwmon temp support for w1_therm

This change adds hwmon temp support for w1_therm.

Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
---
v2
- make changes to support hwmon_device_register_with_info mentioned by Guenter.

 drivers/w1/slaves/w1_therm.c | 117 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 117 insertions(+)

diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index 88f4cc8c3908..147f9dfa13de 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -29,6 +29,7 @@
 #include <linux/types.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
+#include <linux/hwmon.h>

 #include "../w1.h"
 #include "../w1_int.h"
@@ -118,19 +119,61 @@ static struct attribute *w1_ds28ea00_attrs[] = {
 	&dev_attr_w1_seq.attr,
 	NULL,
 };
+
 ATTRIBUTE_GROUPS(w1_therm);
 ATTRIBUTE_GROUPS(w1_ds28ea00);

+static umode_t w1_is_visible(const void *_data,
+			     enum hwmon_sensor_types type,
+			     u32 attr, int channel);
+
+static int w1_read_temp(struct device *dev, u32 attr, int channel,
+			long *val);
+
+static int w1_read(struct device *dev, enum hwmon_sensor_types type,
+		   u32 attr, int channel, long *val);
+
+static int w1_write(struct device *dev, enum hwmon_sensor_types type,
+		    u32 attr, int channel, long val);
+
+static const u32 w1_temp_config[] = {
+	HWMON_T_INPUT,
+	0
+};
+
+static const struct hwmon_channel_info w1_temp = {
+	.type = hwmon_temp,
+	.config = w1_temp_config,
+};
+
+static const struct hwmon_channel_info *w1_info[] = {
+	&w1_temp,
+	NULL
+};
+
+static const struct hwmon_ops w1_hwmon_ops = {
+	.is_visible = w1_is_visible,
+	.read = w1_read,
+	.write = w1_write,
+};
+
+static const struct hwmon_chip_info w1_chip_info = {
+	.ops = &w1_hwmon_ops,
+	.info = w1_info,
+};
+
 static struct w1_family_ops w1_therm_fops = {
 	.add_slave	= w1_therm_add_slave,
 	.remove_slave	= w1_therm_remove_slave,
 	.groups		= w1_therm_groups,
+	.chip_info	= &w1_chip_info,
 };

 static struct w1_family_ops w1_ds28ea00_fops = {
 	.add_slave	= w1_therm_add_slave,
 	.remove_slave	= w1_therm_remove_slave,
 	.groups		= w1_ds28ea00_groups,
+	.chip_info	= &w1_chip_info,
 };

 static struct w1_family w1_therm_family_DS18S20 = {
@@ -559,6 +602,80 @@ static ssize_t w1_slave_show(struct device *device,
 	return ret;
 }

+static int w1_read_temp(struct device *device, u32 attr, int channel,
+			long *val)
+{
+	struct w1_slave *sl = dev_get_drvdata(device);
+	u8 *family_data = sl->family_data;
+	struct therm_info info;
+	int ret;
+
+	if (!sl->family_data)
+		return -ENODEV;
+
+	/* prevent the slave from going away in sleep */
+	atomic_inc(THERM_REFCNT(family_data));
+
+	switch (attr) {
+	case hwmon_temp_input:
+	ret = read_therm(device, sl, &info);
+	if (ret)
+		goto dec_refcnt;
+
+	if (!info.verdict) {
+		dev_warn(device, "Read failed CRC check\n");
+		ret = -EIO;
+		goto dec_refcnt;
+	}
+
+	*val = w1_convert_temp(info.rom, sl->family->fid);
+	ret = 0;
+	break;
+	default:
+		ret = -EOPNOTSUPP;
+		break;
+	}
+
+dec_refcnt:
+	atomic_dec(THERM_REFCNT(family_data));
+	return ret;
+}
+
+static umode_t w1_is_visible(const void *_data, enum hwmon_sensor_types type,
+			     u32 attr, int channel)
+{
+	umode_t mode = 0444;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		break;
+	default:
+		mode = 0;
+		break;
+	}
+	return mode;
+}
+
+static int w1_read(struct device *dev, enum hwmon_sensor_types type,
+		   u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_temp:
+		return w1_read_temp(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int w1_write(struct device *dev, enum hwmon_sensor_types type,
+		    u32 attr, int channel, long val)
+{
+	switch (type) {
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 #define W1_42_CHAIN	0x99
 #define W1_42_CHAIN_OFF	0x3C
 #define W1_42_CHAIN_OFF_INV	0xC3
--
2.13.2.932.g7449e964c-goog

[toc] | [prev] | [next] | [standalone]

#1686613 — Re: [linux,v2,3/3] drivers: w1: add hwmon temp support for w1_therm

On Wed, Jul 12, 2017 at 02:41:18PM -0700, Jaghathiswari Rankappagounder Natarajan wrote:
> This change adds hwmon temp support for w1_therm.
> 
> Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
> Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
> ---
> v2
> - make changes to support hwmon_device_register_with_info mentioned by Guenter.
> 
>  drivers/w1/slaves/w1_therm.c | 117 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 117 insertions(+)
> 
> --
> 2.13.2.932.g7449e964c-goog
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
> index 88f4cc8c3908..147f9dfa13de 100644
> --- a/drivers/w1/slaves/w1_therm.c
> +++ b/drivers/w1/slaves/w1_therm.c
> @@ -29,6 +29,7 @@
>  #include <linux/types.h>
>  #include <linux/slab.h>
>  #include <linux/delay.h>
> +#include <linux/hwmon.h>
> 
>  #include "../w1.h"
>  #include "../w1_int.h"
> @@ -118,19 +119,61 @@ static struct attribute *w1_ds28ea00_attrs[] = {
>  	&dev_attr_w1_seq.attr,
>  	NULL,
>  };
> +
>  ATTRIBUTE_GROUPS(w1_therm);
>  ATTRIBUTE_GROUPS(w1_ds28ea00);
> 

Do you want the following code all the time, or enclose it in
IS_REACHABLE() ?

> +static umode_t w1_is_visible(const void *_data,
> +			     enum hwmon_sensor_types type,
> +			     u32 attr, int channel);
> +
> +static int w1_read_temp(struct device *dev, u32 attr, int channel,
> +			long *val);
> +
> +static int w1_read(struct device *dev, enum hwmon_sensor_types type,
> +		   u32 attr, int channel, long *val);
> +
> +static int w1_write(struct device *dev, enum hwmon_sensor_types type,
> +		    u32 attr, int channel, long val);
> +

Any chance you can rearrange the code to not require forward declarations ?

> +static const u32 w1_temp_config[] = {
> +	HWMON_T_INPUT,
> +	0
> +};
> +
> +static const struct hwmon_channel_info w1_temp = {
> +	.type = hwmon_temp,
> +	.config = w1_temp_config,
> +};
> +
> +static const struct hwmon_channel_info *w1_info[] = {
> +	&w1_temp,
> +	NULL
> +};
> +
> +static const struct hwmon_ops w1_hwmon_ops = {
> +	.is_visible = w1_is_visible,
> +	.read = w1_read,
> +	.write = w1_write,
> +};
> +
> +static const struct hwmon_chip_info w1_chip_info = {
> +	.ops = &w1_hwmon_ops,
> +	.info = w1_info,
> +};
> +
>  static struct w1_family_ops w1_therm_fops = {
>  	.add_slave	= w1_therm_add_slave,
>  	.remove_slave	= w1_therm_remove_slave,
>  	.groups		= w1_therm_groups,
> +	.chip_info	= &w1_chip_info,
>  };
> 
>  static struct w1_family_ops w1_ds28ea00_fops = {
>  	.add_slave	= w1_therm_add_slave,
>  	.remove_slave	= w1_therm_remove_slave,
>  	.groups		= w1_ds28ea00_groups,
> +	.chip_info	= &w1_chip_info,
>  };
> 
>  static struct w1_family w1_therm_family_DS18S20 = {
> @@ -559,6 +602,80 @@ static ssize_t w1_slave_show(struct device *device,
>  	return ret;
>  }
> 
> +static int w1_read_temp(struct device *device, u32 attr, int channel,
> +			long *val)
> +{
> +	struct w1_slave *sl = dev_get_drvdata(device);
> +	u8 *family_data = sl->family_data;
> +	struct therm_info info;
> +	int ret;
> +
> +	if (!sl->family_data)

Why not use family_data here ?

> +		return -ENODEV;
> +
> +	/* prevent the slave from going away in sleep */
> +	atomic_inc(THERM_REFCNT(family_data));

The original code increments the reference count with the bus mutex locked.
Similar, the original code only checks if family_data is NULL after locking
the bus mutex.

Are you sure this change in sequence doesn't cause race conditions ?
If this function can be called with family_data == NULL, what guarantees
that it isn't set to NULL after being checked but before the reference
count is increased (or, in other words, that the atomic_inc() doesn't
increment a reference counter in freed memory) ?

> +
> +	switch (attr) {
> +	case hwmon_temp_input:

indentation below is off.

> +	ret = read_therm(device, sl, &info);
> +	if (ret)
> +		goto dec_refcnt;
> +
> +	if (!info.verdict) {
> +		dev_warn(device, "Read failed CRC check\n");

I personally dislike such noise. Personal preference, of course.

> +		ret = -EIO;
> +		goto dec_refcnt;
> +	}
> +
> +	*val = w1_convert_temp(info.rom, sl->family->fid);
> +	ret = 0;
> +	break;
> +	default:
> +		ret = -EOPNOTSUPP;
> +		break;
> +	}
> +
> +dec_refcnt:
> +	atomic_dec(THERM_REFCNT(family_data));
> +	return ret;
> +}
> +
> +static umode_t w1_is_visible(const void *_data, enum hwmon_sensor_types type,
> +			     u32 attr, int channel)
> +{
> +	umode_t mode = 0444;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		break;
> +	default:
> +		mode = 0;
> +		break;
> +	}
> +	return mode;

	return attr == hwmon_temp_input ? 0444 : 0;

would be a bit simpler here.

> +}
> +
> +static int w1_read(struct device *dev, enum hwmon_sensor_types type,
> +		   u32 attr, int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		return w1_read_temp(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int w1_write(struct device *dev, enum hwmon_sensor_types type,
> +		    u32 attr, int channel, long val)

You don't need a write function, since attributes are never writeable.

> +{
> +	switch (type) {
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
>  #define W1_42_CHAIN	0x99
>  #define W1_42_CHAIN_OFF	0x3C
>  #define W1_42_CHAIN_OFF_INV	0xC3

[toc] | [prev] | [next] | [standalone]

#1686064 — [PATCH linux v2 2/3] drivers: w1: refactor w1_slave_show to make the temp reading functionality separate

Inside the w1_slave_show function refactor the code to read the temp
into a separate function.

Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
---
v2
- no changes

 drivers/w1/slaves/w1_therm.c | 82 +++++++++++++++++++++++++++-----------------
 1 file changed, 51 insertions(+), 31 deletions(-)

diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index 82611f197b0a..88f4cc8c3908 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -64,6 +64,12 @@ struct w1_therm_family_data {
 	atomic_t refcnt;
 };

+struct therm_info {
+	u8 rom[9];
+	u8 crc;
+	u8 verdict;
+};
+
 /* return the address of the refcnt in the family data */
 #define THERM_REFCNT(family_data) \
 	(&((struct w1_therm_family_data *)family_data)->refcnt)
@@ -427,33 +433,23 @@ static ssize_t w1_slave_store(struct device *device,
 	return ret ? : size;
 }

-static ssize_t w1_slave_show(struct device *device,
-	struct device_attribute *attr, char *buf)
+static ssize_t read_therm(struct device *device,
+			  struct w1_slave *sl, struct therm_info *info)
 {
-	struct w1_slave *sl = dev_to_w1_slave(device);
 	struct w1_master *dev = sl->master;
-	u8 rom[9], crc, verdict, external_power;
-	int i, ret, max_trying = 10;
-	ssize_t c = PAGE_SIZE;
-	u8 *family_data = sl->family_data;
+	u8 external_power;
+	int ret, max_trying = 10;

 	ret = mutex_lock_interruptible(&dev->bus_mutex);
 	if (ret != 0)
 		goto post_unlock;

-	if (!sl->family_data) {
-		ret = -ENODEV;
-		goto pre_unlock;
-	}
-
-	/* prevent the slave from going away in sleep */
-	atomic_inc(THERM_REFCNT(family_data));
-	memset(rom, 0, sizeof(rom));
+	memset(info->rom, 0, sizeof(info->rom));

 	while (max_trying--) {

-		verdict = 0;
-		crc = 0;
+		info->verdict = 0;
+		info->crc = 0;

 		if (!w1_reset_select_slave(sl)) {
 			int count = 0;
@@ -496,30 +492,57 @@ static ssize_t w1_slave_show(struct device *device,
 			if (!w1_reset_select_slave(sl)) {

 				w1_write_8(dev, W1_READ_SCRATCHPAD);
-				count = w1_read_block(dev, rom, 9);
+				count = w1_read_block(dev, info->rom, 9);
 				if (count != 9) {
 					dev_warn(device, "w1_read_block() "
 						"returned %u instead of 9.\n",
 						count);
 				}

-				crc = w1_calc_crc8(rom, 8);
+				info->crc = w1_calc_crc8(info->rom, 8);

-				if (rom[8] == crc)
-					verdict = 1;
+				if (info->rom[8] == info->crc)
+					info->verdict = 1;
 			}
 		}

-		if (verdict)
+		if (info->verdict)
 			break;
 	}

+pre_unlock:
+	mutex_unlock(&dev->bus_mutex);
+
+post_unlock:
+	return ret;
+}
+
+static ssize_t w1_slave_show(struct device *device,
+			     struct device_attribute *attr, char *buf)
+{
+	struct w1_slave *sl = dev_to_w1_slave(device);
+	u8 *family_data = sl->family_data;
+	struct therm_info info;
+	int ret;
+	ssize_t c = PAGE_SIZE;
+	int i;
+
+	if (!sl->family_data)
+		return -ENODEV;
+
+	/* prevent the slave from going away in sleep */
+	atomic_inc(THERM_REFCNT(family_data));
+
+	ret = read_therm(device, sl, &info);
+	if (ret)
+		goto dec_refcnt;
+
 	for (i = 0; i < 9; ++i)
-		c -= snprintf(buf + PAGE_SIZE - c, c, "%02x ", rom[i]);
+		c -= snprintf(buf + PAGE_SIZE - c, c, "%02x ", info.rom[i]);
 	c -= snprintf(buf + PAGE_SIZE - c, c, ": crc=%02x %s\n",
-		      crc, (verdict) ? "YES" : "NO");
-	if (verdict)
-		memcpy(family_data, rom, sizeof(rom));
+		      info.crc, (info.verdict) ? "YES" : "NO");
+	if (info.verdict)
+		memcpy(family_data, info.rom, sizeof(info.rom));
 	else
 		dev_warn(device, "Read failed CRC check\n");

@@ -528,13 +551,10 @@ static ssize_t w1_slave_show(struct device *device,
 			      ((u8 *)family_data)[i]);

 	c -= snprintf(buf + PAGE_SIZE - c, c, "t=%d\n",
-		w1_convert_temp(rom, sl->family->fid));
+			w1_convert_temp(info.rom, sl->family->fid));
 	ret = PAGE_SIZE - c;

-pre_unlock:
-	mutex_unlock(&dev->bus_mutex);
-
-post_unlock:
+dec_refcnt:
 	atomic_dec(THERM_REFCNT(family_data));
 	return ret;
 }
--
2.13.2.932.g7449e964c-goog

[toc] | [prev] | [next] | [standalone]

#1686631

Hi

13.07.2017, 00:41, "Jaghathiswari Rankappagounder Natarajan" <jaghu@google.com>:
> Our board has 4 DS18B20 1-wire temperature sensors. Each 1-wire bus and the
> sensor under it is already configured against the Linux 1-wire driver
> (called w1). They have a sysfs file(e.g.
> /sys/bus/w1/devices/w1_bus_master1/28-000007704f4c/w1_slave) which, when read,
> runs code to read the temperature. We'd like the temperatures to show up in
> hwmon, so that the BMC IPMI sensor plumbing can forward those to host.
>
> The changes in version 2 are mentioned in the individual patches.
>
> This patchset is based on linux mainline version v4.10.

I believe this is a resend of your previous patchet, isn't it?
Greg, if you hadn't yet, please pull it into the tree.

Thank you.

[toc] | [prev] | [next] | [standalone]

#1686632

Hi

13.07.2017, 18:36, "Evgeniy Polyakov" <zbr@ioremap.net>:
> I believe this is a resend of your previous patchet, isn't it?
> Greg, if you hadn't yet, please pull it into the tree.

Aaand it looks like we have a discussion now, so please postpone it for a while :)

[toc] | [prev] | [next] | [standalone]

#1689200

Hi Evgeniy/Greg,

Please pull in version 4 patchset into the tree.

On Thu, Jul 13, 2017 at 8:39 AM, Evgeniy Polyakov <zbr@ioremap.net> wrote:
> Hi
>
> 13.07.2017, 18:36, "Evgeniy Polyakov" <zbr@ioremap.net>:
>> I believe this is a resend of your previous patchet, isn't it?
>> Greg, if you hadn't yet, please pull it into the tree.
>
> Aaand it looks like we have a discussion now, so please postpone it for a while :)

[toc] | [prev] | [next] | [standalone]

#1689226 — Re: [PATCH linux v2 0/3] Export 1-wire thermal sensors as hwmon device

On Mon, Jul 17, 2017 at 09:06:00AM -0700, Jaghathiswari Rankappagounder Natarajan wrote:
> Hi Evgeniy/Greg,
> 
> Please pull in version 4 patchset into the tree.

I have no idea what that means :(

Please resend the correct ones so I know what to do here.

thanks,

greg k-h

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web