[PATCH v3 1/5] w1_therm: fix reset_select_slave. Creating w1_therm.h

[PATCH v3 1/5] w1_therm: fix reset_select_slave. Creating w1_therm.h

[Akira Shimahara]

Patch for enhancement of w1_therm module.
 - Creating a w1_therm.h file to clean up the code and documenting it.
 - fix reset_select_slave function: w1_reset_select_slave() from w1_io.c
	could not be used here because a SKIP ROM command is sent if only
	one device is on the line. At the beginning of the search process,
	sl->master->slave_count is 1 even if more devices are on the line,
	causing data collision in the bus.

Signed-off-by: Akira Shimahara <akira215corp@gmail.com>
---
 drivers/w1/slaves/w1_therm.c | 325 ++++++++++++++++-------------------
 drivers/w1/slaves/w1_therm.h | 151 ++++++++++++++++
 2 files changed, 301 insertions(+), 175 deletions(-)
 create mode 100644 drivers/w1/slaves/w1_therm.h

diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index 18f08d7..6245950 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -5,19 +5,15 @@
  * Copyright (c) 2004 Evgeniy Polyakov <zbr@ioremap.net>
  */
 
-#include <asm/types.h>
-
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/sched.h>
-#include <linux/device.h>
-#include <linux/types.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/hwmon.h>
 
-#include <linux/w1.h>
+#include "w1_therm.h"
 
 #define W1_THERM_DS18S20	0x10
 #define W1_THERM_DS1822		0x22
@@ -41,55 +37,6 @@
 static int w1_strong_pullup = 1;
 module_param_named(strong_pullup, w1_strong_pullup, int, 0);
 
-struct w1_therm_family_data {
-	uint8_t rom[9];
-	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)
-
-static int w1_therm_add_slave(struct w1_slave *sl)
-{
-	sl->family_data = kzalloc(sizeof(struct w1_therm_family_data),
-		GFP_KERNEL);
-	if (!sl->family_data)
-		return -ENOMEM;
-	atomic_set(THERM_REFCNT(sl->family_data), 1);
-	return 0;
-}
-
-static void w1_therm_remove_slave(struct w1_slave *sl)
-{
-	int refcnt = atomic_sub_return(1, THERM_REFCNT(sl->family_data));
-
-	while (refcnt) {
-		msleep(1000);
-		refcnt = atomic_read(THERM_REFCNT(sl->family_data));
-	}
-	kfree(sl->family_data);
-	sl->family_data = NULL;
-}
-
-static ssize_t w1_slave_show(struct device *device,
-	struct device_attribute *attr, char *buf);
-
-static ssize_t w1_slave_store(struct device *device,
-	struct device_attribute *attr, const char *buf, size_t size);
-
-static ssize_t w1_seq_show(struct device *device,
-	struct device_attribute *attr, char *buf);
-
-static DEVICE_ATTR_RW(w1_slave);
-static DEVICE_ATTR_RO(w1_seq);
-
 static struct attribute *w1_therm_attrs[] = {
 	&dev_attr_w1_slave.attr,
 	NULL,
@@ -101,6 +48,7 @@ static struct attribute *w1_ds28ea00_attrs[] = {
 	NULL,
 };
 
+/*------------------------------attribute groups----------------------------*/
 ATTRIBUTE_GROUPS(w1_therm);
 ATTRIBUTE_GROUPS(w1_ds28ea00);
 
@@ -154,6 +102,7 @@ static const struct hwmon_chip_info w1_chip_info = {
 #define W1_CHIPINFO	NULL
 #endif
 
+/*------------------------------family operations---------------------------*/
 static struct w1_family_ops w1_therm_fops = {
 	.add_slave	= w1_therm_add_slave,
 	.remove_slave	= w1_therm_remove_slave,
@@ -168,6 +117,7 @@ static struct w1_family_ops w1_ds28ea00_fops = {
 	.chip_info	= W1_CHIPINFO,
 };
 
+/*--------------------family binding on operations struct-------------------*/
 static struct w1_family w1_therm_family_DS18S20 = {
 	.fid = W1_THERM_DS18S20,
 	.fops = &w1_therm_fops,
@@ -193,26 +143,7 @@ static struct w1_family w1_therm_family_DS1825 = {
 	.fops = &w1_therm_fops,
 };
 
-struct w1_therm_family_converter {
-	u8			broken;
-	u16			reserved;
-	struct w1_family	*f;
-	int			(*convert)(u8 rom[9]);
-	int			(*precision)(struct device *device, int val);
-	int			(*eeprom)(struct device *device);
-};
-
-/* write configuration to eeprom */
-static inline int w1_therm_eeprom(struct device *device);
-
-/* Set precision for conversion */
-static inline int w1_DS18B20_precision(struct device *device, int val);
-static inline int w1_DS18S20_precision(struct device *device, int val);
-
-/* The return value is millidegrees Centigrade. */
-static inline int w1_DS18B20_convert_temp(u8 rom[9]);
-static inline int w1_DS18S20_convert_temp(u8 rom[9]);
-
+/*----------------------Device capability description-----------------------*/
 static struct w1_therm_family_converter w1_therm_families[] = {
 	{
 		.f		= &w1_therm_family_DS18S20,
@@ -246,78 +177,7 @@ static struct w1_therm_family_converter w1_therm_families[] = {
 	}
 };
 
-static inline int w1_therm_eeprom(struct device *device)
-{
-	struct w1_slave *sl = dev_to_w1_slave(device);
-	struct w1_master *dev = sl->master;
-	u8 rom[9], external_power;
-	int ret, max_trying = 10;
-	u8 *family_data = sl->family_data;
-
-	if (!sl->family_data) {
-		ret = -ENODEV;
-		goto error;
-	}
-
-	/* prevent the slave from going away in sleep */
-	atomic_inc(THERM_REFCNT(family_data));
-
-	ret = mutex_lock_interruptible(&dev->bus_mutex);
-	if (ret != 0)
-		goto dec_refcnt;
-
-	memset(rom, 0, sizeof(rom));
-
-	while (max_trying--) {
-		if (!w1_reset_select_slave(sl)) {
-			unsigned int tm = 10;
-			unsigned long sleep_rem;
-
-			/* check if in parasite mode */
-			w1_write_8(dev, W1_READ_PSUPPLY);
-			external_power = w1_read_8(dev);
-
-			if (w1_reset_select_slave(sl))
-				continue;
-
-			/* 10ms strong pullup/delay after the copy command */
-			if (w1_strong_pullup == 2 ||
-			    (!external_power && w1_strong_pullup))
-				w1_next_pullup(dev, tm);
-
-			w1_write_8(dev, W1_COPY_SCRATCHPAD);
-
-			if (external_power) {
-				mutex_unlock(&dev->bus_mutex);
-
-				sleep_rem = msleep_interruptible(tm);
-				if (sleep_rem != 0) {
-					ret = -EINTR;
-					goto dec_refcnt;
-				}
-
-				ret = mutex_lock_interruptible(&dev->bus_mutex);
-				if (ret != 0)
-					goto dec_refcnt;
-			} else if (!w1_strong_pullup) {
-				sleep_rem = msleep_interruptible(tm);
-				if (sleep_rem != 0) {
-					ret = -EINTR;
-					goto mt_unlock;
-				}
-			}
-
-			break;
-		}
-	}
-
-mt_unlock:
-	mutex_unlock(&dev->bus_mutex);
-dec_refcnt:
-	atomic_dec(THERM_REFCNT(family_data));
-error:
-	return ret;
-}
+/*------------------------ Device dependent func---------------------------*/
 
 /* DS18S20 does not feature configuration register */
 static inline int w1_DS18S20_precision(struct device *device, int val)
@@ -375,7 +235,7 @@ static inline int w1_DS18B20_precision(struct device *device, int val)
 	while (max_trying--) {
 		crc = 0;
 
-		if (!w1_reset_select_slave(sl)) {
+		if (!reset_select_slave(sl)) {
 			int count = 0;
 
 			/* read values to only alter precision bits */
@@ -388,7 +248,7 @@ static inline int w1_DS18B20_precision(struct device *device, int val)
 			if (rom[8] == crc) {
 				rom[4] = (rom[4] & ~mask) | (precision_bits & mask);
 
-				if (!w1_reset_select_slave(sl)) {
+				if (!reset_select_slave(sl)) {
 					w1_write_8(dev, W1_WRITE_SCRATCHPAD);
 					w1_write_8(dev, rom[2]);
 					w1_write_8(dev, rom[3]);
@@ -434,40 +294,55 @@ static inline int w1_DS18S20_convert_temp(u8 rom[9])
 	return t;
 }
 
-static inline int w1_convert_temp(u8 rom[9], u8 fid)
+/*-------------------------Interface Functions------------------------------*/
+static int w1_therm_add_slave(struct w1_slave *sl)
 {
-	int i;
+	sl->family_data = kzalloc(sizeof(struct w1_therm_family_data),
+		GFP_KERNEL);
+	if (!sl->family_data)
+		return -ENOMEM;
+	atomic_set(THERM_REFCNT(sl->family_data), 1);
+	return 0;
+}
 
-	for (i = 0; i < ARRAY_SIZE(w1_therm_families); ++i)
-		if (w1_therm_families[i].f->fid == fid)
-			return w1_therm_families[i].convert(rom);
+static void w1_therm_remove_slave(struct w1_slave *sl)
+{
+	int refcnt = atomic_sub_return(1, THERM_REFCNT(sl->family_data));
+
+	while (refcnt) {
+		msleep(1000);
+		refcnt = atomic_read(THERM_REFCNT(sl->family_data));
+	}
+	kfree(sl->family_data);
+	sl->family_data = NULL;
+}
+
+/*------------------------Hardware Functions--------------------------*/
+
+/* Safe version of reser_select_slave - avoid using the one in w_io.c */
+static int reset_select_slave(struct w1_slave *sl)
+{
+	u8 match[9] = { W1_MATCH_ROM, };
+	u64 rn = le64_to_cpu(*((u64 *)&sl->reg_num));
+
+	if (w1_reset_bus(sl->master))
+		return -ENODEV;
+
+	memcpy(&match[1], &rn, 8);
+	w1_write_block(sl->master, match, 9);
 
 	return 0;
 }
 
-static ssize_t w1_slave_store(struct device *device,
-			      struct device_attribute *attr, const char *buf,
-			      size_t size)
+static inline int w1_convert_temp(u8 rom[9], u8 fid)
 {
-	int val, ret;
-	struct w1_slave *sl = dev_to_w1_slave(device);
 	int i;
 
-	ret = kstrtoint(buf, 0, &val);
-	if (ret)
-		return ret;
+	for (i = 0; i < ARRAY_SIZE(w1_therm_families); ++i)
+		if (w1_therm_families[i].f->fid == fid)
+			return w1_therm_families[i].convert(rom);
 
-	for (i = 0; i < ARRAY_SIZE(w1_therm_families); ++i) {
-		if (w1_therm_families[i].f->fid == sl->family->fid) {
-			/* zero value indicates to write current configuration to eeprom */
-			if (val == 0)
-				ret = w1_therm_families[i].eeprom(device);
-			else
-				ret = w1_therm_families[i].precision(device, val);
-			break;
-		}
-	}
-	return ret ? : size;
+	return 0;
 }
 
 static ssize_t read_therm(struct device *device,
@@ -497,7 +372,7 @@ static ssize_t read_therm(struct device *device,
 		info->verdict = 0;
 		info->crc = 0;
 
-		if (!w1_reset_select_slave(sl)) {
+		if (!reset_select_slave(sl)) {
 			int count = 0;
 			unsigned int tm = 750;
 			unsigned long sleep_rem;
@@ -505,7 +380,7 @@ static ssize_t read_therm(struct device *device,
 			w1_write_8(dev, W1_READ_PSUPPLY);
 			external_power = w1_read_8(dev);
 
-			if (w1_reset_select_slave(sl))
+			if (reset_select_slave(sl))
 				continue;
 
 			/* 750ms strong pullup (or delay) after the convert */
@@ -535,7 +410,7 @@ static ssize_t read_therm(struct device *device,
 				}
 			}
 
-			if (!w1_reset_select_slave(sl)) {
+			if (!reset_select_slave(sl)) {
 
 				w1_write_8(dev, W1_READ_SCRATCHPAD);
 				count = w1_read_block(dev, info->rom, 9);
@@ -564,6 +439,81 @@ error:
 	return ret;
 }
 
+static inline int w1_therm_eeprom(struct device *device)
+{
+	struct w1_slave *sl = dev_to_w1_slave(device);
+	struct w1_master *dev = sl->master;
+	u8 rom[9], external_power;
+	int ret, max_trying = 10;
+	u8 *family_data = sl->family_data;
+
+	if (!sl->family_data) {
+		ret = -ENODEV;
+		goto error;
+	}
+
+	/* prevent the slave from going away in sleep */
+	atomic_inc(THERM_REFCNT(family_data));
+
+	ret = mutex_lock_interruptible(&dev->bus_mutex);
+	if (ret != 0)
+		goto dec_refcnt;
+
+	memset(rom, 0, sizeof(rom));
+
+	while (max_trying--) {
+		if (!reset_select_slave(sl)) {
+			unsigned int tm = 10;
+			unsigned long sleep_rem;
+
+			/* check if in parasite mode */
+			w1_write_8(dev, W1_READ_PSUPPLY);
+			external_power = w1_read_8(dev);
+
+			if (reset_select_slave(sl))
+				continue;
+
+			/* 10ms strong pullup/delay after the copy command */
+			if (w1_strong_pullup == 2 ||
+			    (!external_power && w1_strong_pullup))
+				w1_next_pullup(dev, tm);
+
+			w1_write_8(dev, W1_COPY_SCRATCHPAD);
+
+			if (external_power) {
+				mutex_unlock(&dev->bus_mutex);
+
+				sleep_rem = msleep_interruptible(tm);
+				if (sleep_rem != 0) {
+					ret = -EINTR;
+					goto dec_refcnt;
+				}
+
+				ret = mutex_lock_interruptible(&dev->bus_mutex);
+				if (ret != 0)
+					goto dec_refcnt;
+			} else if (!w1_strong_pullup) {
+				sleep_rem = msleep_interruptible(tm);
+				if (sleep_rem != 0) {
+					ret = -EINTR;
+					goto mt_unlock;
+				}
+			}
+
+			break;
+		}
+	}
+
+mt_unlock:
+	mutex_unlock(&dev->bus_mutex);
+dec_refcnt:
+	atomic_dec(THERM_REFCNT(family_data));
+error:
+	return ret;
+}
+
+/*------------------------Interface sysfs--------------------------*/
+
 static ssize_t w1_slave_show(struct device *device,
 			     struct device_attribute *attr, char *buf)
 {
@@ -597,6 +547,31 @@ static ssize_t w1_slave_show(struct device *device,
 	return ret;
 }
 
+static ssize_t w1_slave_store(struct device *device,
+			      struct device_attribute *attr, const char *buf,
+			      size_t size)
+{
+	struct w1_slave *sl = dev_to_w1_slave(device);
+	int val, ret, i;
+
+	ret = kstrtoint(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ARRAY_SIZE(w1_therm_families); ++i) {
+		if (w1_therm_families[i].f->fid == sl->family->fid) {
+	/* zero value indicates to write current configuration to eeprom */
+			if (val == 0)
+				ret = w1_therm_families[i].eeprom(device);
+			else
+				ret = w1_therm_families[i].precision(device,
+								val);
+			break;
+		}
+	}
+	return ret ? : size;
+}
+
 #if IS_REACHABLE(CONFIG_HWMON)
 static int w1_read_temp(struct device *device, u32 attr, int channel,
 			long *val)
diff --git a/drivers/w1/slaves/w1_therm.h b/drivers/w1/slaves/w1_therm.h
new file mode 100644
index 0000000..b73af0b
--- /dev/null
+++ b/drivers/w1/slaves/w1_therm.h
@@ -0,0 +1,151 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ *	w1_therm.h
+ *
+ * Written by Akira Shimahara
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __W1_THERM_H
+#define __W1_THERM_H
+
+#include <asm/types.h>
+
+#include <linux/device.h>
+#include <linux/types.h>
+#include <linux/mutex.h>
+#include <linux/w1.h>
+
+/*----------------------------------Structs---------------------------------*/
+
+/**
+ * struct w1_therm_family_converter
+ * @brief Used to bind standard function call
+ * to device specific function
+ * it could be routed to NULL if device don't support feature
+ * see helper : device_family()
+ */
+struct w1_therm_family_converter {
+	u8			broken;
+	u16			reserved;
+	struct w1_family	*f;
+	int			(*convert)(u8 rom[9]);
+	int			(*precision)(struct device *device, int val);
+	int			(*eeprom)(struct device *device);
+};
+
+/**
+ * struct w1_therm_family_data
+ * @param rom data
+ * @param refcnt ref count
+ */
+struct w1_therm_family_data {
+	uint8_t rom[9];
+	atomic_t refcnt;
+};
+
+/**
+ * struct therm_info
+ * @brief Only used to store temperature reading
+ * @param rom RAM device data
+ * @param crc computed crc from rom
+ * @param verdict 1 crc checked, 0 crc not matching
+ */
+struct therm_info {
+	u8 rom[9];
+	u8 crc;
+	u8 verdict;
+};
+
+/*-----------------------Device specific functions-------------------------*/
+
+/* write configuration to eeprom */
+static inline int w1_therm_eeprom(struct device *device);
+
+/* Set precision for conversion */
+static inline int w1_DS18B20_precision(struct device *device, int val);
+static inline int w1_DS18S20_precision(struct device *device, int val);
+
+/* The return value is millidegrees Centigrade. */
+static inline int w1_DS18B20_convert_temp(u8 rom[9]);
+static inline int w1_DS18S20_convert_temp(u8 rom[9]);
+
+/*-------------------------------Macros--------------------------------------*/
+
+/* return the address of the refcnt in the family data */
+#define THERM_REFCNT(family_data) \
+	(&((struct w1_therm_family_data *)family_data)->refcnt)
+
+/*---------------------------Hardware Functions-----------------------------*/
+
+/**
+ * reset_select_slave() - reset and select a slave
+ * @brief Resets the bus and select the slave by sending either a ROM MATCH
+ * w1_reset_select_slave() from w1_io.c could not be used
+ * here because a SKIP ROM command is sent if only one device is on the line.
+ * At the beginning of the such process, sl->master->slave_count is 1 even if
+ * more devices are on the line, causing collision on the line.
+ * The w1 master lock must be held.
+ * @param sl the slave to select
+ * @return 0 if success, negative kernel error code otherwise
+ */
+static int reset_select_slave(struct w1_slave *sl);
+
+/** read_therm()
+ * @param sl pointer to the slave to read
+ * @param info pointer to a structure to store the read results
+ * @return 0 if success, -kernel error code otherwise
+ */
+static ssize_t read_therm(struct device *device,
+			struct w1_slave *sl, struct therm_info *info);
+
+/*----------------------------Interface sysfs-------------------------------*/
+
+/** @brief A callback function to output the temperature Old way
+ *  read temperature and return the result in the sys file
+ *  This has been kept for compatibility
+ */
+static ssize_t w1_slave_show(struct device *device,
+	struct device_attribute *attr, char *buf);
+
+/** @brief A callback function to set the resolution Old way
+ *  This has been kept for compatibility
+ *  @param 0, it write config in the EEPROM
+ *  @param 9..12, it set the resolution in the RAM
+ */
+static ssize_t w1_slave_store(struct device *device,
+	struct device_attribute *attr, const char *buf, size_t size);
+
+static ssize_t w1_seq_show(struct device *device,
+	struct device_attribute *attr, char *buf);
+
+/*-----------------------------Attributes declarations----------------------*/
+
+static DEVICE_ATTR_RW(w1_slave);
+static DEVICE_ATTR_RO(w1_seq);
+
+/*--------------------------Interface Functions-----------------------------*/
+
+/** w1_therm_add_slave() - Called each time a search discover a new device
+ * @brief used to initialized slave (family datas)
+ * @param sl slave just discovered
+ * @return 0 - If success, negative kernel code otherwise
+ */
+static int w1_therm_add_slave(struct w1_slave *sl);
+
+/** w1_therm_remove_slave() - Called each time a slave is removed
+ * @brief used to free memory
+ * @param sl slave to be removed
+ */
+static void w1_therm_remove_slave(struct w1_slave *sl);
+
+#endif  /* __W1_THERM_H */
\ No newline at end of file
-- 
2.25.4

Re: [PATCH v3 1/5] w1_therm: fix reset_select_slave. Creating w1_therm.h

[Greg KH]

On Wed, Apr 29, 2020 at 03:30:48PM +0200, Akira Shimahara wrote:
> Patch for enhancement of w1_therm module.
>  - Creating a w1_therm.h file to clean up the code and documenting it.
>  - fix reset_select_slave function: w1_reset_select_slave() from w1_io.c
> 	could not be used here because a SKIP ROM command is sent if only
> 	one device is on the line. At the beginning of the search process,
> 	sl->master->slave_count is 1 even if more devices are on the line,
> 	causing data collision in the bus.

This should be three different patches, one to create the file by moving
the code, one to document it, and the last to do the fix.

thanks,

greg k-h

Re: [PATCH v3 1/5] w1_therm: fix reset_select_slave. Creating w1_therm.h

[Akira shimahara]

Le mercredi 29 avril 2020 à 15:43 +0200, Greg KH a écrit :
> On Wed, Apr 29, 2020 at 03:30:48PM +0200, Akira Shimahara wrote:
> > Patch for enhancement of w1_therm module.
> >  - Creating a w1_therm.h file to clean up the code and documenting
> > it.
> >  - fix reset_select_slave function: w1_reset_select_slave() from
> > w1_io.c
> > 	could not be used here because a SKIP ROM command is sent if
> > only
> > 	one device is on the line. At the beginning of the search
> > process,
> > 	sl->master->slave_count is 1 even if more devices are on the
> > line,
> > 	causing data collision in the bus.
> 
> This should be three different patches, one to create the file by
> moving
> the code, one to document it, and the last to do the fix.
> 
> thanks,
> 
> greg k-h

Ok, I will do

Akira Shimahara