All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 5/9] w1_therm: adding resolution sysfs entry
@ 2020-05-10 14:17 Akira Shimahara
  2020-05-11  3:25 ` Randy Dunlap
  0 siblings, 1 reply; 4+ messages in thread
From: Akira Shimahara @ 2020-05-10 14:17 UTC (permalink / raw)
  To: greg, rdunlap; +Cc: zbr, linux-kernel, Akira Shimahara

Adding resolution sysfs entry (RW) to get or set the device resolution
Write values are managed as follow:
	* '9..12': resolution to set in bit
	* Anything else: do nothing
Read values are :
	* '9..12': device resolution in bit
	* '-xx': xx is kernel error when reading the resolution

Only supported devices will show the sysfs entry. A new family has been
created for DS18S20 devices as they do not implement resolution feature.

The resolution of each device is check when the device is
discover by the bus master, in 'w1_therm_add_slave(struct w1_slave *)'.
The status is stored in the device structure w1_therm_family_data so
that the driver always knows the resolution of each device, which could
be used later to determine the required conversion duration (resolution
dependent).

The resolution is re evaluate each time a user read or write the sysfs
entry.

To avoid looping through the w1_therm_families at run time, the pointer
'specific_functions' is set up to the correct 'w1_therm_family_converter'
when the slave is added (which mean when it is discovered by the master).
This initialization is done by a helper function 
'device_family(struct w1_slave *sl)', and a dedicated macro 
'SLAVE_SPECIFIC_FUNC(sl)' allow the access to the specific function of the
slave device.

'read_scratchpad' and 'write_scratchpad' are the hardware functions to
access the device RAM, as per protocol specification.

It cancel the former 'precision' functions, which was only set and never
read (so not stored in the device struct).

Updating Documentation/ABI/testing/sysfs-driver-w1_therm accordingly.

Signed-off-by: Akira Shimahara <akira215corp@gmail.com>
---
Changes in v5:
- All patch serie in one .c file
- Correcting some comments
- adding <linux/string.h> include
Changes in v6:
- Formatting code comments according to kernel-doc requirements

 .../ABI/testing/sysfs-driver-w1_therm         |  17 +
 drivers/w1/slaves/w1_therm.c                  | 438 ++++++++++++++----
 2 files changed, 357 insertions(+), 98 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-w1_therm b/Documentation/ABI/testing/sysfs-driver-w1_therm
index 99d73ee..7ed95e9 100644
--- a/Documentation/ABI/testing/sysfs-driver-w1_therm
+++ b/Documentation/ABI/testing/sysfs-driver-w1_therm
@@ -10,6 +10,23 @@ Users:		any user space application which wants to communicate with
 		w1_term device
 
 
+What:		/sys/bus/w1/devices/.../resolution
+Date:		May 2020
+Contact:	Akira Shimahara <akira215corp@gmail.com>
+Description:
+		(RW) get or set the device resolution (on supported devices,
+		if not, this entry is not present). Note that the resolution
+		will be changed only in device RAM, so it will be cleared when
+		power is lost. Trigger a 'save' to EEPROM command to keep
+		values after power-on. Read or write are :
+			* '9..12': device resolution in bit
+			or resolution to set in bit
+			* '-xx': xx is kernel error when reading the resolution
+			* Anything else: do nothing
+Users:		any user space application which wants to communicate with
+		w1_term device
+
+
 What:		/sys/bus/w1/devices/.../w1_slave
 Date:		May 2020
 Contact:	Akira Shimahara <akira215corp@gmail.com>
diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index 08579dc..b1734ae 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -50,12 +50,24 @@ module_param_named(strong_pullup, w1_strong_pullup, int, 0);
 
 /* Helpers Macros */
 
+/* return a pointer on the slave w1_therm_family_converter struct:
+ * always test family data existence before
+ */
+#define SLAVE_SPECIFIC_FUNC(sl) \
+	(((struct w1_therm_family_data *)(sl->family_data))->specific_functions)
+
 /* return the power mode of the sl slave : 1-ext, 0-parasite, <0 unknown
  * always test family data existence before
  */
 #define SLAVE_POWERMODE(sl) \
 	(((struct w1_therm_family_data *)(sl->family_data))->external_powered)
 
+/* return the resolution in bit of the sl slave : <0 unknown
+ *	always test family data existence before
+ */
+#define SLAVE_RESOLUTION(sl) \
+	(((struct w1_therm_family_data *)(sl->family_data))->resolution)
+
 /* return the address of the refcnt in the family data */
 #define THERM_REFCNT(family_data) \
 	(&((struct w1_therm_family_data *)family_data)->refcnt)
@@ -68,7 +80,8 @@ module_param_named(strong_pullup, w1_strong_pullup, int, 0);
  * @reserved: not used here
  * @f: pointer to the device binding structure
  * @convert: pointer to the device conversion function
- * @precision: pointer to the device precicion function
+ * @set_resolution: pointer to the device set_resolution function
+ * @get_resolution: pointer to the device get_resolution function
  * @eeprom: pointer to eeprom function
  */
 struct w1_therm_family_converter {
@@ -76,7 +89,8 @@ struct w1_therm_family_converter {
 	u16		reserved;
 	struct w1_family	*f;
 	int		(*convert)(u8 rom[9]);
-	int		(*precision)(struct device *device, int val);
+	int		(*set_resolution)(struct w1_slave *sl, int val);
+	int		(*get_resolution)(struct w1_slave *sl);
 	int		(*eeprom)(struct device *device);
 };
 
@@ -87,11 +101,15 @@ struct w1_therm_family_converter {
  * @external_powered:	1 device powered externally,
  *				0 device parasite powered,
  *				-x error or undefined
+ * @resolution: current device resolution
+ * @specific_functions: pointer to struct of device specific function
  */
 struct w1_therm_family_data {
 	uint8_t rom[9];
 	atomic_t refcnt;
 	int external_powered;
+	int resolution;
+	struct w1_therm_family_converter *specific_functions;
 };
 
 /**
@@ -124,6 +142,25 @@ struct therm_info {
  */
 static int reset_select_slave(struct w1_slave *sl);
 
+/**
+ * read_scratchpad() - read the data in device RAM
+ * @sl: pointer to the slave to read
+ * @info: pointer to a structure to store the read results
+ *
+ * Return: 0 if success, -kernel error code otherwise
+ */
+static int read_scratchpad(struct w1_slave *sl, struct therm_info *info);
+
+/**
+ * write_scratchpad() - write nb_bytes in the device RAM
+ * @sl: pointer to the slave to write in
+ * @data: pointer to an array of 3 bytes, as 3 bytes MUST be written
+ * @nb_bytes: Nb bytes to be written (2 for DS18S20, 3 for other devices)
+ *
+ * Return: 0 if success, -kernel error code otherwise
+ */
+static int write_scratchpad(struct w1_slave *sl, const u8 *data, u8 nb_bytes);
+
 /**
  * read_powermode() - Query the power mode of the slave
  * @sl: slave to retrieve the power mode
@@ -152,11 +189,18 @@ static ssize_t w1_seq_show(struct device *device,
 static ssize_t ext_power_show(struct device *device,
 	struct device_attribute *attr, char *buf);
 
+static ssize_t resolution_show(struct device *device,
+	struct device_attribute *attr, char *buf);
+
+static ssize_t resolution_store(struct device *device,
+	struct device_attribute *attr, const char *buf, size_t size);
+
 /* Attributes declarations */
 
 static DEVICE_ATTR_RW(w1_slave);
 static DEVICE_ATTR_RO(w1_seq);
 static DEVICE_ATTR_RO(ext_power);
+static DEVICE_ATTR_RW(resolution);
 
 /* Interface Functions declaration */
 
@@ -183,6 +227,13 @@ static void w1_therm_remove_slave(struct w1_slave *sl);
 /* Family attributes */
 
 static struct attribute *w1_therm_attrs[] = {
+	&dev_attr_w1_slave.attr,
+	&dev_attr_ext_power.attr,
+	&dev_attr_resolution.attr,
+	NULL,
+};
+
+static struct attribute *w1_ds18s20_attrs[] = {
 	&dev_attr_w1_slave.attr,
 	&dev_attr_ext_power.attr,
 	NULL,
@@ -192,12 +243,14 @@ static struct attribute *w1_ds28ea00_attrs[] = {
 	&dev_attr_w1_slave.attr,
 	&dev_attr_w1_seq.attr,
 	&dev_attr_ext_power.attr,
+	&dev_attr_resolution.attr,
 	NULL,
 };
 
 /* Attribute groups */
 
 ATTRIBUTE_GROUPS(w1_therm);
+ATTRIBUTE_GROUPS(w1_ds18s20);
 ATTRIBUTE_GROUPS(w1_ds28ea00);
 
 #if IS_REACHABLE(CONFIG_HWMON)
@@ -259,6 +312,13 @@ static struct w1_family_ops w1_therm_fops = {
 	.chip_info	= W1_CHIPINFO,
 };
 
+static struct w1_family_ops w1_ds18s20_fops = {
+	.add_slave	= w1_therm_add_slave,
+	.remove_slave	= w1_therm_remove_slave,
+	.groups		= w1_ds18s20_groups,
+	.chip_info	= W1_CHIPINFO,
+};
+
 static struct w1_family_ops w1_ds28ea00_fops = {
 	.add_slave	= w1_therm_add_slave,
 	.remove_slave	= w1_therm_remove_slave,
@@ -270,7 +330,7 @@ static struct w1_family_ops w1_ds28ea00_fops = {
 
 static struct w1_family w1_therm_family_DS18S20 = {
 	.fid = W1_THERM_DS18S20,
-	.fops = &w1_therm_fops,
+	.fops = &w1_ds18s20_fops,
 };
 
 static struct w1_family w1_therm_family_DS18B20 = {
@@ -298,92 +358,66 @@ static struct w1_family w1_therm_family_DS1825 = {
 /* write configuration to eeprom */
 static inline int w1_therm_eeprom(struct device *device);
 
-/* DS18S20 does not feature configuration register */
-static inline int w1_DS18S20_precision(struct device *device, int val)
+static inline int w1_DS18B20_write_data(struct w1_slave *sl,
+				const u8 *data)
 {
-	return 0;
+	return write_scratchpad(sl, data, 3);
 }
 
-/* Set precision for conversion */
-static inline int w1_DS18B20_precision(struct device *device, int val)
+static inline int w1_DS18S20_write_data(struct w1_slave *sl,
+				const u8 *data)
 {
-	struct w1_slave *sl = dev_to_w1_slave(device);
-	struct w1_master *dev = sl->master;
-	u8 rom[9], crc;
-	int ret, max_trying = 10;
-	u8 *family_data = sl->family_data;
-	uint8_t precision_bits;
-	uint8_t mask = 0x60;
-
-	if (val > 12 || val < 9) {
-		pr_warn("Unsupported precision\n");
-		ret = -EINVAL;
-		goto error;
-	}
-
-	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));
+	/* No config register */
+	return write_scratchpad(sl, data, 2);
+}
 
-	/* translate precision to bitmask (see datasheet page 9) */
-	switch (val) {
-	case 9:
-		precision_bits = 0x00;
-		break;
-	case 10:
-		precision_bits = 0x20;
-		break;
-	case 11:
-		precision_bits = 0x40;
-		break;
-	case 12:
-	default:
-		precision_bits = 0x60;
-		break;
-	}
+static inline int w1_DS18B20_set_resolution(struct w1_slave *sl, int val)
+{
+	int ret = -ENODEV;
+	u8 new_config_register[3];	/* array of data to be written */
+	struct therm_info info;
 
-	while (max_trying--) {
-		crc = 0;
+	/* resolution of DS18B20 is in the range [9..12] bits */
+	if (val < 9 || val > 12)
+		return -EINVAL;
+
+	val -= 9; /* soustract 9 the lowest resolution in bit */
+	val = (val << 5); /* shift to position bit 5 & bit 6 */
+
+	/* Read the scratchpad to change only the required bits
+	 * (bit5 & bit 6 from byte 4)
+	 */
+	ret = read_scratchpad(sl, &info);
+	if (!ret) {
+		new_config_register[0] = info.rom[2];
+		new_config_register[1] = info.rom[3];
+		/* config register is byte 4 & mask 0b10011111*/
+		new_config_register[2] = (info.rom[4] & 0x9F) |
+					(u8) val;
+	} else
+		return ret;
 
-		if (!reset_select_slave(sl)) {
-			int count = 0;
+	/* Write data in the device RAM */
+	ret = w1_DS18B20_write_data(sl, new_config_register);
 
-			/* read values to only alter precision bits */
-			w1_write_8(dev, W1_READ_SCRATCHPAD);
-			count = w1_read_block(dev, rom, 9);
-			if (count != 9)
-				dev_warn(device, "w1_read_block() returned %u instead of 9.\n",	count);
+	return ret;
+}
 
-			crc = w1_calc_crc8(rom, 8);
-			if (rom[8] == crc) {
-				rom[4] = (rom[4] & ~mask) | (precision_bits & mask);
+static inline int w1_DS18B20_get_resolution(struct w1_slave *sl)
+{
+	int ret = -ENODEV;
+	u8 config_register;
+	struct therm_info info;
 
-				if (!reset_select_slave(sl)) {
-					w1_write_8(dev, W1_WRITE_SCRATCHPAD);
-					w1_write_8(dev, rom[2]);
-					w1_write_8(dev, rom[3]);
-					w1_write_8(dev, rom[4]);
+	ret = read_scratchpad(sl, &info);
 
-					break;
-				}
-			}
-		}
+	if (!ret)	{
+		config_register = info.rom[4]; // config register is byte 4
+		config_register &= 0x60; // 0b01100000 keep only bit 5 & 6
+		config_register = (config_register >> 5);	// shift
+		config_register += 9; // add 9 the lowest resolution in bit
+		ret = (int) config_register;
 	}
-
-	mutex_unlock(&dev->bus_mutex);
-dec_refcnt:
-	atomic_dec(THERM_REFCNT(family_data));
-error:
 	return ret;
 }
 
@@ -422,37 +456,62 @@ static struct w1_therm_family_converter w1_therm_families[] = {
 	{
 		.f		= &w1_therm_family_DS18S20,
 		.convert	= w1_DS18S20_convert_temp,
-		.precision	= w1_DS18S20_precision,
+		.set_resolution	= NULL,	// no config register
+		.get_resolution	= NULL,	// no config register
 		.eeprom		= w1_therm_eeprom
 	},
 	{
 		.f		= &w1_therm_family_DS1822,
 		.convert	= w1_DS18B20_convert_temp,
-		.precision	= w1_DS18S20_precision,
+		.set_resolution	= w1_DS18B20_set_resolution,
+		.get_resolution	= w1_DS18B20_get_resolution,
 		.eeprom		= w1_therm_eeprom
 	},
 	{
 		.f		= &w1_therm_family_DS18B20,
 		.convert	= w1_DS18B20_convert_temp,
-		.precision	= w1_DS18B20_precision,
+		.set_resolution	= w1_DS18B20_set_resolution,
+		.get_resolution	= w1_DS18B20_get_resolution,
 		.eeprom		= w1_therm_eeprom
 	},
 	{
 		.f		= &w1_therm_family_DS28EA00,
 		.convert	= w1_DS18B20_convert_temp,
-		.precision	= w1_DS18S20_precision,
+		.set_resolution	= w1_DS18B20_set_resolution,
+		.get_resolution	= w1_DS18B20_get_resolution,
 		.eeprom		= w1_therm_eeprom
 	},
 	{
 		.f		= &w1_therm_family_DS1825,
 		.convert	= w1_DS18B20_convert_temp,
-		.precision	= w1_DS18S20_precision,
+		.set_resolution	= w1_DS18B20_set_resolution,
+		.get_resolution	= w1_DS18B20_get_resolution,
 		.eeprom		= w1_therm_eeprom
 	}
 };
 
 /* Helpers Functions */
 
+/**
+ * device_family() - Retrieve a pointer on &struct w1_therm_family_converter
+ * @sl: slave to retrieve the device specific structure
+ *
+ * Return: pointer to the slaves's family converter, NULL if not known
+ */
+static struct w1_therm_family_converter *device_family(struct w1_slave *sl)
+{
+	struct w1_therm_family_converter *ret = NULL;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(w1_therm_families); ++i) {
+		if (w1_therm_families[i].f->fid == sl->family->fid) {
+			ret = &w1_therm_families[i];
+			break;
+		}
+	}
+	return ret;
+}
+
 /**
  * bus_mutex_lock() - Acquire the mutex
  * @lock: w1 bus mutex to acquire
@@ -496,6 +555,9 @@ static inline int w1_convert_temp(u8 rom[9], u8 fid)
 
 static int w1_therm_add_slave(struct w1_slave *sl)
 {
+	struct w1_therm_family_converter *sl_family_conv;
+
+	/* Allocate memory*/
 	sl->family_data = kzalloc(sizeof(struct w1_therm_family_data),
 		GFP_KERNEL);
 	if (!sl->family_data)
@@ -503,6 +565,15 @@ static int w1_therm_add_slave(struct w1_slave *sl)
 
 	atomic_set(THERM_REFCNT(sl->family_data), 1);
 
+	/* Get a pointer to the device specific function struct */
+	sl_family_conv = device_family(sl);
+	if (!sl_family_conv) {
+		kfree(sl->family_data);
+		return -ENODEV;
+	}
+	/* save this pointer to the device structure */
+	SLAVE_SPECIFIC_FUNC(sl) = sl_family_conv;
+
 	/* Getting the power mode of the device {external, parasite}*/
 	SLAVE_POWERMODE(sl) = read_powermode(sl);
 
@@ -513,6 +584,18 @@ static int w1_therm_add_slave(struct w1_slave *sl)
 			 __func__, SLAVE_POWERMODE(sl));
 	}
 
+	/* Getting the resolution of the device */
+	if (SLAVE_SPECIFIC_FUNC(sl)->get_resolution) {
+		SLAVE_RESOLUTION(sl) =
+			SLAVE_SPECIFIC_FUNC(sl)->get_resolution(sl);
+		if (SLAVE_RESOLUTION(sl) < 0) {
+			/* no error returned as device has been added */
+			dev_warn(&sl->dev,
+				"%s:Device has been added, but resolution may be corrupted. err=%d\n",
+				__func__, SLAVE_RESOLUTION(sl));
+		}
+	}
+
 	return 0;
 }
 
@@ -639,6 +722,93 @@ error:
 	return ret;
 }
 
+static int read_scratchpad(struct w1_slave *sl, struct therm_info *info)
+{
+	struct w1_master *dev_master = sl->master;
+	int max_trying = W1_THERM_MAX_TRY;
+	int ret = -ENODEV;
+
+	info->verdict = 0;
+
+	if (!sl->family_data)
+		goto error;
+
+	memset(info->rom, 0, sizeof(info->rom));
+
+	/* prevent the slave from going away in sleep */
+	atomic_inc(THERM_REFCNT(sl->family_data));
+
+	if (!bus_mutex_lock(&dev_master->bus_mutex)) {
+		ret = -EAGAIN;	// Didn't acquire the mutex
+		goto dec_refcnt;
+	}
+
+	while (max_trying-- && ret) { /* ret should be 0 */
+		/* safe version to select slave */
+		if (!reset_select_slave(sl)) {
+			u8 nb_bytes_read;
+
+			w1_write_8(dev_master, W1_READ_SCRATCHPAD);
+
+			nb_bytes_read = w1_read_block(dev_master, info->rom, 9);
+			if (nb_bytes_read != 9) {
+				dev_warn(&sl->dev,
+					"w1_read_block(): returned %u instead of 9.\n",
+					nb_bytes_read);
+				ret = -EIO;
+			}
+
+			info->crc = w1_calc_crc8(info->rom, 8);
+
+			if (info->rom[8] == info->crc) {
+				info->verdict = 1;
+				ret = 0;
+			} else
+				ret = -EIO; /* CRC not checked */
+		}
+
+	}
+	mutex_unlock(&dev_master->bus_mutex);
+
+dec_refcnt:
+	atomic_dec(THERM_REFCNT(sl->family_data));
+error:
+	return ret;
+}
+
+static int write_scratchpad(struct w1_slave *sl, const u8 *data, u8 nb_bytes)
+{
+	struct w1_master *dev_master = sl->master;
+	int max_trying = W1_THERM_MAX_TRY;
+	int ret = -ENODEV;
+
+	if (!sl->family_data)
+		goto error;
+
+	/* prevent the slave from going away in sleep */
+	atomic_inc(THERM_REFCNT(sl->family_data));
+
+	if (!bus_mutex_lock(&dev_master->bus_mutex)) {
+		ret = -EAGAIN;	// Didn't acquire the mutex
+		goto dec_refcnt;
+	}
+
+	while (max_trying-- && ret) { /* ret should be 0 */
+		/* safe version to select slave */
+		if (!reset_select_slave(sl)) {
+			w1_write_8(dev_master, W1_WRITE_SCRATCHPAD);
+			w1_write_block(dev_master, data, nb_bytes);
+			ret = 0;
+		}
+	}
+	mutex_unlock(&dev_master->bus_mutex);
+
+dec_refcnt:
+	atomic_dec(THERM_REFCNT(sl->family_data));
+error:
+	return ret;
+}
+
 static inline int w1_therm_eeprom(struct device *device)
 {
 	struct w1_slave *sl = dev_to_w1_slave(device);
@@ -788,26 +958,38 @@ static ssize_t w1_slave_store(struct device *device,
 			      struct device_attribute *attr, const char *buf,
 			      size_t size)
 {
-	int val, ret;
+	int val, ret = 0;
 	struct w1_slave *sl = dev_to_w1_slave(device);
-	int i;
 
-	ret = kstrtoint(buf, 0, &val);
-	if (ret)
-		return ret;
+	ret = kstrtoint(buf, 10, &val); /* converting user entry to int */
 
-	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;
-		}
+	if (ret) {	/* conversion error */
+		dev_info(device,
+			"%s: conversion error. err= %d\n", __func__, ret);
+		return size;	/* return size to avoid call back again*/
+	}
+
+	if ((!sl->family_data) || (!SLAVE_SPECIFIC_FUNC(sl))) {
+		dev_info(device,
+			"%s: Device not supported by the driver\n", __func__);
+		return size;  /* No device family */
+	}
+
+	if (val == 0)	/* val=0 : trigger a EEPROM save */
+		ret = SLAVE_SPECIFIC_FUNC(sl)->eeprom(device);
+	else {
+		if (SLAVE_SPECIFIC_FUNC(sl)->set_resolution)
+			ret = SLAVE_SPECIFIC_FUNC(sl)->set_resolution(sl, val);
 	}
-	return ret ? : size;
+
+	if (ret) {
+		dev_info(device,
+			"%s: writing error %d\n", __func__, ret);
+		/* return size to avoid call back again*/
+	} else
+		SLAVE_RESOLUTION(sl) = val;
+
+	return size; /* always return size to avoid infinite calling */
 }
 
 static ssize_t ext_power_show(struct device *device,
@@ -832,6 +1014,66 @@ static ssize_t ext_power_show(struct device *device,
 	return sprintf(buf, "%d\n", SLAVE_POWERMODE(sl));
 }
 
+static ssize_t resolution_show(struct device *device,
+	struct device_attribute *attr, char *buf)
+{
+	struct w1_slave *sl = dev_to_w1_slave(device);
+
+	if ((!sl->family_data) || (!SLAVE_SPECIFIC_FUNC(sl))) {
+		dev_info(device,
+			"%s: Device not supported by the driver\n", __func__);
+		return 0;  /* No device family */
+	}
+
+	/* get the correct function depending on the device */
+	SLAVE_RESOLUTION(sl) = SLAVE_SPECIFIC_FUNC(sl)->get_resolution(sl);
+	if (SLAVE_RESOLUTION(sl) < 0) {
+		dev_dbg(device,
+			"%s: Resolution may be corrupted. err=%d\n",
+			__func__, SLAVE_RESOLUTION(sl));
+	}
+
+	return sprintf(buf, "%d\n", SLAVE_RESOLUTION(sl));
+}
+
+static ssize_t resolution_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;
+	int ret = 0;
+
+	ret = kstrtoint(buf, 10, &val); /* converting user entry to int */
+
+	if (ret) {	/* conversion error */
+		dev_info(device,
+			"%s: conversion error. err= %d\n", __func__, ret);
+		return size;	/* return size to avoid call back again*/
+	}
+
+	if ((!sl->family_data) || (!SLAVE_SPECIFIC_FUNC(sl))) {
+		dev_info(device,
+			"%s: Device not supported by the driver\n", __func__);
+		return size;  /* No device family */
+	}
+
+	/* Don't deal with the val enterd by user,
+	 * only device knows what is correct or not
+	 */
+
+	/* get the correct function depending on the device */
+	ret = SLAVE_SPECIFIC_FUNC(sl)->set_resolution(sl, val);
+
+	if (ret) {
+		dev_info(device,
+			"%s: writing error %d\n", __func__, ret);
+		/* return size to avoid call back again*/
+	} else
+		SLAVE_RESOLUTION(sl) = val;
+
+	return size;
+}
+
 #if IS_REACHABLE(CONFIG_HWMON)
 static int w1_read_temp(struct device *device, u32 attr, int channel,
 			long *val)
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v6 5/9] w1_therm: adding resolution sysfs entry
  2020-05-10 14:17 [PATCH v6 5/9] w1_therm: adding resolution sysfs entry Akira Shimahara
@ 2020-05-11  3:25 ` Randy Dunlap
  2020-05-11  7:13   ` Akira shimahara
  0 siblings, 1 reply; 4+ messages in thread
From: Randy Dunlap @ 2020-05-11  3:25 UTC (permalink / raw)
  To: Akira Shimahara, greg; +Cc: zbr, linux-kernel

Hi,

The kernel-doc comment changes look good.  Thanks for doing that.


On 5/10/20 7:17 AM, Akira Shimahara wrote:

> diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
> index 08579dc..b1734ae 100644
> --- a/drivers/w1/slaves/w1_therm.c
> +++ b/drivers/w1/slaves/w1_therm.c
> @@ -50,12 +50,24 @@ module_param_named(strong_pullup, w1_strong_pullup, int, 0);
>  
>  /* Helpers Macros */
>  
> +/* return a pointer on the slave w1_therm_family_converter struct:
> + * always test family data existence before
> + */

/*
 * Kernel multi-line comment coding style (except for networking source code)
 * is like this.
 */

(in multiple places)

> +/**
> + * write_scratchpad() - write nb_bytes in the device RAM
> + * @sl: pointer to the slave to write in
> + * @data: pointer to an array of 3 bytes, as 3 bytes MUST be written
> + * @nb_bytes: Nb bytes to be written (2 for DS18S20, 3 for other devices)

If Nb means Number, please spell it out.

> + *
> + * Return: 0 if success, -kernel error code otherwise
> + */
> +static int write_scratchpad(struct w1_slave *sl, const u8 *data, u8 nb_bytes);
> +
>  /**
>   * read_powermode() - Query the power mode of the slave
>   * @sl: slave to retrieve the power mode


thanks.
-- 
~Randy


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v6 5/9] w1_therm: adding resolution sysfs entry
  2020-05-11  3:25 ` Randy Dunlap
@ 2020-05-11  7:13   ` Akira shimahara
  0 siblings, 0 replies; 4+ messages in thread
From: Akira shimahara @ 2020-05-11  7:13 UTC (permalink / raw)
  To: Randy Dunlap, greg; +Cc: zbr, linux-kernel

Hi,

Le dimanche 10 mai 2020 à 20:25 -0700, Randy Dunlap a écrit :
> Hi,
> 
> 
> 
> The kernel-doc comment changes look good.  Thanks for doing that.
> 
> 
> 
> 
> 
> On 5/10/20 7:17 AM, Akira Shimahara wrote:
> 
> 
> 
> > diff --git a/drivers/w1/slaves/w1_therm.c
> > b/drivers/w1/slaves/w1_therm.c
> > index 08579dc..b1734ae 100644
> > --- a/drivers/w1/slaves/w1_therm.c
> > +++ b/drivers/w1/slaves/w1_therm.c
> > @@ -50,12 +50,24 @@ module_param_named(strong_pullup,
> > w1_strong_pullup, int, 0);
> >   
> >   /* Helpers Macros */
> >   
> > +/* return a pointer on the slave w1_therm_family_converter struct:
> > + * always test family data existence before
> > + */
> 
> 
> /*
> 
>  * Kernel multi-line comment coding style (except for networking
> source code)
> 
>  * is like this.
> 
>  */
> 
> 
> 
> (in multiple places)
> 
> 
> 
> > +/**
> > + * write_scratchpad() - write nb_bytes in the device RAM
> > + * @sl: pointer to the slave to write in
> > + * @data: pointer to an array of 3 bytes, as 3 bytes MUST be
> > written
> > + * @nb_bytes: Nb bytes to be written (2 for DS18S20, 3 for other
> > devices)
> 
> 
> If Nb means Number, please spell it out.
> 
> 
> 
> > + *
> > + * Return: 0 if success, -kernel error code otherwise
> > + */
> > +static int write_scratchpad(struct w1_slave *sl, const u8 *data,
> > u8 nb_bytes);
> > +
> >   /**
> >    * read_powermode() - Query the power mode of the slave
> >    * @sl: slave to retrieve the power mode
> 
> 
> 
> 
> thanks.
> 
> --
> 
> ~Randy

Thanks for your comments and your time,
well noted, I will do
accordingly.

Akira Shimahara


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v6 5/9] w1_therm: adding resolution sysfs entry
@ 2020-05-10 16:28 kbuild test robot
  0 siblings, 0 replies; 4+ messages in thread
From: kbuild test robot @ 2020-05-10 16:28 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 5475 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20200510141707.173273-1-akira215corp@gmail.com>
References: <20200510141707.173273-1-akira215corp@gmail.com>
TO: Akira Shimahara <akira215corp@gmail.com>
TO: greg(a)kroah.com
TO: rdunlap(a)infradead.org
CC: zbr(a)ioremap.net
CC: linux-kernel(a)vger.kernel.org
CC: Akira Shimahara <akira215corp@gmail.com>

Hi Akira,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.7-rc4 next-20200508]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Akira-Shimahara/w1_therm-adding-code-comments-and-code-reordering/20200510-222040
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e99332e7b4cda6e60f5b5916cf9943a79dbef902
:::::: branch date: 2 hours ago
:::::: commit date: 2 hours ago

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>


cppcheck warnings: (new ones prefixed by >>)

>> drivers/w1/slaves/w1_therm.c:390:6: warning: Redundant initialization for 'ret'. The initialized value is overwritten before it is read. [redundantInitialization]
    ret = read_scratchpad(sl, &info);
        ^
   drivers/w1/slaves/w1_therm.c:376:10: note: ret is initialized
    int ret = -ENODEV;
            ^
   drivers/w1/slaves/w1_therm.c:390:6: note: ret is overwritten
    ret = read_scratchpad(sl, &info);
        ^
   drivers/w1/slaves/w1_therm.c:412:6: warning: Redundant initialization for 'ret'. The initialized value is overwritten before it is read. [redundantInitialization]
    ret = read_scratchpad(sl, &info);
        ^
   drivers/w1/slaves/w1_therm.c:408:10: note: ret is initialized
    int ret = -ENODEV;
            ^
   drivers/w1/slaves/w1_therm.c:412:6: note: ret is overwritten
    ret = read_scratchpad(sl, &info);
        ^
>> drivers/w1/slaves/w1_therm.c:765:9: warning: Variable 'ret' is reassigned a value before the old one has been used. [redundantAssignment]
       ret = 0;
           ^
   drivers/w1/slaves/w1_therm.c:758:9: note: ret is assigned
       ret = -EIO;
           ^
   drivers/w1/slaves/w1_therm.c:765:9: note: ret is overwritten
       ret = 0;
           ^

# https://github.com/0day-ci/linux/commit/660c8cc0fcf6b44dc06925055938090c4c937dab
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 660c8cc0fcf6b44dc06925055938090c4c937dab
vim +/ret +390 drivers/w1/slaves/w1_therm.c

0a19f129d71f18 Ben Sen            2016-05-01  373  
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  374  static inline int w1_DS18B20_set_resolution(struct w1_slave *sl, int val)
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  375  {
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  376  	int ret = -ENODEV;
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  377  	u8 new_config_register[3];	/* array of data to be written */
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  378  	struct therm_info info;
f53459c00871a7 Alexey Khoroshilov 2017-10-22  379  
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  380  	/* resolution of DS18B20 is in the range [9..12] bits */
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  381  	if (val < 9 || val > 12)
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  382  		return -EINVAL;
f53459c00871a7 Alexey Khoroshilov 2017-10-22  383  
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  384  	val -= 9; /* soustract 9 the lowest resolution in bit */
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  385  	val = (val << 5); /* shift to position bit 5 & bit 6 */
0a19f129d71f18 Ben Sen            2016-05-01  386  
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  387  	/* Read the scratchpad to change only the required bits
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  388  	 * (bit5 & bit 6 from byte 4)
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  389  	 */
660c8cc0fcf6b4 Akira Shimahara    2020-05-10 @390  	ret = read_scratchpad(sl, &info);
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  391  	if (!ret) {
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  392  		new_config_register[0] = info.rom[2];
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  393  		new_config_register[1] = info.rom[3];
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  394  		/* config register is byte 4 & mask 0b10011111*/
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  395  		new_config_register[2] = (info.rom[4] & 0x9F) |
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  396  					(u8) val;
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  397  	} else
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  398  		return ret;
0a19f129d71f18 Ben Sen            2016-05-01  399  
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  400  	/* Write data in the device RAM */
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  401  	ret = w1_DS18B20_write_data(sl, new_config_register);
0a19f129d71f18 Ben Sen            2016-05-01  402  
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  403  	return ret;
660c8cc0fcf6b4 Akira Shimahara    2020-05-10  404  }
0a19f129d71f18 Ben Sen            2016-05-01  405  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-05-11  7:16 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-10 14:17 [PATCH v6 5/9] w1_therm: adding resolution sysfs entry Akira Shimahara
2020-05-11  3:25 ` Randy Dunlap
2020-05-11  7:13   ` Akira shimahara
2020-05-10 16:28 kbuild test robot

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.