Re: [PATCH v3 2/5] w1_therm: adding sysfs entry to check device power
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" : > > > > > > > > > > > > > > +What: /sys/bus/w1/devices/.../w1_slave > > > > > +Date: Apr 2020 > > > > > +Contact: Akira Shimahara > > > > > +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
Re: [PATCH v3 2/5] w1_therm: adding sysfs entry to check device power
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" : > > > > > > > > > > +What: /sys/bus/w1/devices/.../w1_slave > > > > +Date: Apr 2020 > > > > +Contact: Akira Shimahara > > > > +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
Hello, Le mercredi 29 avril 2020 à 18:18 +0300, Evgeniy Polyakov a écrit : > Hi > > > > 29.04.2020, 16:47, "Greg KH" : > > > > > > +What: /sys/bus/w1/devices/.../w1_slave > > > +Date: Apr 2020 > > > +Contact: Akira Shimahara > > > +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
Hi 29.04.2020, 16:47, "Greg KH" : >> +What: /sys/bus/w1/devices/.../w1_slave >> +Date: Apr 2020 >> +Contact: Akira Shimahara >> +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
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
> ---
> .../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 000..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
> +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
> +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[] = {
> _attr_w1_slave.attr,
> + _attr_ext_power.attr,
> NULL,
> };
>
> static struct attribute *w1_ds28ea00_attrs[] = {
> _attr_w1_slave.attr,
> _attr_w1_seq.attr,
> + _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(>dev,
> + "%s: Device has been added, but power_mode may be
> corrupted. err=%d\n",
> +
[PATCH v3 2/5] w1_therm: adding sysfs entry to check device power
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
---
.../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 000..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
+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
+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[] = {
_attr_w1_slave.attr,
+ _attr_ext_power.attr,
NULL,
};
static struct attribute *w1_ds28ea00_attrs[] = {
_attr_w1_slave.attr,
_attr_w1_seq.attr,
+ _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(>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(_master->bus_mutex)) {
+ ret = -EAGAIN; // Didn't acquire the mutex
+ goto dec_refcnt;
+ }
+
+ while ((max_trying--) && (ret < 0)) {
+ /* safe version to select slave */
+ if
