_

[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@xxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxx>
> > +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@xxxxxxxxx>
> > +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