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 <akira215c...@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 <akira215c...@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 <akira215c...@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