[PATCH v3 2/5] w1_therm: adding sysfs entry to check device power

[PATCH v3 2/5] w1_therm: adding sysfs entry to check device power

[Akira Shimahara]

Patch for enhacement of w1_therm module.
Adding ext_power sysfs entry (RO). Return the power status of the device:
 - 0: device parasite powered
 - 1: device externally powered
 - xx: xx is kernel error

Creating Documentation/ABI/testing/sysfs-driver-w1_therm for the old 
driver sysfs and this new entry.

Signed-off-by: Akira Shimahara <akira215corp@gmail.com>
---
 .../ABI/testing/sysfs-driver-w1_therm         | 29 ++++++
 drivers/w1/slaves/w1_therm.c                  | 93 ++++++++++++++++++-
 drivers/w1/slaves/w1_therm.h                  | 44 ++++++++-
 3 files changed, 163 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-w1_therm

diff --git a/Documentation/ABI/testing/sysfs-driver-w1_therm b/Documentation/ABI/testing/sysfs-driver-w1_therm
new file mode 100644
index 0000000..9aaf625
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-w1_therm
@@ -0,0 +1,29 @@
+What:		/sys/bus/w1/devices/.../ext_power
+Date:		Apr 2020
+Contact:	Akira Shimahara <akira215corp@gmail.com>
+Description:
+		(RO) return the power status by asking the device
+			* `0`: device parasite powered
+			* `1`: device externally powered
+			* `-xx`: xx is kernel error when reading power status
+Users:		any user space application which wants to communicate with
+		w1_term device
+
+
+What:		/sys/bus/w1/devices/.../w1_slave
+Date:		Apr 2020
+Contact:	Akira Shimahara <akira215corp@gmail.com>
+Description:
+		(RW) return the temperature in 1/1000 degC.
+		*read*: return 2 lines with the hexa output data sent on the
+		bus, return the CRC check and temperature in 1/1000 degC
+		*write* :
+			* `0` : save the 2 or 3 bytes to the device EEPROM
+			(i.e. TH, TL and config register)
+			* `9..12` : set the device resolution in RAM
+			(if supported)
+			* Anything else: do nothing
+		refer to Documentation/w1/slaves/w1_therm.rst for detailed
+		information.
+Users:		any user space application which wants to communicate with
+		w1_term device
\ No newline at end of file
diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index 6245950..a530853 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -39,12 +39,14 @@ module_param_named(strong_pullup, w1_strong_pullup, int, 0);
 
 static struct attribute *w1_therm_attrs[] = {
 	&dev_attr_w1_slave.attr,
+	&dev_attr_ext_power.attr,
 	NULL,
 };
 
 static struct attribute *w1_ds28ea00_attrs[] = {
 	&dev_attr_w1_slave.attr,
 	&dev_attr_w1_seq.attr,
+	&dev_attr_ext_power.attr,
 	NULL,
 };
 
@@ -294,6 +296,26 @@ static inline int w1_DS18S20_convert_temp(u8 rom[9])
 	return t;
 }
 
+/*------------------------ Helpers Functions----------------------------*/
+
+static inline bool bus_mutex_lock(struct mutex *lock)
+{
+	int max_trying = W1_THERM_MAX_TRY;
+	/* try to acquire the mutex, if not, sleep retry_delay before retry) */
+	while (mutex_lock_interruptible(lock) != 0 && max_trying > 0) {
+		unsigned long sleep_rem;
+
+		sleep_rem = msleep_interruptible(W1_THERM_RETRY_DELAY);
+		if (!sleep_rem)
+			max_trying--;
+	}
+
+	if (!max_trying)
+		return false;	/* Didn't acquire the bus mutex */
+
+	return true;
+}
+
 /*-------------------------Interface Functions------------------------------*/
 static int w1_therm_add_slave(struct w1_slave *sl)
 {
@@ -302,6 +324,16 @@ static int w1_therm_add_slave(struct w1_slave *sl)
 	if (!sl->family_data)
 		return -ENOMEM;
 	atomic_set(THERM_REFCNT(sl->family_data), 1);
+
+	/* Getting the power mode of the device {external, parasite}*/
+	SLAVE_POWERMODE(sl) = read_powermode(sl);
+
+	if (SLAVE_POWERMODE(sl) < 0) {
+		/* no error returned as device has been added */
+		dev_warn(&sl->dev,
+			"%s: Device has been added, but power_mode may be corrupted. err=%d\n",
+			 __func__, SLAVE_POWERMODE(sl));
+	}
 	return 0;
 }
 
@@ -512,6 +544,43 @@ error:
 	return ret;
 }
 
+static int read_powermode(struct w1_slave *sl)
+{
+	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 < 0)) {
+		/* safe version to select slave */
+		if (!reset_select_slave(sl)) {
+			w1_write_8(dev_master, W1_READ_PSUPPLY);
+			/* Read only one bit,
+			 * 1 is externally powered,
+			 * 0 is parasite powered
+			 */
+			ret = w1_touch_bit(dev_master, 1);
+			/* ret should be either 1 either 0 */
+		}
+	}
+	mutex_unlock(&dev_master->bus_mutex);
+
+dec_refcnt:
+	atomic_dec(THERM_REFCNT(sl->family_data));
+error:
+	return ret;
+}
+
 /*------------------------Interface sysfs--------------------------*/
 
 static ssize_t w1_slave_show(struct device *device,
@@ -565,13 +634,35 @@ static ssize_t w1_slave_store(struct device *device,
 				ret = w1_therm_families[i].eeprom(device);
 			else
 				ret = w1_therm_families[i].precision(device,
-								val);
+									val);
 			break;
 		}
 	}
 	return ret ? : size;
 }
 
+static ssize_t ext_power_show(struct device *device,
+	struct device_attribute *attr, char *buf)
+{
+	struct w1_slave *sl = dev_to_w1_slave(device);
+
+	if (!sl->family_data) {
+		dev_info(device,
+			"%s: Device not supported by the driver\n", __func__);
+		return 0;  /* No device family */
+	}
+
+	/* Getting the power mode of the device {external, parasite}*/
+	SLAVE_POWERMODE(sl) = read_powermode(sl);
+
+	if (SLAVE_POWERMODE(sl) < 0) {
+		dev_dbg(device,
+			"%s: Power_mode may be corrupted. err=%d\n",
+			__func__, SLAVE_POWERMODE(sl));
+	}
+	return sprintf(buf, "%d\n", SLAVE_POWERMODE(sl));
+}
+
 #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
index b73af0b..2f975a4 100644
--- a/drivers/w1/slaves/w1_therm.h
+++ b/drivers/w1/slaves/w1_therm.h
@@ -25,6 +25,12 @@
 #include <linux/mutex.h>
 #include <linux/w1.h>
 
+/*----------------------------------Defines---------------------------------*/
+/* Nb of try for an operation */
+#define W1_THERM_MAX_TRY		5
+
+/* ms delay to retry bus mutex */
+#define W1_THERM_RETRY_DELAY		20
 /*----------------------------------Structs---------------------------------*/
 
 /**
@@ -47,10 +53,15 @@ struct w1_therm_family_converter {
  * struct w1_therm_family_data
  * @param rom data
  * @param refcnt ref count
+ * @param external_powered
+ *		1 device powered externally,
+ *		0 device parasite powered,
+ *		-x error or undefined
  */
 struct w1_therm_family_data {
 	uint8_t rom[9];
 	atomic_t refcnt;
+	int external_powered;
 };
 
 /**
@@ -80,11 +91,24 @@ static inline int w1_DS18B20_convert_temp(u8 rom[9]);
 static inline int w1_DS18S20_convert_temp(u8 rom[9]);
 
 /*-------------------------------Macros--------------------------------------*/
+/* 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 address of the refcnt in the family data */
 #define THERM_REFCNT(family_data) \
 	(&((struct w1_therm_family_data *)family_data)->refcnt)
 
+/*-------------------------- Helpers Functions------------------------------*/
+
+/** bus_mutex_lock() get the mutex & retry
+ *  @param lock w1 bus mutex to get
+ *  @return value true is mutex is acquired and lock, false otherwise
+ */
+static inline bool bus_mutex_lock(struct mutex *lock);
+
 /*---------------------------Hardware Functions-----------------------------*/
 
 /**
@@ -107,7 +131,14 @@ static int reset_select_slave(struct w1_slave *sl);
  */
 static ssize_t read_therm(struct device *device,
 			struct w1_slave *sl, struct therm_info *info);
-
+/** read_powermode()
+ * @brief ask the device to get its power mode {external, parasite}
+ * @param sl slave to be interrogated
+ * @return	0 parasite powered device
+ *			1 externally powered device
+ *			<0 kernel error code
+ */
+static int read_powermode(struct w1_slave *sl);
 /*----------------------------Interface sysfs-------------------------------*/
 
 /** @brief A callback function to output the temperature Old way
@@ -127,11 +158,20 @@ static ssize_t w1_slave_store(struct device *device,
 
 static ssize_t w1_seq_show(struct device *device,
 	struct device_attribute *attr, char *buf);
-
+/** @brief A callback function to output the power mode of the device
+ *	Once done, it is stored in the sl->family_data to avoid doing the test
+ *	during data read
+ *  @return	0 : device parasite powered
+ *			1 : device externally powered
+ *			-xx : xx is kernel error code
+ */
+static ssize_t ext_power_show(struct device *device,
+	struct device_attribute *attr, char *buf);
 /*-----------------------------Attributes declarations----------------------*/
 
 static DEVICE_ATTR_RW(w1_slave);
 static DEVICE_ATTR_RO(w1_seq);
+static DEVICE_ATTR_RO(ext_power);
 
 /*--------------------------Interface Functions-----------------------------*/
 
-- 
2.25.4

Re: [PATCH v3 2/5] w1_therm: adding sysfs entry to check device power

[Greg KH]

On Wed, Apr 29, 2020 at 03:32:04PM +0200, Akira Shimahara wrote:
> Patch for enhacement of w1_therm module.
> Adding ext_power sysfs entry (RO). Return the power status of the device:
>  - 0: device parasite powered
>  - 1: device externally powered
>  - xx: xx is kernel error
> 
> Creating Documentation/ABI/testing/sysfs-driver-w1_therm for the old 
> driver sysfs and this new entry.
> 
> Signed-off-by: Akira Shimahara <akira215corp@gmail.com>
> ---
>  .../ABI/testing/sysfs-driver-w1_therm         | 29 ++++++
>  drivers/w1/slaves/w1_therm.c                  | 93 ++++++++++++++++++-
>  drivers/w1/slaves/w1_therm.h                  | 44 ++++++++-
>  3 files changed, 163 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-w1_therm
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-w1_therm b/Documentation/ABI/testing/sysfs-driver-w1_therm
> new file mode 100644
> index 0000000..9aaf625
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-w1_therm
> @@ -0,0 +1,29 @@
> +What:		/sys/bus/w1/devices/.../ext_power
> +Date:		Apr 2020
> +Contact:	Akira Shimahara <akira215corp@gmail.com>
> +Description:
> +		(RO) return the power status by asking the device
> +			* `0`: device parasite powered
> +			* `1`: device externally powered
> +			* `-xx`: xx is kernel error when reading power status
> +Users:		any user space application which wants to communicate with
> +		w1_term device
> +
> +
> +What:		/sys/bus/w1/devices/.../w1_slave
> +Date:		Apr 2020
> +Contact:	Akira Shimahara <akira215corp@gmail.com>
> +Description:
> +		(RW) return the temperature in 1/1000 degC.
> +		*read*: return 2 lines with the hexa output data sent on the
> +		bus, return the CRC check and temperature in 1/1000 degC

the w1_slave file returns a temperature???

And sysfs is 1 value-per-file, not multiple lines.

And as this is a temperature, what's wrong with the iio interface that
handles temperature already?  Don't go making up new userspace apis when
we already have good ones today :)

> +		*write* :
> +			* `0` : save the 2 or 3 bytes to the device EEPROM
> +			(i.e. TH, TL and config register)
> +			* `9..12` : set the device resolution in RAM
> +			(if supported)

I don't understand these write values, how do they match up to a
temperature readin?

> +			* Anything else: do nothing
> +		refer to Documentation/w1/slaves/w1_therm.rst for detailed
> +		information.
> +Users:		any user space application which wants to communicate with
> +		w1_term device
> \ No newline at end of file
> diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
> index 6245950..a530853 100644
> --- a/drivers/w1/slaves/w1_therm.c
> +++ b/drivers/w1/slaves/w1_therm.c
> @@ -39,12 +39,14 @@ module_param_named(strong_pullup, w1_strong_pullup, int, 0);
>  
>  static struct attribute *w1_therm_attrs[] = {
>  	&dev_attr_w1_slave.attr,
> +	&dev_attr_ext_power.attr,
>  	NULL,
>  };
>  
>  static struct attribute *w1_ds28ea00_attrs[] = {
>  	&dev_attr_w1_slave.attr,
>  	&dev_attr_w1_seq.attr,
> +	&dev_attr_ext_power.attr,
>  	NULL,
>  };
>  
> @@ -294,6 +296,26 @@ static inline int w1_DS18S20_convert_temp(u8 rom[9])
>  	return t;
>  }
>  
> +/*------------------------ Helpers Functions----------------------------*/
> +
> +static inline bool bus_mutex_lock(struct mutex *lock)
> +{
> +	int max_trying = W1_THERM_MAX_TRY;
> +	/* try to acquire the mutex, if not, sleep retry_delay before retry) */

Please use scripts/checkpatch.pl on your patches, it should have asked
you to put an empty line after the int definition.



> +	while (mutex_lock_interruptible(lock) != 0 && max_trying > 0) {
> +		unsigned long sleep_rem;
> +
> +		sleep_rem = msleep_interruptible(W1_THERM_RETRY_DELAY);
> +		if (!sleep_rem)
> +			max_trying--;
> +	}
> +
> +	if (!max_trying)
> +		return false;	/* Didn't acquire the bus mutex */
> +
> +	return true;
> +}
> +
>  /*-------------------------Interface Functions------------------------------*/
>  static int w1_therm_add_slave(struct w1_slave *sl)
>  {
> @@ -302,6 +324,16 @@ static int w1_therm_add_slave(struct w1_slave *sl)
>  	if (!sl->family_data)
>  		return -ENOMEM;
>  	atomic_set(THERM_REFCNT(sl->family_data), 1);
> +
> +	/* Getting the power mode of the device {external, parasite}*/
> +	SLAVE_POWERMODE(sl) = read_powermode(sl);
> +
> +	if (SLAVE_POWERMODE(sl) < 0) {
> +		/* no error returned as device has been added */
> +		dev_warn(&sl->dev,
> +			"%s: Device has been added, but power_mode may be corrupted. err=%d\n",
> +			 __func__, SLAVE_POWERMODE(sl));
> +	}
>  	return 0;
>  }
>  
> @@ -512,6 +544,43 @@ error:
>  	return ret;
>  }
>  
> +static int read_powermode(struct w1_slave *sl)
> +{
> +	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 < 0)) {
> +		/* safe version to select slave */
> +		if (!reset_select_slave(sl)) {
> +			w1_write_8(dev_master, W1_READ_PSUPPLY);
> +			/* Read only one bit,
> +			 * 1 is externally powered,
> +			 * 0 is parasite powered
> +			 */
> +			ret = w1_touch_bit(dev_master, 1);
> +			/* ret should be either 1 either 0 */
> +		}
> +	}
> +	mutex_unlock(&dev_master->bus_mutex);
> +
> +dec_refcnt:
> +	atomic_dec(THERM_REFCNT(sl->family_data));
> +error:
> +	return ret;
> +}
> +
>  /*------------------------Interface sysfs--------------------------*/
>  
>  static ssize_t w1_slave_show(struct device *device,
> @@ -565,13 +634,35 @@ static ssize_t w1_slave_store(struct device *device,
>  				ret = w1_therm_families[i].eeprom(device);
>  			else
>  				ret = w1_therm_families[i].precision(device,
> -								val);
> +									val);
>  			break;
>  		}
>  	}
>  	return ret ? : size;
>  }
>  
> +static ssize_t ext_power_show(struct device *device,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct w1_slave *sl = dev_to_w1_slave(device);
> +
> +	if (!sl->family_data) {
> +		dev_info(device,
> +			"%s: Device not supported by the driver\n", __func__);
> +		return 0;  /* No device family */
> +	}
> +
> +	/* Getting the power mode of the device {external, parasite}*/
> +	SLAVE_POWERMODE(sl) = read_powermode(sl);
> +
> +	if (SLAVE_POWERMODE(sl) < 0) {
> +		dev_dbg(device,
> +			"%s: Power_mode may be corrupted. err=%d\n",
> +			__func__, SLAVE_POWERMODE(sl));
> +	}
> +	return sprintf(buf, "%d\n", SLAVE_POWERMODE(sl));
> +}
> +
>  #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
> index b73af0b..2f975a4 100644
> --- a/drivers/w1/slaves/w1_therm.h
> +++ b/drivers/w1/slaves/w1_therm.h
> @@ -25,6 +25,12 @@
>  #include <linux/mutex.h>
>  #include <linux/w1.h>
>  
> +/*----------------------------------Defines---------------------------------*/

No real need for this, we can see defines just fine :)

thanks,

greg k-h

_

[Akira shimahara]

Le mercredi 29 avril 2020 à 15:46 +0200, Greg KH a écrit :
> On Wed, Apr 29, 2020 at 03:32:04PM +0200, Akira Shimahara wrote:
> > Patch for enhacement of w1_therm module.
> > Adding ext_power sysfs entry (RO). Return the power status of the
> > device:
> >  - 0: device parasite powered
> >  - 1: device externally powered
> >  - xx: xx is kernel error
> > 
> > Creating Documentation/ABI/testing/sysfs-driver-w1_therm for the
> > old 
> > driver sysfs and this new entry.
> > 
> > Signed-off-by: Akira Shimahara <akira215corp@gmail.com>
> > ---
> >  .../ABI/testing/sysfs-driver-w1_therm         | 29 ++++++
> >  drivers/w1/slaves/w1_therm.c                  | 93
> > ++++++++++++++++++-
> >  drivers/w1/slaves/w1_therm.h                  | 44 ++++++++-
> >  3 files changed, 163 insertions(+), 3 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-driver-w1_therm
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-driver-w1_therm
> > b/Documentation/ABI/testing/sysfs-driver-w1_therm
> > new file mode 100644
> > index 0000000..9aaf625
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-driver-w1_therm
> > @@ -0,0 +1,29 @@
> > +What:		/sys/bus/w1/devices/.../ext_power
> > +Date:		Apr 2020
> > +Contact:	Akira Shimahara <akira215corp@gmail.com>
> > +Description:
> > +		(RO) return the power status by asking the device
> > +			* `0`: device parasite powered
> > +			* `1`: device externally powered
> > +			* `-xx`: xx is kernel error when reading power
> > status
> > +Users:		any user space application which wants to
> > communicate with
> > +		w1_term device
> > +
> > +
> > +What:		/sys/bus/w1/devices/.../w1_slave
> > +Date:		Apr 2020
> > +Contact:	Akira Shimahara <akira215corp@gmail.com>
> > +Description:
> > +		(RW) return the temperature in 1/1000 degC.
> > +		*read*: return 2 lines with the hexa output data sent
> > on the
> > +		bus, return the CRC check and temperature in 1/1000
> > degC
> 
> the w1_slave file returns a temperature???
> 
> And sysfs is 1 value-per-file, not multiple lines.
> 
> And as this is a temperature, what's wrong with the iio interface
> that
> handles temperature already?  Don't go making up new userspace apis
> when
> we already have good ones today :)

Yes the existing syfs w1_slave return 2 lines, user application 
catch normally only the temperature. We keep it as many userspace
application are already based on this output, but to ease user
to catch the only purpose of these devices (temperature sensors),
we added on entry which return only the temperature (avoiding
string parsing ,... on both side i.e. driver and user app).

> 
> > +		*write* :
> > +			* `0` : save the 2 or 3 bytes to the device
> > EEPROM
> > +			(i.e. TH, TL and config register)
> > +			* `9..12` : set the device resolution in RAM
> > +			(if supported)
> 
> I don't understand these write values, how do they match up to a
> temperature readin?

This is the previous implementation, and yes, it was not very clear.
These value are not connected to temperature reading. The sysfs 
entry was used to enter user value to device RAM, in 2 registers:
if 0 it trigger a save to the device EEPROM, if the value is between
9 and 12, it stores in the resolution register of the device.
In the next patches, we add more sysfs entries to separate things.

> 
> > +			* Anything else: do nothing
> > +		refer to Documentation/w1/slaves/w1_therm.rst for
> > detailed
> > +		information.
> > +Users:		any user space application which wants to
> > communicate with
> > +		w1_term device
> > \ No newline at end of file
> > diff --git a/drivers/w1/slaves/w1_therm.c
> > b/drivers/w1/slaves/w1_therm.c
> > index 6245950..a530853 100644
> > --- a/drivers/w1/slaves/w1_therm.c
> > +++ b/drivers/w1/slaves/w1_therm.c
> > @@ -39,12 +39,14 @@ module_param_named(strong_pullup,
> > w1_strong_pullup, int, 0);
> >  
> >  static struct attribute *w1_therm_attrs[] = {
> >  	&dev_attr_w1_slave.attr,
> > +	&dev_attr_ext_power.attr,
> >  	NULL,
> >  };
> >  
> >  static struct attribute *w1_ds28ea00_attrs[] = {
> >  	&dev_attr_w1_slave.attr,
> >  	&dev_attr_w1_seq.attr,
> > +	&dev_attr_ext_power.attr,
> >  	NULL,
> >  };
> >  
> > @@ -294,6 +296,26 @@ static inline int w1_DS18S20_convert_temp(u8
> > rom[9])
> >  	return t;
> >  }
> >  
> > +/*------------------------ Helpers Functions--------------------
> > --------*/
> > +
> > +static inline bool bus_mutex_lock(struct mutex *lock)
> > +{
> > +	int max_trying = W1_THERM_MAX_TRY;
> > +	/* try to acquire the mutex, if not, sleep retry_delay before
> > retry) */
> 
> Please use scripts/checkpatch.pl on your patches, it should have
> asked
> you to put an empty line after the int definition.
> 
I used it, no warning on this line but I will add
> 
> 
> > +	while (mutex_lock_interruptible(lock) != 0 && max_trying > 0) {
> > +		unsigned long sleep_rem;
> > +
> > +		sleep_rem = msleep_interruptible(W1_THERM_RETRY_DELAY);
> > +		if (!sleep_rem)
> > +			max_trying--;
> > +	}
> > +
> > +	if (!max_trying)
> > +		return false;	/* Didn't acquire the bus mutex */
> > +
> > +	return true;
> > +}
> > +
> >  /*-------------------------Interface Functions------------------
> > ------------*/
> >  static int w1_therm_add_slave(struct w1_slave *sl)
> >  {
> > @@ -302,6 +324,16 @@ static int w1_therm_add_slave(struct w1_slave
> > *sl)
> >  	if (!sl->family_data)
> >  		return -ENOMEM;
> >  	atomic_set(THERM_REFCNT(sl->family_data), 1);
> > +
> > +	/* Getting the power mode of the device {external, parasite}*/
> > +	SLAVE_POWERMODE(sl) = read_powermode(sl);
> > +
> > +	if (SLAVE_POWERMODE(sl) < 0) {
> > +		/* no error returned as device has been added */
> > +		dev_warn(&sl->dev,
> > +			"%s: Device has been added, but power_mode may
> > be corrupted. err=%d\n",
> > +			 __func__, SLAVE_POWERMODE(sl));
> > +	}
> >  	return 0;
> >  }
> >  
> > @@ -512,6 +544,43 @@ error:
> >  	return ret;
> >  }
> >  
> > +static int read_powermode(struct w1_slave *sl)
> > +{
> > +	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 < 0)) {
> > +		/* safe version to select slave */
> > +		if (!reset_select_slave(sl)) {
> > +			w1_write_8(dev_master, W1_READ_PSUPPLY);
> > +			/* Read only one bit,
> > +			 * 1 is externally powered,
> > +			 * 0 is parasite powered
> > +			 */
> > +			ret = w1_touch_bit(dev_master, 1);
> > +			/* ret should be either 1 either 0 */
> > +		}
> > +	}
> > +	mutex_unlock(&dev_master->bus_mutex);
> > +
> > +dec_refcnt:
> > +	atomic_dec(THERM_REFCNT(sl->family_data));
> > +error:
> > +	return ret;
> > +}
> > +
> >  /*------------------------Interface sysfs-------------------------
> > -*/
> >  
> >  static ssize_t w1_slave_show(struct device *device,
> > @@ -565,13 +634,35 @@ static ssize_t w1_slave_store(struct device
> > *device,
> >  				ret =
> > w1_therm_families[i].eeprom(device);
> >  			else
> >  				ret =
> > w1_therm_families[i].precision(device,
> > -								val);
> > +									
> > val);
> >  			break;
> >  		}
> >  	}
> >  	return ret ? : size;
> >  }
> >  
> > +static ssize_t ext_power_show(struct device *device,
> > +	struct device_attribute *attr, char *buf)
> > +{
> > +	struct w1_slave *sl = dev_to_w1_slave(device);
> > +
> > +	if (!sl->family_data) {
> > +		dev_info(device,
> > +			"%s: Device not supported by the driver\n",
> > __func__);
> > +		return 0;  /* No device family */
> > +	}
> > +
> > +	/* Getting the power mode of the device {external, parasite}*/
> > +	SLAVE_POWERMODE(sl) = read_powermode(sl);
> > +
> > +	if (SLAVE_POWERMODE(sl) < 0) {
> > +		dev_dbg(device,
> > +			"%s: Power_mode may be corrupted. err=%d\n",
> > +			__func__, SLAVE_POWERMODE(sl));
> > +	}
> > +	return sprintf(buf, "%d\n", SLAVE_POWERMODE(sl));
> > +}
> > +
> >  #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
> > index b73af0b..2f975a4 100644
> > --- a/drivers/w1/slaves/w1_therm.h
> > +++ b/drivers/w1/slaves/w1_therm.h
> > @@ -25,6 +25,12 @@
> >  #include <linux/mutex.h>
> >  #include <linux/w1.h>
> >  
> > +/*----------------------------------Defines-----------------------
> > ----------*/
> 
> No real need for this, we can see defines just fine :)
> 
Well noted
> thanks,
> 
> greg k-h

Re: [PATCH v3 2/5] w1_therm: adding sysfs entry to check device power

[Evgeniy Polyakov]

Hi

29.04.2020, 16:47, "Greg KH" <greg@kroah.com>:

>>  +What: /sys/bus/w1/devices/.../w1_slave
>>  +Date: Apr 2020
>>  +Contact: Akira Shimahara <akira215corp@gmail.com>
>>  +Description:
>>  + (RW) return the temperature in 1/1000 degC.
>>  + *read*: return 2 lines with the hexa output data sent on the
>>  + bus, return the CRC check and temperature in 1/1000 degC
>
> the w1_slave file returns a temperature???
>
> And sysfs is 1 value-per-file, not multiple lines.

It was 'content crc' previously, and probably a good idea would be to add just one file with 'content'

> And as this is a temperature, what's wrong with the iio interface that
> handles temperature already? Don't go making up new userspace apis when
> we already have good ones today :)

What is that?
w1 always had a sysfs files for its contents whether it is converted temperature or raw bytes data,
there is also netlink interface which is there since the day one. 

>>  + *write* :
>>  + * `0` : save the 2 or 3 bytes to the device EEPROM
>>  + (i.e. TH, TL and config register)
>>  + * `9..12` : set the device resolution in RAM
>>  + (if supported)
>
> I don't understand these write values, how do they match up to a
> temperature readin?

You kind of writing to device about how to convert its raw content into readable content, which will eventually become a temperature

Re: [PATCH v3 2/5] w1_therm: adding sysfs entry to check device power

[Akira shimahara]

Hello,

Le mercredi 29 avril 2020 à 18:18 +0300, Evgeniy Polyakov a écrit :
> Hi
> 
> 
> 
> 29.04.2020, 16:47, "Greg KH" <greg@kroah.com>:
> 
> 
> 
> > >  +What: /sys/bus/w1/devices/.../w1_slave
> > >  +Date: Apr 2020
> > >  +Contact: Akira Shimahara <akira215corp@gmail.com>
> > >  +Description:
> > >  + (RW) return the temperature in 1/1000 degC.
> > >  + *read*: return 2 lines with the hexa output data sent on the
> > >  + bus, return the CRC check and temperature in 1/1000 degC
> > the w1_slave file returns a temperature???
> > And sysfs is 1 value-per-file, not multiple lines.
> 
> 
> It was 'content crc' previously, and probably a good idea would be to
> add just one file with 'content'
 
That's the purpose of the new sysfs entry 'temperature'. It only
content temperature. As already mentionned we have to keep the w1_slave
entry for compatibility purpose, all existing user application parse
this file.

I submitted the patch series yesterday night, splitting it in 9
patches.

Regards,

Akira Shimahara

Re: [PATCH v3 2/5] w1_therm: adding sysfs entry to check device power

[Greg KH]

On Thu, Apr 30, 2020 at 12:34:03PM +0200, Akira shimahara wrote:
> Hello,
> 
> Le mercredi 29 avril 2020 à 18:18 +0300, Evgeniy Polyakov a écrit :
> > Hi
> > 
> > 
> > 
> > 29.04.2020, 16:47, "Greg KH" <greg@kroah.com>:
> > 
> > 
> > 
> > > >  +What: /sys/bus/w1/devices/.../w1_slave
> > > >  +Date: Apr 2020
> > > >  +Contact: Akira Shimahara <akira215corp@gmail.com>
> > > >  +Description:
> > > >  + (RW) return the temperature in 1/1000 degC.
> > > >  + *read*: return 2 lines with the hexa output data sent on the
> > > >  + bus, return the CRC check and temperature in 1/1000 degC
> > > the w1_slave file returns a temperature???
> > > And sysfs is 1 value-per-file, not multiple lines.
> > 
> > 
> > It was 'content crc' previously, and probably a good idea would be to
> > add just one file with 'content'
>  
> That's the purpose of the new sysfs entry 'temperature'. It only
> content temperature. As already mentionned we have to keep the w1_slave
> entry for compatibility purpose, all existing user application parse
> this file.

That's fine, but the document you wrote here says the file is called
"w1_slave", not "temperature" :)

Re: [PATCH v3 2/5] w1_therm: adding sysfs entry to check device power

[Akira shimahara]

Le jeudi 30 avril 2020 à 13:21 +0200, Greg KH a écrit :
> On Thu, Apr 30, 2020 at 12:34:03PM +0200, Akira shimahara wrote:
> > Hello,
> > 
> > Le mercredi 29 avril 2020 à 18:18 +0300, Evgeniy Polyakov a écrit :
> > > Hi
> > > 
> > > 
> > > 
> > > 29.04.2020, 16:47, "Greg KH" <greg@kroah.com>:
> > > 
> > > 
> > > 
> > > > >  +What: /sys/bus/w1/devices/.../w1_slave
> > > > >  +Date: Apr 2020
> > > > >  +Contact: Akira Shimahara <akira215corp@gmail.com>
> > > > >  +Description:
> > > > >  + (RW) return the temperature in 1/1000 degC.
> > > > >  + *read*: return 2 lines with the hexa output data sent on
> > > > > the
> > > > >  + bus, return the CRC check and temperature in 1/1000 degC
> > > > the w1_slave file returns a temperature???
> > > > And sysfs is 1 value-per-file, not multiple lines.
> > > 
> > > It was 'content crc' previously, and probably a good idea would
> > > be to
> > > add just one file with 'content'
> >  
> > That's the purpose of the new sysfs entry 'temperature'. It only
> > content temperature. As already mentionned we have to keep the
> > w1_slave
> > entry for compatibility purpose, all existing user application
> > parse
> > this file.
> 
> That's fine, but the document you wrote here says the file is called
> "w1_slave", not "temperature" :)
> 

Yes because it is the patch 2/5. At this stage, I just create the
sysfs-driver-w1_therm doc for the old w1_slave sysfs entry. The
temperature  sysfs entry is introduce in v3 patch 3/5 (and v4 patch
7/9)and the sysfs-driver-w1_therm is updated accordingly.

Akira Shimahara