The following patches aim to make a user able to calibrate the current measurement of the DS2438. This chip uses a offset register in page1, which is added to the current register to give the user the current measurement. If this value is wrong, the user will get an offset current value, even if the current is zero, for instance. This patch gives support for reading the page1 registers (including the offset register) and for writing to the offset register. The DS2438 datasheet shows a calibration routine, and with this patch, the user can do this quickly by writing the correct value to the offset register. This patch was tested on real hardware using a power supply and an electronic load. Please help to review this series of patches. Best regards! Sampaio --- Changes in v5: - Merged all brackets coding style issue in one patch - Changing from BIN_ATTR to BIN_ATTR_RW in w1_ds2438.c - Wrapping email lines - Addind Documentation/ABI/ Changes in v4: - Fixing different patches with identical subject lines as requested Changes in v3: - I accidentally added a wrong line that would not compile. I'm sorry. Fixed it. Changes in v2: - Using git send-email to send the patches - Adding documentation as requested - Separating the coding style changes in different patches as requested Luiz Sampaio (9): w1: ds2438: fixed a coding style issue w1: ds2438: fixed a coding style issue w1: ds2438: fixed a coding style issue w1: ds2438: fixed a coding style issue w1: ds2438: fixed a coding style issue w1: ds2438: fixed a coding style issue w1: ds2438: fixing bug that would always get page0 w1: ds2438: adding support for reading page1 w1: ds2438: support for writing to offset register Documentation/w1/slaves/w1_ds2438.rst | 19 +++- drivers/w1/slaves/w1_ds2438.c | 122 ++++++++++++++++++++++---- 2 files changed, 124 insertions(+), 17 deletions(-) -- 2.30.1
There is an if statement and, if the function goes into it, it returns. So, the next else is not required. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- drivers/w1/slaves/w1_ds2438.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 5cfb0ae23e91..148921fb9702 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -154,11 +154,11 @@ static int w1_ds2438_change_config_bit(struct w1_slave *sl, u8 mask, u8 value) if ((status & mask) == value) return 0; /* already set as requested */ - else { - /* changing bit */ - status ^= mask; - perform_write = 1; - } + + /* changing bit */ + status ^= mask; + perform_write = 1; + break; } -- 2.30.1
Since there is only one statement inside the if clause, no brackets are required. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- drivers/w1/slaves/w1_ds2438.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 148921fb9702..56e53a748059 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -287,9 +287,9 @@ static ssize_t iad_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_current(sl, &voltage) == 0) { + if (w1_ds2438_get_current(sl, &voltage) == 0) ret = snprintf(buf, count, "%i\n", voltage); - } else + else ret = -EIO; return ret; @@ -338,9 +338,9 @@ static ssize_t temperature_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_temperature(sl, &temp) == 0) { + if (w1_ds2438_get_temperature(sl, &temp) == 0) ret = snprintf(buf, count, "%i\n", temp); - } else + else ret = -EIO; return ret; @@ -359,9 +359,9 @@ static ssize_t vad_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, &voltage) == 0) { + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, &voltage) == 0) ret = snprintf(buf, count, "%u\n", voltage); - } else + else ret = -EIO; return ret; @@ -380,9 +380,9 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, &voltage) == 0) { + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, &voltage) == 0) ret = snprintf(buf, count, "%u\n", voltage); - } else + else ret = -EIO; return ret; -- 2.30.1
Changed the permissions to preferred octal style. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- drivers/w1/slaves/w1_ds2438.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 56e53a748059..ccb06b8c2d78 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -388,7 +388,7 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, return ret; } -static BIN_ATTR(iad, S_IRUGO | S_IWUSR | S_IWGRP, iad_read, iad_write, 0); +static BIN_ATTR(iad, 0664, iad_write, 0); static BIN_ATTR_RO(page0, DS2438_PAGE_SIZE); static BIN_ATTR_RO(temperature, 0/* real length varies */); static BIN_ATTR_RO(vad, 0/* real length varies */); -- 2.30.1
The purpose of the w1_ds2438_get_page function is to get the register values at the page passed as the pageno parameter. However, the page0 was hardcoded, such that the function always returned the page0 contents. Fixed so that the function can retrieve any page. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- drivers/w1/slaves/w1_ds2438.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index ccb06b8c2d78..ef6217ecb1cb 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -62,13 +62,13 @@ static int w1_ds2438_get_page(struct w1_slave *sl, int pageno, u8 *buf) if (w1_reset_select_slave(sl)) continue; w1_buf[0] = W1_DS2438_RECALL_MEMORY; - w1_buf[1] = 0x00; + w1_buf[1] = (u8)pageno; w1_write_block(sl->master, w1_buf, 2); if (w1_reset_select_slave(sl)) continue; w1_buf[0] = W1_DS2438_READ_SCRATCH; - w1_buf[1] = 0x00; + w1_buf[1] = (u8)pageno; w1_write_block(sl->master, w1_buf, 2); count = w1_read_block(sl->master, buf, DS2438_PAGE_SIZE + 1); -- 2.30.1
Added a sysfs entry to support reading the page1 registers. This registers contain Elapsed Time Meter (ETM) data, which shows for how long the chip is on, as well as an Offset Register data, which can be used to calibrate the current measurement of the chip. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- .../ABI/stable/sysfs-driver-w1_ds2438 | 6 +++ Documentation/w1/slaves/w1_ds2438.rst | 8 ++++ drivers/w1/slaves/w1_ds2438.c | 41 +++++++++++++++++++ 3 files changed, 55 insertions(+) create mode 100644 Documentation/ABI/stable/sysfs-driver-w1_ds2438 diff --git a/Documentation/ABI/stable/sysfs-driver-w1_ds2438 b/Documentation/ABI/stable/sysfs-driver-w1_ds2438 new file mode 100644 index 000000000000..fa47437c11d9 --- /dev/null +++ b/Documentation/ABI/stable/sysfs-driver-w1_ds2438 @@ -0,0 +1,6 @@ +What: /sys/bus/w1/devices/.../page1 +Date: April 2021 +Contact: Luiz Sampaio <sampaio.ime@gmail.com> +Description: read the contents of the page1 of the DS2438 + see Documentation/w1/slaves/w1_ds2438.rst for detailed information +Users: any user space application which wants to communicate with DS2438 diff --git a/Documentation/w1/slaves/w1_ds2438.rst b/Documentation/w1/slaves/w1_ds2438.rst index a29309a3f8e5..ac8d0d4b0d0e 100644 --- a/Documentation/w1/slaves/w1_ds2438.rst +++ b/Documentation/w1/slaves/w1_ds2438.rst @@ -44,6 +44,14 @@ Internally when this file is read, the additional CRC byte is also obtained from the slave device. If it is correct, the 8 bytes page data are passed to userspace, otherwise an I/O error is returned. +"page1" +------- +This file provides full 8 bytes of the chip Page 1 (01h). +This page contains the ICA, elapsed time meter and current offset data of the DS2438. +Internally when this file is read, the additional CRC byte is also obtained +from the slave device. If it is correct, the 8 bytes page data are passed +to userspace, otherwise an I/O error is returned. + "temperature" ------------- Opening and reading this file initiates the CONVERT_T (temperature conversion) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index ef6217ecb1cb..2cfdfedb584f 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -49,6 +49,15 @@ #define DS2438_CURRENT_MSB 0x06 #define DS2438_THRESHOLD 0x07 +/* Page #1 definitions */ +#define DS2438_ETM_0 0x00 +#define DS2438_ETM_1 0x01 +#define DS2438_ETM_2 0x02 +#define DS2438_ETM_3 0x03 +#define DS2438_ICA 0x04 +#define DS2438_OFFSET_LSB 0x05 +#define DS2438_OFFSET_MSB 0x06 + static int w1_ds2438_get_page(struct w1_slave *sl, int pageno, u8 *buf) { unsigned int retries = W1_DS2438_RETRIES; @@ -325,6 +334,36 @@ static ssize_t page0_read(struct file *filp, struct kobject *kobj, return ret; } +static ssize_t page1_read(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, + loff_t off, size_t count) +{ + struct w1_slave *sl = kobj_to_w1_slave(kobj); + int ret; + u8 w1_buf[DS2438_PAGE_SIZE + 1 /*for CRC*/]; + + if (off != 0) + return 0; + if (!buf) + return -EINVAL; + + mutex_lock(&sl->master->bus_mutex); + + /* Read no more than page1 size */ + if (count > DS2438_PAGE_SIZE) + count = DS2438_PAGE_SIZE; + + if (w1_ds2438_get_page(sl, 1, w1_buf) == 0) { + memcpy(buf, &w1_buf, count); + ret = count; + } else + ret = -EIO; + + mutex_unlock(&sl->master->bus_mutex); + + return ret; +} + static ssize_t temperature_read(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count) @@ -390,6 +429,7 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, static BIN_ATTR(iad, 0664, iad_write, 0); static BIN_ATTR_RO(page0, DS2438_PAGE_SIZE); +static BIN_ATTR_RO(page1, DS2438_PAGE_SIZE); static BIN_ATTR_RO(temperature, 0/* real length varies */); static BIN_ATTR_RO(vad, 0/* real length varies */); static BIN_ATTR_RO(vdd, 0/* real length varies */); @@ -397,6 +437,7 @@ static BIN_ATTR_RO(vdd, 0/* real length varies */); static struct bin_attribute *w1_ds2438_bin_attrs[] = { &bin_attr_iad, &bin_attr_page0, + &bin_attr_page1, &bin_attr_temperature, &bin_attr_vad, &bin_attr_vdd, -- 2.30.1
Added a sysfs entry to support writing to the offset register on page1. This register is used to calibrate the chip canceling offset errors in the current ADC. This means that, over time, reading the IAD register will not return the correct current measurement, it will have an offset. Writing to the offset register if the two's complement of the current register while passing zero current to the load will calibrate the measurements. This change was tested on real hardware and it was able to calibrate the chip correctly. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- .../ABI/stable/sysfs-driver-w1_ds2438 | 7 +++ Documentation/w1/slaves/w1_ds2438.rst | 11 ++++- drivers/w1/slaves/w1_ds2438.c | 49 +++++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) diff --git a/Documentation/ABI/stable/sysfs-driver-w1_ds2438 b/Documentation/ABI/stable/sysfs-driver-w1_ds2438 index fa47437c11d9..d2e7681cc287 100644 --- a/Documentation/ABI/stable/sysfs-driver-w1_ds2438 +++ b/Documentation/ABI/stable/sysfs-driver-w1_ds2438 @@ -4,3 +4,10 @@ Contact: Luiz Sampaio <sampaio.ime@gmail.com> Description: read the contents of the page1 of the DS2438 see Documentation/w1/slaves/w1_ds2438.rst for detailed information Users: any user space application which wants to communicate with DS2438 + +What: /sys/bus/w1/devices/.../offset +Date: April 2021 +Contact: Luiz Sampaio <sampaio.ime@gmail.com> +Description: write the contents to the offset register of the DS2438 + see Documentation/w1/slaves/w1_ds2438.rst for detailed information +Users: any user space application which wants to communicate with DS2438 diff --git a/Documentation/w1/slaves/w1_ds2438.rst b/Documentation/w1/slaves/w1_ds2438.rst index ac8d0d4b0d0e..5c5573991351 100644 --- a/Documentation/w1/slaves/w1_ds2438.rst +++ b/Documentation/w1/slaves/w1_ds2438.rst @@ -22,7 +22,7 @@ is also often used in weather stations and applications such as: rain gauge, wind speed/direction measuring, humidity sensing, etc. Current support is provided through the following sysfs files (all files -except "iad" are readonly): +except "iad" and "offset" are readonly): "iad" ----- @@ -52,6 +52,15 @@ Internally when this file is read, the additional CRC byte is also obtained from the slave device. If it is correct, the 8 bytes page data are passed to userspace, otherwise an I/O error is returned. +"offset" +------- +This file controls the 2-byte Offset Register of the chip. +Writing a 2-byte value will change the Offset Register, which changes the +current measurement done by the chip. Changing this register to the two's complement +of the current register while forcing zero current through the load will calibrate +the chip, canceling offset errors in the current ADC. + + "temperature" ------------- Opening and reading this file initiates the CONVERT_T (temperature conversion) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 2cfdfedb584f..223a9aae6e2d 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -193,6 +193,34 @@ static int w1_ds2438_change_config_bit(struct w1_slave *sl, u8 mask, u8 value) return -1; } +static int w1_ds2438_change_offset_register(struct w1_slave *sl, u8 *value) +{ + unsigned int retries = W1_DS2438_RETRIES; + u8 w1_buf[9]; + u8 w1_page1_buf[DS2438_PAGE_SIZE + 1 /*for CRC*/]; + + if (w1_ds2438_get_page(sl, 1, w1_page1_buf) == 0) { + memcpy(&w1_buf[2], w1_page1_buf, DS2438_PAGE_SIZE - 1); /* last register reserved */ + w1_buf[7] = value[0]; /* change only offset register */ + w1_buf[8] = value[1]; + while (retries--) { + if (w1_reset_select_slave(sl)) + continue; + w1_buf[0] = W1_DS2438_WRITE_SCRATCH; + w1_buf[1] = 0x01; /* write to page 1 */ + w1_write_block(sl->master, w1_buf, 9); + + if (w1_reset_select_slave(sl)) + continue; + w1_buf[0] = W1_DS2438_COPY_SCRATCH; + w1_buf[1] = 0x01; + w1_write_block(sl->master, w1_buf, 2); + return 0; + } + } + return -1; +} + static int w1_ds2438_get_voltage(struct w1_slave *sl, int adc_input, uint16_t *voltage) { @@ -364,6 +392,25 @@ static ssize_t page1_read(struct file *filp, struct kobject *kobj, return ret; } +static ssize_t offset_write(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, + loff_t off, size_t count) +{ + struct w1_slave *sl = kobj_to_w1_slave(kobj); + int ret; + + mutex_lock(&sl->master->bus_mutex); + + if (w1_ds2438_change_offset_register(sl, buf) == 0) + ret = count; + else + ret = -EIO; + + mutex_unlock(&sl->master->bus_mutex); + + return ret; +} + static ssize_t temperature_read(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count) @@ -430,6 +477,7 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, static BIN_ATTR(iad, 0664, iad_write, 0); static BIN_ATTR_RO(page0, DS2438_PAGE_SIZE); static BIN_ATTR_RO(page1, DS2438_PAGE_SIZE); +static BIN_ATTR_WO(offset, 2); static BIN_ATTR_RO(temperature, 0/* real length varies */); static BIN_ATTR_RO(vad, 0/* real length varies */); static BIN_ATTR_RO(vdd, 0/* real length varies */); @@ -438,6 +486,7 @@ static struct bin_attribute *w1_ds2438_bin_attrs[] = { &bin_attr_iad, &bin_attr_page0, &bin_attr_page1, + &bin_attr_offset, &bin_attr_temperature, &bin_attr_vad, &bin_attr_vdd, -- 2.30.1
The following patches aim to make a user able to calibrate the current measurement of the DS2438. This chip uses a offset register in page1, which is added to the current register to give the user the current measurement. If this value is wrong, the user will get an offset current value, even if the current is zero, for instance. This patch gives support for reading the page1 registers (including the offset register) and for writing to the offset register. The DS2438 datasheet shows a calibration routine, and with this patch, the user can do this quickly by writing the correct value to the offset register. This patch was tested on real hardware using a power supply and an electronic load. Please help to review this series of patches. Best regards! Sampaio --- Changes in v6: - Actually changing from BIN_ATTR to BIN_ATTR_RW Changes in v5: - Merged all brackets coding style issue in one patch - Changing from BIN_ATTR to BIN_ATTR_RW in w1_ds2438.c - Wrapping email lines - Addind Documentation/ABI/ Changes in v4: - Fixing different patches with identical subject lines as requested Changes in v3: - I accidentally added a wrong line that would not compile. I'm sorry. Fixed it. Changes in v2: - Using git send-email to send the patches - Adding documentation as requested - Separating the coding style changes in different patches as requested Luiz Sampaio (9): w1: ds2438: fixed a coding style issue w1: ds2438: fixed a coding style issue w1: ds2438: fixed a coding style issue w1: ds2438: fixed a coding style issue w1: ds2438: fixed a coding style issue w1: ds2438: fixed a coding style issue w1: ds2438: fixing bug that would always get page0 w1: ds2438: adding support for reading page1 w1: ds2438: support for writing to offset register Documentation/w1/slaves/w1_ds2438.rst | 19 +++- drivers/w1/slaves/w1_ds2438.c | 122 ++++++++++++++++++++++---- 2 files changed, 124 insertions(+), 17 deletions(-) -- 2.30.1
There is an if statement and, if the function goes into it, it returns. So, the next else is not required. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- drivers/w1/slaves/w1_ds2438.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 5cfb0ae23e91..148921fb9702 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -154,11 +154,11 @@ static int w1_ds2438_change_config_bit(struct w1_slave *sl, u8 mask, u8 value) if ((status & mask) == value) return 0; /* already set as requested */ - else { - /* changing bit */ - status ^= mask; - perform_write = 1; - } + + /* changing bit */ + status ^= mask; + perform_write = 1; + break; } -- 2.30.1
Since there is only one statement inside the if clause, no brackets are required. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- drivers/w1/slaves/w1_ds2438.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 148921fb9702..56e53a748059 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -287,9 +287,9 @@ static ssize_t iad_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_current(sl, &voltage) == 0) { + if (w1_ds2438_get_current(sl, &voltage) == 0) ret = snprintf(buf, count, "%i\n", voltage); - } else + else ret = -EIO; return ret; @@ -338,9 +338,9 @@ static ssize_t temperature_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_temperature(sl, &temp) == 0) { + if (w1_ds2438_get_temperature(sl, &temp) == 0) ret = snprintf(buf, count, "%i\n", temp); - } else + else ret = -EIO; return ret; @@ -359,9 +359,9 @@ static ssize_t vad_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, &voltage) == 0) { + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, &voltage) == 0) ret = snprintf(buf, count, "%u\n", voltage); - } else + else ret = -EIO; return ret; @@ -380,9 +380,9 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, &voltage) == 0) { + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, &voltage) == 0) ret = snprintf(buf, count, "%u\n", voltage); - } else + else ret = -EIO; return ret; -- 2.30.1
Changed the permissions to preferred octal style. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- drivers/w1/slaves/w1_ds2438.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 56e53a748059..ccb06b8c2d78 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -388,7 +388,7 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, return ret; } -static BIN_ATTR(iad, S_IRUGO | S_IWUSR | S_IWGRP, iad_read, iad_write, 0); +static BIN_ATTR_RW(iad, 0664, iad_write, 0); static BIN_ATTR_RO(page0, DS2438_PAGE_SIZE); static BIN_ATTR_RO(temperature, 0/* real length varies */); static BIN_ATTR_RO(vad, 0/* real length varies */); -- 2.30.1
The purpose of the w1_ds2438_get_page function is to get the register values at the page passed as the pageno parameter. However, the page0 was hardcoded, such that the function always returned the page0 contents. Fixed so that the function can retrieve any page. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- drivers/w1/slaves/w1_ds2438.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index ccb06b8c2d78..ef6217ecb1cb 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -62,13 +62,13 @@ static int w1_ds2438_get_page(struct w1_slave *sl, int pageno, u8 *buf) if (w1_reset_select_slave(sl)) continue; w1_buf[0] = W1_DS2438_RECALL_MEMORY; - w1_buf[1] = 0x00; + w1_buf[1] = (u8)pageno; w1_write_block(sl->master, w1_buf, 2); if (w1_reset_select_slave(sl)) continue; w1_buf[0] = W1_DS2438_READ_SCRATCH; - w1_buf[1] = 0x00; + w1_buf[1] = (u8)pageno; w1_write_block(sl->master, w1_buf, 2); count = w1_read_block(sl->master, buf, DS2438_PAGE_SIZE + 1); -- 2.30.1
Added a sysfs entry to support reading the page1 registers. This registers contain Elapsed Time Meter (ETM) data, which shows for how long the chip is on, as well as an Offset Register data, which can be used to calibrate the current measurement of the chip. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- .../ABI/stable/sysfs-driver-w1_ds2438 | 6 +++ Documentation/w1/slaves/w1_ds2438.rst | 8 ++++ drivers/w1/slaves/w1_ds2438.c | 41 +++++++++++++++++++ 3 files changed, 55 insertions(+) create mode 100644 Documentation/ABI/stable/sysfs-driver-w1_ds2438 diff --git a/Documentation/ABI/stable/sysfs-driver-w1_ds2438 b/Documentation/ABI/stable/sysfs-driver-w1_ds2438 new file mode 100644 index 000000000000..fa47437c11d9 --- /dev/null +++ b/Documentation/ABI/stable/sysfs-driver-w1_ds2438 @@ -0,0 +1,6 @@ +What: /sys/bus/w1/devices/.../page1 +Date: April 2021 +Contact: Luiz Sampaio <sampaio.ime@gmail.com> +Description: read the contents of the page1 of the DS2438 + see Documentation/w1/slaves/w1_ds2438.rst for detailed information +Users: any user space application which wants to communicate with DS2438 diff --git a/Documentation/w1/slaves/w1_ds2438.rst b/Documentation/w1/slaves/w1_ds2438.rst index a29309a3f8e5..ac8d0d4b0d0e 100644 --- a/Documentation/w1/slaves/w1_ds2438.rst +++ b/Documentation/w1/slaves/w1_ds2438.rst @@ -44,6 +44,14 @@ Internally when this file is read, the additional CRC byte is also obtained from the slave device. If it is correct, the 8 bytes page data are passed to userspace, otherwise an I/O error is returned. +"page1" +------- +This file provides full 8 bytes of the chip Page 1 (01h). +This page contains the ICA, elapsed time meter and current offset data of the DS2438. +Internally when this file is read, the additional CRC byte is also obtained +from the slave device. If it is correct, the 8 bytes page data are passed +to userspace, otherwise an I/O error is returned. + "temperature" ------------- Opening and reading this file initiates the CONVERT_T (temperature conversion) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index ef6217ecb1cb..2cfdfedb584f 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -49,6 +49,15 @@ #define DS2438_CURRENT_MSB 0x06 #define DS2438_THRESHOLD 0x07 +/* Page #1 definitions */ +#define DS2438_ETM_0 0x00 +#define DS2438_ETM_1 0x01 +#define DS2438_ETM_2 0x02 +#define DS2438_ETM_3 0x03 +#define DS2438_ICA 0x04 +#define DS2438_OFFSET_LSB 0x05 +#define DS2438_OFFSET_MSB 0x06 + static int w1_ds2438_get_page(struct w1_slave *sl, int pageno, u8 *buf) { unsigned int retries = W1_DS2438_RETRIES; @@ -325,6 +334,36 @@ static ssize_t page0_read(struct file *filp, struct kobject *kobj, return ret; } +static ssize_t page1_read(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, + loff_t off, size_t count) +{ + struct w1_slave *sl = kobj_to_w1_slave(kobj); + int ret; + u8 w1_buf[DS2438_PAGE_SIZE + 1 /*for CRC*/]; + + if (off != 0) + return 0; + if (!buf) + return -EINVAL; + + mutex_lock(&sl->master->bus_mutex); + + /* Read no more than page1 size */ + if (count > DS2438_PAGE_SIZE) + count = DS2438_PAGE_SIZE; + + if (w1_ds2438_get_page(sl, 1, w1_buf) == 0) { + memcpy(buf, &w1_buf, count); + ret = count; + } else + ret = -EIO; + + mutex_unlock(&sl->master->bus_mutex); + + return ret; +} + static ssize_t temperature_read(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count) @@ -390,6 +429,7 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, static BIN_ATTR_RW(iad, 0664, iad_write, 0); static BIN_ATTR_RO(page0, DS2438_PAGE_SIZE); +static BIN_ATTR_RO(page1, DS2438_PAGE_SIZE); static BIN_ATTR_RO(temperature, 0/* real length varies */); static BIN_ATTR_RO(vad, 0/* real length varies */); static BIN_ATTR_RO(vdd, 0/* real length varies */); @@ -397,6 +437,7 @@ static BIN_ATTR_RO(vdd, 0/* real length varies */); static struct bin_attribute *w1_ds2438_bin_attrs[] = { &bin_attr_iad, &bin_attr_page0, + &bin_attr_page1, &bin_attr_temperature, &bin_attr_vad, &bin_attr_vdd, -- 2.30.1
Added a sysfs entry to support writing to the offset register on page1. This register is used to calibrate the chip canceling offset errors in the current ADC. This means that, over time, reading the IAD register will not return the correct current measurement, it will have an offset. Writing to the offset register if the two's complement of the current register while passing zero current to the load will calibrate the measurements. This change was tested on real hardware and it was able to calibrate the chip correctly. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- .../ABI/stable/sysfs-driver-w1_ds2438 | 7 +++ Documentation/w1/slaves/w1_ds2438.rst | 11 ++++- drivers/w1/slaves/w1_ds2438.c | 49 +++++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) diff --git a/Documentation/ABI/stable/sysfs-driver-w1_ds2438 b/Documentation/ABI/stable/sysfs-driver-w1_ds2438 index fa47437c11d9..d2e7681cc287 100644 --- a/Documentation/ABI/stable/sysfs-driver-w1_ds2438 +++ b/Documentation/ABI/stable/sysfs-driver-w1_ds2438 @@ -4,3 +4,10 @@ Contact: Luiz Sampaio <sampaio.ime@gmail.com> Description: read the contents of the page1 of the DS2438 see Documentation/w1/slaves/w1_ds2438.rst for detailed information Users: any user space application which wants to communicate with DS2438 + +What: /sys/bus/w1/devices/.../offset +Date: April 2021 +Contact: Luiz Sampaio <sampaio.ime@gmail.com> +Description: write the contents to the offset register of the DS2438 + see Documentation/w1/slaves/w1_ds2438.rst for detailed information +Users: any user space application which wants to communicate with DS2438 diff --git a/Documentation/w1/slaves/w1_ds2438.rst b/Documentation/w1/slaves/w1_ds2438.rst index ac8d0d4b0d0e..5c5573991351 100644 --- a/Documentation/w1/slaves/w1_ds2438.rst +++ b/Documentation/w1/slaves/w1_ds2438.rst @@ -22,7 +22,7 @@ is also often used in weather stations and applications such as: rain gauge, wind speed/direction measuring, humidity sensing, etc. Current support is provided through the following sysfs files (all files -except "iad" are readonly): +except "iad" and "offset" are readonly): "iad" ----- @@ -52,6 +52,15 @@ Internally when this file is read, the additional CRC byte is also obtained from the slave device. If it is correct, the 8 bytes page data are passed to userspace, otherwise an I/O error is returned. +"offset" +------- +This file controls the 2-byte Offset Register of the chip. +Writing a 2-byte value will change the Offset Register, which changes the +current measurement done by the chip. Changing this register to the two's complement +of the current register while forcing zero current through the load will calibrate +the chip, canceling offset errors in the current ADC. + + "temperature" ------------- Opening and reading this file initiates the CONVERT_T (temperature conversion) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 2cfdfedb584f..223a9aae6e2d 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -193,6 +193,34 @@ static int w1_ds2438_change_config_bit(struct w1_slave *sl, u8 mask, u8 value) return -1; } +static int w1_ds2438_change_offset_register(struct w1_slave *sl, u8 *value) +{ + unsigned int retries = W1_DS2438_RETRIES; + u8 w1_buf[9]; + u8 w1_page1_buf[DS2438_PAGE_SIZE + 1 /*for CRC*/]; + + if (w1_ds2438_get_page(sl, 1, w1_page1_buf) == 0) { + memcpy(&w1_buf[2], w1_page1_buf, DS2438_PAGE_SIZE - 1); /* last register reserved */ + w1_buf[7] = value[0]; /* change only offset register */ + w1_buf[8] = value[1]; + while (retries--) { + if (w1_reset_select_slave(sl)) + continue; + w1_buf[0] = W1_DS2438_WRITE_SCRATCH; + w1_buf[1] = 0x01; /* write to page 1 */ + w1_write_block(sl->master, w1_buf, 9); + + if (w1_reset_select_slave(sl)) + continue; + w1_buf[0] = W1_DS2438_COPY_SCRATCH; + w1_buf[1] = 0x01; + w1_write_block(sl->master, w1_buf, 2); + return 0; + } + } + return -1; +} + static int w1_ds2438_get_voltage(struct w1_slave *sl, int adc_input, uint16_t *voltage) { @@ -364,6 +392,25 @@ static ssize_t page1_read(struct file *filp, struct kobject *kobj, return ret; } +static ssize_t offset_write(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, + loff_t off, size_t count) +{ + struct w1_slave *sl = kobj_to_w1_slave(kobj); + int ret; + + mutex_lock(&sl->master->bus_mutex); + + if (w1_ds2438_change_offset_register(sl, buf) == 0) + ret = count; + else + ret = -EIO; + + mutex_unlock(&sl->master->bus_mutex); + + return ret; +} + static ssize_t temperature_read(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count) @@ -430,6 +477,7 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, static BIN_ATTR_RW(iad, 0664, iad_write, 0); static BIN_ATTR_RO(page0, DS2438_PAGE_SIZE); static BIN_ATTR_RO(page1, DS2438_PAGE_SIZE); +static BIN_ATTR_WO(offset, 2); static BIN_ATTR_RO(temperature, 0/* real length varies */); static BIN_ATTR_RO(vad, 0/* real length varies */); static BIN_ATTR_RO(vdd, 0/* real length varies */); @@ -438,6 +486,7 @@ static struct bin_attribute *w1_ds2438_bin_attrs[] = { &bin_attr_iad, &bin_attr_page0, &bin_attr_page1, + &bin_attr_offset, &bin_attr_temperature, &bin_attr_vad, &bin_attr_vdd, -- 2.30.1
On Fri, 2021-04-09 at 00:09 -0300, Luiz Sampaio wrote:
> Since there is only one statement inside the if clause, no brackets are
> required.
>
> Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com>
> ---
> drivers/w1/slaves/w1_ds2438.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c
> index 148921fb9702..56e53a748059 100644
> --- a/drivers/w1/slaves/w1_ds2438.c
> +++ b/drivers/w1/slaves/w1_ds2438.c
> @@ -287,9 +287,9 @@ static ssize_t iad_read(struct file *filp, struct kobject *kobj,
> if (!buf)
> return -EINVAL;
>
>
> - if (w1_ds2438_get_current(sl, &voltage) == 0) {
> + if (w1_ds2438_get_current(sl, &voltage) == 0)
> ret = snprintf(buf, count, "%i\n", voltage);
> - } else
> + else
> ret = -EIO;
>
>
> return ret;
> @@ -338,9 +338,9 @@ static ssize_t temperature_read(struct file *filp, struct kobject *kobj,
> if (!buf)
> return -EINVAL;
>
>
> - if (w1_ds2438_get_temperature(sl, &temp) == 0) {
> + if (w1_ds2438_get_temperature(sl, &temp) == 0)
> ret = snprintf(buf, count, "%i\n", temp);
> - } else
> + else
> ret = -EIO;
>
>
> return ret;
> @@ -359,9 +359,9 @@ static ssize_t vad_read(struct file *filp, struct kobject *kobj,
> if (!buf)
> return -EINVAL;
>
>
> - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, &voltage) == 0) {
> + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, &voltage) == 0)
> ret = snprintf(buf, count, "%u\n", voltage);
> - } else
> + else
> ret = -EIO;
>
>
> return ret;
> @@ -380,9 +380,9 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj,
> if (!buf)
> return -EINVAL;
>
>
> - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, &voltage) == 0) {
> + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, &voltage) == 0)
> ret = snprintf(buf, count, "%u\n", voltage);
> - } else
> + else
> ret = -EIO;
>
>
> return ret;
[-- Attachment #1: Type: text/plain, Size: 3065 bytes --] Hi Luiz, Thank you for the patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on v5.12-rc6 next-20210408] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Luiz-Sampaio/w1-ds2438-fixed-a-coding-style-issue/20210409-121608 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 17e7124aad766b3f158943acb51467f86220afe9 config: x86_64-randconfig-a004-20210409 (attached as .config) compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project dd453a1389b6a7e6d9214b449d3c54981b1a89b6) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install x86_64 cross compiling tool for clang build # apt-get install binutils-x86-64-linux-gnu # https://github.com/0day-ci/linux/commit/3ca70e59a342a9c6fd7db0bc937bbd0c80da9711 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Luiz-Sampaio/w1-ds2438-fixed-a-coding-style-issue/20210409-121608 git checkout 3ca70e59a342a9c6fd7db0bc937bbd0c80da9711 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): >> drivers/w1/slaves/w1_ds2438.c:391:31: error: too many arguments provided to function-like macro invocation static BIN_ATTR_RW(iad, 0664, iad_write, 0); ^ include/linux/sysfs.h:229:9: note: macro 'BIN_ATTR_RW' defined here #define BIN_ATTR_RW(_name, _size) \ ^ drivers/w1/slaves/w1_ds2438.c:398:3: error: use of undeclared identifier 'bin_attr_iad'; did you mean 'bin_attr_vad'? &bin_attr_iad, ^~~~~~~~~~~~ bin_attr_vad drivers/w1/slaves/w1_ds2438.c:394:8: note: 'bin_attr_vad' declared here static BIN_ATTR_RO(vad, 0/* real length varies */); ^ include/linux/sysfs.h:224:22: note: expanded from macro 'BIN_ATTR_RO' struct bin_attribute bin_attr_##_name = __BIN_ATTR_RO(_name, _size) ^ <scratch space>:113:1: note: expanded from here bin_attr_vad ^ 2 errors generated. vim +391 drivers/w1/slaves/w1_ds2438.c 390 > 391 static BIN_ATTR_RW(iad, 0664, iad_write, 0); 392 static BIN_ATTR_RO(page0, DS2438_PAGE_SIZE); 393 static BIN_ATTR_RO(temperature, 0/* real length varies */); 394 static BIN_ATTR_RO(vad, 0/* real length varies */); 395 static BIN_ATTR_RO(vdd, 0/* real length varies */); 396 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 34309 bytes --]
[-- Attachment #1: Type: text/plain, Size: 3713 bytes --] Hi Luiz, Thank you for the patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on v5.12-rc6 next-20210408] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Luiz-Sampaio/w1-ds2438-fixed-a-coding-style-issue/20210409-121608 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 17e7124aad766b3f158943acb51467f86220afe9 config: arm-randconfig-r032-20210409 (attached as .config) compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/3ca70e59a342a9c6fd7db0bc937bbd0c80da9711 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Luiz-Sampaio/w1-ds2438-fixed-a-coding-style-issue/20210409-121608 git checkout 3ca70e59a342a9c6fd7db0bc937bbd0c80da9711 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): >> drivers/w1/slaves/w1_ds2438.c:391:43: error: macro "BIN_ATTR_RW" passed 4 arguments, but takes just 2 391 | static BIN_ATTR_RW(iad, 0664, iad_write, 0); | ^ In file included from include/linux/kobject.h:20, from include/linux/module.h:20, from drivers/w1/slaves/w1_ds2438.c:9: include/linux/sysfs.h:229: note: macro "BIN_ATTR_RW" defined here 229 | #define BIN_ATTR_RW(_name, _size) \ | >> drivers/w1/slaves/w1_ds2438.c:391:8: error: type defaults to 'int' in declaration of 'BIN_ATTR_RW' [-Werror=implicit-int] 391 | static BIN_ATTR_RW(iad, 0664, iad_write, 0); | ^~~~~~~~~~~ drivers/w1/slaves/w1_ds2438.c:398:3: error: 'bin_attr_iad' undeclared here (not in a function); did you mean 'bin_attr_vad'? 398 | &bin_attr_iad, | ^~~~~~~~~~~~ | bin_attr_vad drivers/w1/slaves/w1_ds2438.c:391:8: warning: 'BIN_ATTR_RW' defined but not used [-Wunused-variable] 391 | static BIN_ATTR_RW(iad, 0664, iad_write, 0); | ^~~~~~~~~~~ drivers/w1/slaves/w1_ds2438.c:277:16: warning: 'iad_read' defined but not used [-Wunused-function] 277 | static ssize_t iad_read(struct file *filp, struct kobject *kobj, | ^~~~~~~~ drivers/w1/slaves/w1_ds2438.c:255:16: warning: 'iad_write' defined but not used [-Wunused-function] 255 | static ssize_t iad_write(struct file *filp, struct kobject *kobj, | ^~~~~~~~~ cc1: some warnings being treated as errors Kconfig warnings: (for reference only) WARNING: unmet direct dependencies detected for ADI_AXI_ADC Depends on IIO && HAS_IOMEM && OF Selected by - AD9467 && IIO && SPI vim +/BIN_ATTR_RW +391 drivers/w1/slaves/w1_ds2438.c 390 > 391 static BIN_ATTR_RW(iad, 0664, iad_write, 0); 392 static BIN_ATTR_RO(page0, DS2438_PAGE_SIZE); 393 static BIN_ATTR_RO(temperature, 0/* real length varies */); 394 static BIN_ATTR_RO(vad, 0/* real length varies */); 395 static BIN_ATTR_RO(vdd, 0/* real length varies */); 396 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 24566 bytes --]
On Fri, 2021-04-09 at 00:09 -0300, Luiz Sampaio wrote: > Since there is only one statement inside the if clause, no brackets are > required. [] > diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c [] > @@ -287,9 +287,9 @@ static ssize_t iad_read(struct file *filp, struct kobject *kobj, > if (!buf) > return -EINVAL; > > > - if (w1_ds2438_get_current(sl, &voltage) == 0) { > + if (w1_ds2438_get_current(sl, &voltage) == 0) > ret = snprintf(buf, count, "%i\n", voltage); > - } else > + else > ret = -EIO; > > > return ret; to me this would look better using a style like the below: (and it might be better using sysfs_emit and not snprintf too) --- drivers/w1/slaves/w1_ds2438.c | 36 ++++++++++++------------------------ 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 5cfb0ae23e91..9115c5a9bc4f 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -279,7 +279,6 @@ static ssize_t iad_read(struct file *filp, struct kobject *kobj, loff_t off, size_t count) { struct w1_slave *sl = kobj_to_w1_slave(kobj); - int ret; int16_t voltage; if (off != 0) @@ -287,12 +286,10 @@ static ssize_t iad_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_current(sl, &voltage) == 0) { - ret = snprintf(buf, count, "%i\n", voltage); - } else - ret = -EIO; + if (w1_ds2438_get_current(sl, &voltage)) + return -EIO; - return ret; + return snprintf(buf, count, "%i\n", voltage); } static ssize_t page0_read(struct file *filp, struct kobject *kobj, @@ -330,7 +327,6 @@ static ssize_t temperature_read(struct file *filp, struct kobject *kobj, loff_t off, size_t count) { struct w1_slave *sl = kobj_to_w1_slave(kobj); - int ret; int16_t temp; if (off != 0) @@ -338,12 +334,10 @@ static ssize_t temperature_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_temperature(sl, &temp) == 0) { - ret = snprintf(buf, count, "%i\n", temp); - } else - ret = -EIO; + if (w1_ds2438_get_temperature(sl, &temp)) + return -EIO; - return ret; + return snprintf(buf, count, "%i\n", temp); } static ssize_t vad_read(struct file *filp, struct kobject *kobj, @@ -351,7 +345,6 @@ static ssize_t vad_read(struct file *filp, struct kobject *kobj, loff_t off, size_t count) { struct w1_slave *sl = kobj_to_w1_slave(kobj); - int ret; uint16_t voltage; if (off != 0) @@ -359,12 +352,10 @@ static ssize_t vad_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, &voltage) == 0) { - ret = snprintf(buf, count, "%u\n", voltage); - } else - ret = -EIO; + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, &voltage)) + return -EIO; - return ret; + return snprintf(buf, count, "%u\n", voltage); } static ssize_t vdd_read(struct file *filp, struct kobject *kobj, @@ -372,7 +363,6 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, loff_t off, size_t count) { struct w1_slave *sl = kobj_to_w1_slave(kobj); - int ret; uint16_t voltage; if (off != 0) @@ -380,12 +370,10 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, &voltage) == 0) { - ret = snprintf(buf, count, "%u\n", voltage); - } else - ret = -EIO; + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, &voltage)) + return -EIO; - return ret; + return snprintf(buf, count, "%u\n", voltage); } static BIN_ATTR(iad, S_IRUGO | S_IWUSR | S_IWGRP, iad_read, iad_write, 0);
On Fri, Apr 09, 2021 at 12:15:30AM -0300, Luiz Sampaio wrote:
> Changed the permissions to preferred octal style.
>
> Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com>
> ---
> drivers/w1/slaves/w1_ds2438.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c
> index 56e53a748059..ccb06b8c2d78 100644
> --- a/drivers/w1/slaves/w1_ds2438.c
> +++ b/drivers/w1/slaves/w1_ds2438.c
> @@ -388,7 +388,7 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj,
> return ret;
> }
>
> -static BIN_ATTR(iad, S_IRUGO | S_IWUSR | S_IWGRP, iad_read, iad_write, 0);
> +static BIN_ATTR_RW(iad, 0664, iad_write, 0);
You obviously did not build this commit :(
And you did more than just fix a "coding style issue".
thanks,
greg k-h
The following patches aim to make a user able to calibrate the current measurement of the DS2438. This chip uses a offset register in page1, which is added to the current register to give the user the current measurement. If this value is wrong, the user will get an offset current value, even if the current is zero, for instance. This patch gives support for reading the page1 registers (including the offset register) and for writing to the offset register. The DS2438 datasheet shows a calibration routine, and with this patch, the user can do this quickly by writing the correct value to the offset register. This patch was tested on real hardware using a power supply and an electronic load. Please help to review this series of patches. Best regards! Sampaio --- Changes in v7: - Build test again Changes in v6: - Actually changing from BIN_ATTR to BIN_ATTR_RW --- Changes in v5: - Merged all brackets coding style issue in one patch - Changing from BIN_ATTR to BIN_ATTR_RW in w1_ds2438.c - Wrapping email lines - Addind Documentation/ABI/ Changes in v4: - Fixing different patches with identical subject lines as requested Changes in v3: - I accidentally added a wrong line that would not compile. I'm sorry. Fixed it. Changes in v2: - Using git send-email to send the patches - Adding documentation as requested - Separating the coding style changes in different patches as requested Luiz Sampaio (6): w1: ds2438: fixed a coding style issue w1: ds2438: fixed if brackets coding style issue w1: ds2438: changed sysfs macro for rw file w1: ds2438: fixing bug that would always get page0 w1: ds2438: adding support for reading page1 w1: ds2438: support for writing to offset register .../ABI/stable/sysfs-driver-w1_ds2438 | 13 ++ Documentation/w1/slaves/w1_ds2438.rst | 19 ++- drivers/w1/slaves/w1_ds2438.c | 122 +++++++++++++++--- 3 files changed, 137 insertions(+), 17 deletions(-) create mode 100644 Documentation/ABI/stable/sysfs-driver-w1_ds2438 -- 2.30.1
There is an if statement and, if the function goes into it, it returns. So, the next else is not required. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- drivers/w1/slaves/w1_ds2438.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 5cfb0ae23e91..148921fb9702 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -154,11 +154,11 @@ static int w1_ds2438_change_config_bit(struct w1_slave *sl, u8 mask, u8 value) if ((status & mask) == value) return 0; /* already set as requested */ - else { - /* changing bit */ - status ^= mask; - perform_write = 1; - } + + /* changing bit */ + status ^= mask; + perform_write = 1; + break; } -- 2.30.1
Since there is only one statement inside the if clause, no brackets are required. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- drivers/w1/slaves/w1_ds2438.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 148921fb9702..56e53a748059 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -287,9 +287,9 @@ static ssize_t iad_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_current(sl, &voltage) == 0) { + if (w1_ds2438_get_current(sl, &voltage) == 0) ret = snprintf(buf, count, "%i\n", voltage); - } else + else ret = -EIO; return ret; @@ -338,9 +338,9 @@ static ssize_t temperature_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_temperature(sl, &temp) == 0) { + if (w1_ds2438_get_temperature(sl, &temp) == 0) ret = snprintf(buf, count, "%i\n", temp); - } else + else ret = -EIO; return ret; @@ -359,9 +359,9 @@ static ssize_t vad_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, &voltage) == 0) { + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, &voltage) == 0) ret = snprintf(buf, count, "%u\n", voltage); - } else + else ret = -EIO; return ret; @@ -380,9 +380,9 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, &voltage) == 0) { + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, &voltage) == 0) ret = snprintf(buf, count, "%u\n", voltage); - } else + else ret = -EIO; return ret; -- 2.30.1
The iad sysfs file has permissions for read and write. Changed to the recommended macro BIN_ATTR_RW. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- drivers/w1/slaves/w1_ds2438.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 56e53a748059..910e25163898 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -388,7 +388,7 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, return ret; } -static BIN_ATTR(iad, S_IRUGO | S_IWUSR | S_IWGRP, iad_read, iad_write, 0); +static BIN_ATTR_RW(iad, 0); static BIN_ATTR_RO(page0, DS2438_PAGE_SIZE); static BIN_ATTR_RO(temperature, 0/* real length varies */); static BIN_ATTR_RO(vad, 0/* real length varies */); -- 2.30.1
The purpose of the w1_ds2438_get_page function is to get the register values at the page passed as the pageno parameter. However, the page0 was hardcoded, such that the function always returned the page0 contents. Fixed so that the function can retrieve any page. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- drivers/w1/slaves/w1_ds2438.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 910e25163898..1e95f3a256c7 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -62,13 +62,13 @@ static int w1_ds2438_get_page(struct w1_slave *sl, int pageno, u8 *buf) if (w1_reset_select_slave(sl)) continue; w1_buf[0] = W1_DS2438_RECALL_MEMORY; - w1_buf[1] = 0x00; + w1_buf[1] = (u8)pageno; w1_write_block(sl->master, w1_buf, 2); if (w1_reset_select_slave(sl)) continue; w1_buf[0] = W1_DS2438_READ_SCRATCH; - w1_buf[1] = 0x00; + w1_buf[1] = (u8)pageno; w1_write_block(sl->master, w1_buf, 2); count = w1_read_block(sl->master, buf, DS2438_PAGE_SIZE + 1); -- 2.30.1
Added a sysfs entry to support reading the page1 registers. This registers contain Elapsed Time Meter (ETM) data, which shows for how long the chip is on, as well as an Offset Register data, which can be used to calibrate the current measurement of the chip. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- .../ABI/stable/sysfs-driver-w1_ds2438 | 6 +++ Documentation/w1/slaves/w1_ds2438.rst | 8 ++++ drivers/w1/slaves/w1_ds2438.c | 41 +++++++++++++++++++ 3 files changed, 55 insertions(+) create mode 100644 Documentation/ABI/stable/sysfs-driver-w1_ds2438 diff --git a/Documentation/ABI/stable/sysfs-driver-w1_ds2438 b/Documentation/ABI/stable/sysfs-driver-w1_ds2438 new file mode 100644 index 000000000000..fa47437c11d9 --- /dev/null +++ b/Documentation/ABI/stable/sysfs-driver-w1_ds2438 @@ -0,0 +1,6 @@ +What: /sys/bus/w1/devices/.../page1 +Date: April 2021 +Contact: Luiz Sampaio <sampaio.ime@gmail.com> +Description: read the contents of the page1 of the DS2438 + see Documentation/w1/slaves/w1_ds2438.rst for detailed information +Users: any user space application which wants to communicate with DS2438 diff --git a/Documentation/w1/slaves/w1_ds2438.rst b/Documentation/w1/slaves/w1_ds2438.rst index a29309a3f8e5..ac8d0d4b0d0e 100644 --- a/Documentation/w1/slaves/w1_ds2438.rst +++ b/Documentation/w1/slaves/w1_ds2438.rst @@ -44,6 +44,14 @@ Internally when this file is read, the additional CRC byte is also obtained from the slave device. If it is correct, the 8 bytes page data are passed to userspace, otherwise an I/O error is returned. +"page1" +------- +This file provides full 8 bytes of the chip Page 1 (01h). +This page contains the ICA, elapsed time meter and current offset data of the DS2438. +Internally when this file is read, the additional CRC byte is also obtained +from the slave device. If it is correct, the 8 bytes page data are passed +to userspace, otherwise an I/O error is returned. + "temperature" ------------- Opening and reading this file initiates the CONVERT_T (temperature conversion) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 1e95f3a256c7..42080ae779f0 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -49,6 +49,15 @@ #define DS2438_CURRENT_MSB 0x06 #define DS2438_THRESHOLD 0x07 +/* Page #1 definitions */ +#define DS2438_ETM_0 0x00 +#define DS2438_ETM_1 0x01 +#define DS2438_ETM_2 0x02 +#define DS2438_ETM_3 0x03 +#define DS2438_ICA 0x04 +#define DS2438_OFFSET_LSB 0x05 +#define DS2438_OFFSET_MSB 0x06 + static int w1_ds2438_get_page(struct w1_slave *sl, int pageno, u8 *buf) { unsigned int retries = W1_DS2438_RETRIES; @@ -325,6 +334,36 @@ static ssize_t page0_read(struct file *filp, struct kobject *kobj, return ret; } +static ssize_t page1_read(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, + loff_t off, size_t count) +{ + struct w1_slave *sl = kobj_to_w1_slave(kobj); + int ret; + u8 w1_buf[DS2438_PAGE_SIZE + 1 /*for CRC*/]; + + if (off != 0) + return 0; + if (!buf) + return -EINVAL; + + mutex_lock(&sl->master->bus_mutex); + + /* Read no more than page1 size */ + if (count > DS2438_PAGE_SIZE) + count = DS2438_PAGE_SIZE; + + if (w1_ds2438_get_page(sl, 1, w1_buf) == 0) { + memcpy(buf, &w1_buf, count); + ret = count; + } else + ret = -EIO; + + mutex_unlock(&sl->master->bus_mutex); + + return ret; +} + static ssize_t temperature_read(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count) @@ -390,6 +429,7 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, static BIN_ATTR_RW(iad, 0); static BIN_ATTR_RO(page0, DS2438_PAGE_SIZE); +static BIN_ATTR_RO(page1, DS2438_PAGE_SIZE); static BIN_ATTR_RO(temperature, 0/* real length varies */); static BIN_ATTR_RO(vad, 0/* real length varies */); static BIN_ATTR_RO(vdd, 0/* real length varies */); @@ -397,6 +437,7 @@ static BIN_ATTR_RO(vdd, 0/* real length varies */); static struct bin_attribute *w1_ds2438_bin_attrs[] = { &bin_attr_iad, &bin_attr_page0, + &bin_attr_page1, &bin_attr_temperature, &bin_attr_vad, &bin_attr_vdd, -- 2.30.1
Added a sysfs entry to support writing to the offset register on page1. This register is used to calibrate the chip canceling offset errors in the current ADC. This means that, over time, reading the IAD register will not return the correct current measurement, it will have an offset. Writing to the offset register if the two's complement of the current register while passing zero current to the load will calibrate the measurements. This change was tested on real hardware and it was able to calibrate the chip correctly. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- .../ABI/stable/sysfs-driver-w1_ds2438 | 7 +++ Documentation/w1/slaves/w1_ds2438.rst | 11 ++++- drivers/w1/slaves/w1_ds2438.c | 49 +++++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) diff --git a/Documentation/ABI/stable/sysfs-driver-w1_ds2438 b/Documentation/ABI/stable/sysfs-driver-w1_ds2438 index fa47437c11d9..d2e7681cc287 100644 --- a/Documentation/ABI/stable/sysfs-driver-w1_ds2438 +++ b/Documentation/ABI/stable/sysfs-driver-w1_ds2438 @@ -4,3 +4,10 @@ Contact: Luiz Sampaio <sampaio.ime@gmail.com> Description: read the contents of the page1 of the DS2438 see Documentation/w1/slaves/w1_ds2438.rst for detailed information Users: any user space application which wants to communicate with DS2438 + +What: /sys/bus/w1/devices/.../offset +Date: April 2021 +Contact: Luiz Sampaio <sampaio.ime@gmail.com> +Description: write the contents to the offset register of the DS2438 + see Documentation/w1/slaves/w1_ds2438.rst for detailed information +Users: any user space application which wants to communicate with DS2438 diff --git a/Documentation/w1/slaves/w1_ds2438.rst b/Documentation/w1/slaves/w1_ds2438.rst index ac8d0d4b0d0e..5c5573991351 100644 --- a/Documentation/w1/slaves/w1_ds2438.rst +++ b/Documentation/w1/slaves/w1_ds2438.rst @@ -22,7 +22,7 @@ is also often used in weather stations and applications such as: rain gauge, wind speed/direction measuring, humidity sensing, etc. Current support is provided through the following sysfs files (all files -except "iad" are readonly): +except "iad" and "offset" are readonly): "iad" ----- @@ -52,6 +52,15 @@ Internally when this file is read, the additional CRC byte is also obtained from the slave device. If it is correct, the 8 bytes page data are passed to userspace, otherwise an I/O error is returned. +"offset" +------- +This file controls the 2-byte Offset Register of the chip. +Writing a 2-byte value will change the Offset Register, which changes the +current measurement done by the chip. Changing this register to the two's complement +of the current register while forcing zero current through the load will calibrate +the chip, canceling offset errors in the current ADC. + + "temperature" ------------- Opening and reading this file initiates the CONVERT_T (temperature conversion) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 42080ae779f0..9c39bd6f5fcc 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -193,6 +193,34 @@ static int w1_ds2438_change_config_bit(struct w1_slave *sl, u8 mask, u8 value) return -1; } +static int w1_ds2438_change_offset_register(struct w1_slave *sl, u8 *value) +{ + unsigned int retries = W1_DS2438_RETRIES; + u8 w1_buf[9]; + u8 w1_page1_buf[DS2438_PAGE_SIZE + 1 /*for CRC*/]; + + if (w1_ds2438_get_page(sl, 1, w1_page1_buf) == 0) { + memcpy(&w1_buf[2], w1_page1_buf, DS2438_PAGE_SIZE - 1); /* last register reserved */ + w1_buf[7] = value[0]; /* change only offset register */ + w1_buf[8] = value[1]; + while (retries--) { + if (w1_reset_select_slave(sl)) + continue; + w1_buf[0] = W1_DS2438_WRITE_SCRATCH; + w1_buf[1] = 0x01; /* write to page 1 */ + w1_write_block(sl->master, w1_buf, 9); + + if (w1_reset_select_slave(sl)) + continue; + w1_buf[0] = W1_DS2438_COPY_SCRATCH; + w1_buf[1] = 0x01; + w1_write_block(sl->master, w1_buf, 2); + return 0; + } + } + return -1; +} + static int w1_ds2438_get_voltage(struct w1_slave *sl, int adc_input, uint16_t *voltage) { @@ -364,6 +392,25 @@ static ssize_t page1_read(struct file *filp, struct kobject *kobj, return ret; } +static ssize_t offset_write(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, + loff_t off, size_t count) +{ + struct w1_slave *sl = kobj_to_w1_slave(kobj); + int ret; + + mutex_lock(&sl->master->bus_mutex); + + if (w1_ds2438_change_offset_register(sl, buf) == 0) + ret = count; + else + ret = -EIO; + + mutex_unlock(&sl->master->bus_mutex); + + return ret; +} + static ssize_t temperature_read(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count) @@ -430,6 +477,7 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, static BIN_ATTR_RW(iad, 0); static BIN_ATTR_RO(page0, DS2438_PAGE_SIZE); static BIN_ATTR_RO(page1, DS2438_PAGE_SIZE); +static BIN_ATTR_WO(offset, 2); static BIN_ATTR_RO(temperature, 0/* real length varies */); static BIN_ATTR_RO(vad, 0/* real length varies */); static BIN_ATTR_RO(vdd, 0/* real length varies */); @@ -438,6 +486,7 @@ static struct bin_attribute *w1_ds2438_bin_attrs[] = { &bin_attr_iad, &bin_attr_page0, &bin_attr_page1, + &bin_attr_offset, &bin_attr_temperature, &bin_attr_vad, &bin_attr_vdd, -- 2.30.1
On Fri, Apr 09, 2021 at 07:40:57AM -0700, Joe Perches wrote:
> On Fri, 2021-04-09 at 00:09 -0300, Luiz Sampaio wrote:
> > Since there is only one statement inside the if clause, no brackets are
> > required.
> []
> > diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c
> []
> > @@ -287,9 +287,9 @@ static ssize_t iad_read(struct file *filp, struct kobject *kobj,
> > if (!buf)
> > return -EINVAL;
> >
> >
> > - if (w1_ds2438_get_current(sl, &voltage) == 0) {
> > + if (w1_ds2438_get_current(sl, &voltage) == 0)
> > ret = snprintf(buf, count, "%i\n", voltage);
> > - } else
> > + else
> > ret = -EIO;
> >
> >
> > return ret;
>
> to me this would look better using a style like the below:
> (and it might be better using sysfs_emit and not snprintf too)
>
> ---
> drivers/w1/slaves/w1_ds2438.c | 36 ++++++++++++------------------------
> 1 file changed, 12 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c
> index 5cfb0ae23e91..9115c5a9bc4f 100644
> --- a/drivers/w1/slaves/w1_ds2438.c
> +++ b/drivers/w1/slaves/w1_ds2438.c
> @@ -279,7 +279,6 @@ static ssize_t iad_read(struct file *filp, struct kobject *kobj,
> loff_t off, size_t count)
> {
> struct w1_slave *sl = kobj_to_w1_slave(kobj);
> - int ret;
> int16_t voltage;
>
> if (off != 0)
> @@ -287,12 +286,10 @@ static ssize_t iad_read(struct file *filp, struct kobject *kobj,
> if (!buf)
> return -EINVAL;
>
> - if (w1_ds2438_get_current(sl, &voltage) == 0) {
> - ret = snprintf(buf, count, "%i\n", voltage);
> - } else
> - ret = -EIO;
> + if (w1_ds2438_get_current(sl, &voltage))
> + return -EIO;
>
> - return ret;
> + return snprintf(buf, count, "%i\n", voltage);
> }
>
> static ssize_t page0_read(struct file *filp, struct kobject *kobj,
> @@ -330,7 +327,6 @@ static ssize_t temperature_read(struct file *filp, struct kobject *kobj,
> loff_t off, size_t count)
> {
> struct w1_slave *sl = kobj_to_w1_slave(kobj);
> - int ret;
> int16_t temp;
>
> if (off != 0)
> @@ -338,12 +334,10 @@ static ssize_t temperature_read(struct file *filp, struct kobject *kobj,
> if (!buf)
> return -EINVAL;
>
> - if (w1_ds2438_get_temperature(sl, &temp) == 0) {
> - ret = snprintf(buf, count, "%i\n", temp);
> - } else
> - ret = -EIO;
> + if (w1_ds2438_get_temperature(sl, &temp))
> + return -EIO;
>
> - return ret;
> + return snprintf(buf, count, "%i\n", temp);
> }
>
> static ssize_t vad_read(struct file *filp, struct kobject *kobj,
> @@ -351,7 +345,6 @@ static ssize_t vad_read(struct file *filp, struct kobject *kobj,
> loff_t off, size_t count)
> {
> struct w1_slave *sl = kobj_to_w1_slave(kobj);
> - int ret;
> uint16_t voltage;
>
> if (off != 0)
> @@ -359,12 +352,10 @@ static ssize_t vad_read(struct file *filp, struct kobject *kobj,
> if (!buf)
> return -EINVAL;
>
> - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, &voltage) == 0) {
> - ret = snprintf(buf, count, "%u\n", voltage);
> - } else
> - ret = -EIO;
> + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, &voltage))
> + return -EIO;
>
> - return ret;
> + return snprintf(buf, count, "%u\n", voltage);
> }
>
> static ssize_t vdd_read(struct file *filp, struct kobject *kobj,
> @@ -372,7 +363,6 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj,
> loff_t off, size_t count)
> {
> struct w1_slave *sl = kobj_to_w1_slave(kobj);
> - int ret;
> uint16_t voltage;
>
> if (off != 0)
> @@ -380,12 +370,10 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj,
> if (!buf)
> return -EINVAL;
>
> - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, &voltage) == 0) {
> - ret = snprintf(buf, count, "%u\n", voltage);
> - } else
> - ret = -EIO;
> + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, &voltage))
> + return -EIO;
>
> - return ret;
> + return snprintf(buf, count, "%u\n", voltage);
> }
>
> static BIN_ATTR(iad, S_IRUGO | S_IWUSR | S_IWGRP, iad_read, iad_write, 0);
>
Sorry for the late reply! I agree, this would look nicer. I will wait for
the current revision and change this for the next one.
[-- Attachment #1: Type: text/plain, Size: 2054 bytes --] Hi Luiz, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linus/master] [also build test WARNING on v5.12-rc7 next-20210416] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Luiz-Sampaio/w1-ds2438-fixed-a-coding-style-issue/20210417-071754 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 151501160401e2dc669ea7dac2c599b53f220c33 config: csky-randconfig-m031-20210416 (attached as .config) compiler: csky-linux-gcc (GCC) 9.3.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> smatch warnings: drivers/w1/slaves/w1_ds2438.c:218 w1_ds2438_change_offset_register() warn: inconsistent indenting vim +218 drivers/w1/slaves/w1_ds2438.c 195 196 static int w1_ds2438_change_offset_register(struct w1_slave *sl, u8 *value) 197 { 198 unsigned int retries = W1_DS2438_RETRIES; 199 u8 w1_buf[9]; 200 u8 w1_page1_buf[DS2438_PAGE_SIZE + 1 /*for CRC*/]; 201 202 if (w1_ds2438_get_page(sl, 1, w1_page1_buf) == 0) { 203 memcpy(&w1_buf[2], w1_page1_buf, DS2438_PAGE_SIZE - 1); /* last register reserved */ 204 w1_buf[7] = value[0]; /* change only offset register */ 205 w1_buf[8] = value[1]; 206 while (retries--) { 207 if (w1_reset_select_slave(sl)) 208 continue; 209 w1_buf[0] = W1_DS2438_WRITE_SCRATCH; 210 w1_buf[1] = 0x01; /* write to page 1 */ 211 w1_write_block(sl->master, w1_buf, 9); 212 213 if (w1_reset_select_slave(sl)) 214 continue; 215 w1_buf[0] = W1_DS2438_COPY_SCRATCH; 216 w1_buf[1] = 0x01; 217 w1_write_block(sl->master, w1_buf, 2); > 218 return 0; 219 } 220 } 221 return -1; 222 } 223 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 24550 bytes --]
On Fri, Apr 16, 2021 at 07:17:33PM -0300, Luiz Sampaio wrote:
> The following patches aim to make a user able to calibrate the current
> measurement of the DS2438. This chip uses a offset register in page1, which
> is added to the current register to give the user the current measurement.
> If this value is wrong, the user will get an offset current value, even if
> the current is zero, for instance. This patch gives support for reading the
> page1 registers (including the offset register) and for writing to the
> offset register. The DS2438 datasheet shows a calibration routine, and with
> this patch, the user can do this quickly by writing the correct value to
> the offset register. This patch was tested on real hardware using a power
> supply and an electronic load.
> Please help to review this series of patches.
>
> Best regards!
> Sampaio
> ---
> Changes in v7:
> - Build test again
Can you fix the warning in patch 6/6 that the kernel test robot found,
and resend?
thanks,
greg k-h
The following patches aim to make a user able to calibrate the current measurement of the DS2438. This chip uses a offset register in page1, which is added to the current register to give the user the current measurement. If this value is wrong, the user will get an offset current value, even if the current is zero, for instance. This patch gives support for reading the page1 registers (including the offset register) and for writing to the offset register. The DS2438 datasheet shows a calibration routine, and with this patch, the user can do this quickly by writing the correct value to the offset register. This patch was tested on real hardware using a power supply and an electronic load. Please help to review this series of patches. Best regards! Sampaio --- Changes in v8: - Fixing bot identation warning Changes in v7: - Build test again Changes in v6: - Actually changing from BIN_ATTR to BIN_ATTR_RW --- Changes in v5: - Merged all brackets coding style issue in one patch - Changing from BIN_ATTR to BIN_ATTR_RW in w1_ds2438.c - Wrapping email lines - Addind Documentation/ABI/ Changes in v4: - Fixing different patches with identical subject lines as requested Changes in v3: - I accidentally added a wrong line that would not compile. I'm sorry. Fixed it. Changes in v2: - Using git send-email to send the patches - Adding documentation as requested - Separating the coding style changes in different patches as requested Luiz Sampaio (6): w1: ds2438: fixed a coding style issue w1: ds2438: fixed if brackets coding style issue w1: ds2438: changed sysfs macro for rw file w1: ds2438: fixing bug that would always get page0 w1: ds2438: adding support for reading page1 w1: ds2438: support for writing to offset register .../ABI/stable/sysfs-driver-w1_ds2438 | 13 ++ Documentation/w1/slaves/w1_ds2438.rst | 19 ++- drivers/w1/slaves/w1_ds2438.c | 122 +++++++++++++++--- 3 files changed, 137 insertions(+), 17 deletions(-) create mode 100644 Documentation/ABI/stable/sysfs-driver-w1_ds2438 -- 2.30.1
There is an if statement and, if the function goes into it, it returns. So, the next else is not required. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- drivers/w1/slaves/w1_ds2438.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 5cfb0ae23e91..148921fb9702 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -154,11 +154,11 @@ static int w1_ds2438_change_config_bit(struct w1_slave *sl, u8 mask, u8 value) if ((status & mask) == value) return 0; /* already set as requested */ - else { - /* changing bit */ - status ^= mask; - perform_write = 1; - } + + /* changing bit */ + status ^= mask; + perform_write = 1; + break; } -- 2.30.1
Since there is only one statement inside the if clause, no brackets are required. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- drivers/w1/slaves/w1_ds2438.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 148921fb9702..56e53a748059 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -287,9 +287,9 @@ static ssize_t iad_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_current(sl, &voltage) == 0) { + if (w1_ds2438_get_current(sl, &voltage) == 0) ret = snprintf(buf, count, "%i\n", voltage); - } else + else ret = -EIO; return ret; @@ -338,9 +338,9 @@ static ssize_t temperature_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_temperature(sl, &temp) == 0) { + if (w1_ds2438_get_temperature(sl, &temp) == 0) ret = snprintf(buf, count, "%i\n", temp); - } else + else ret = -EIO; return ret; @@ -359,9 +359,9 @@ static ssize_t vad_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, &voltage) == 0) { + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, &voltage) == 0) ret = snprintf(buf, count, "%u\n", voltage); - } else + else ret = -EIO; return ret; @@ -380,9 +380,9 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, if (!buf) return -EINVAL; - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, &voltage) == 0) { + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, &voltage) == 0) ret = snprintf(buf, count, "%u\n", voltage); - } else + else ret = -EIO; return ret; -- 2.30.1
The iad sysfs file has permissions for read and write. Changed to the recommended macro BIN_ATTR_RW. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- drivers/w1/slaves/w1_ds2438.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 56e53a748059..910e25163898 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -388,7 +388,7 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, return ret; } -static BIN_ATTR(iad, S_IRUGO | S_IWUSR | S_IWGRP, iad_read, iad_write, 0); +static BIN_ATTR_RW(iad, 0); static BIN_ATTR_RO(page0, DS2438_PAGE_SIZE); static BIN_ATTR_RO(temperature, 0/* real length varies */); static BIN_ATTR_RO(vad, 0/* real length varies */); -- 2.30.1
The purpose of the w1_ds2438_get_page function is to get the register values at the page passed as the pageno parameter. However, the page0 was hardcoded, such that the function always returned the page0 contents. Fixed so that the function can retrieve any page. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- drivers/w1/slaves/w1_ds2438.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 910e25163898..1e95f3a256c7 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -62,13 +62,13 @@ static int w1_ds2438_get_page(struct w1_slave *sl, int pageno, u8 *buf) if (w1_reset_select_slave(sl)) continue; w1_buf[0] = W1_DS2438_RECALL_MEMORY; - w1_buf[1] = 0x00; + w1_buf[1] = (u8)pageno; w1_write_block(sl->master, w1_buf, 2); if (w1_reset_select_slave(sl)) continue; w1_buf[0] = W1_DS2438_READ_SCRATCH; - w1_buf[1] = 0x00; + w1_buf[1] = (u8)pageno; w1_write_block(sl->master, w1_buf, 2); count = w1_read_block(sl->master, buf, DS2438_PAGE_SIZE + 1); -- 2.30.1
Added a sysfs entry to support reading the page1 registers. This registers contain Elapsed Time Meter (ETM) data, which shows for how long the chip is on, as well as an Offset Register data, which can be used to calibrate the current measurement of the chip. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- .../ABI/stable/sysfs-driver-w1_ds2438 | 6 +++ Documentation/w1/slaves/w1_ds2438.rst | 8 ++++ drivers/w1/slaves/w1_ds2438.c | 41 +++++++++++++++++++ 3 files changed, 55 insertions(+) create mode 100644 Documentation/ABI/stable/sysfs-driver-w1_ds2438 diff --git a/Documentation/ABI/stable/sysfs-driver-w1_ds2438 b/Documentation/ABI/stable/sysfs-driver-w1_ds2438 new file mode 100644 index 000000000000..fa47437c11d9 --- /dev/null +++ b/Documentation/ABI/stable/sysfs-driver-w1_ds2438 @@ -0,0 +1,6 @@ +What: /sys/bus/w1/devices/.../page1 +Date: April 2021 +Contact: Luiz Sampaio <sampaio.ime@gmail.com> +Description: read the contents of the page1 of the DS2438 + see Documentation/w1/slaves/w1_ds2438.rst for detailed information +Users: any user space application which wants to communicate with DS2438 diff --git a/Documentation/w1/slaves/w1_ds2438.rst b/Documentation/w1/slaves/w1_ds2438.rst index a29309a3f8e5..ac8d0d4b0d0e 100644 --- a/Documentation/w1/slaves/w1_ds2438.rst +++ b/Documentation/w1/slaves/w1_ds2438.rst @@ -44,6 +44,14 @@ Internally when this file is read, the additional CRC byte is also obtained from the slave device. If it is correct, the 8 bytes page data are passed to userspace, otherwise an I/O error is returned. +"page1" +------- +This file provides full 8 bytes of the chip Page 1 (01h). +This page contains the ICA, elapsed time meter and current offset data of the DS2438. +Internally when this file is read, the additional CRC byte is also obtained +from the slave device. If it is correct, the 8 bytes page data are passed +to userspace, otherwise an I/O error is returned. + "temperature" ------------- Opening and reading this file initiates the CONVERT_T (temperature conversion) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 1e95f3a256c7..42080ae779f0 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -49,6 +49,15 @@ #define DS2438_CURRENT_MSB 0x06 #define DS2438_THRESHOLD 0x07 +/* Page #1 definitions */ +#define DS2438_ETM_0 0x00 +#define DS2438_ETM_1 0x01 +#define DS2438_ETM_2 0x02 +#define DS2438_ETM_3 0x03 +#define DS2438_ICA 0x04 +#define DS2438_OFFSET_LSB 0x05 +#define DS2438_OFFSET_MSB 0x06 + static int w1_ds2438_get_page(struct w1_slave *sl, int pageno, u8 *buf) { unsigned int retries = W1_DS2438_RETRIES; @@ -325,6 +334,36 @@ static ssize_t page0_read(struct file *filp, struct kobject *kobj, return ret; } +static ssize_t page1_read(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, + loff_t off, size_t count) +{ + struct w1_slave *sl = kobj_to_w1_slave(kobj); + int ret; + u8 w1_buf[DS2438_PAGE_SIZE + 1 /*for CRC*/]; + + if (off != 0) + return 0; + if (!buf) + return -EINVAL; + + mutex_lock(&sl->master->bus_mutex); + + /* Read no more than page1 size */ + if (count > DS2438_PAGE_SIZE) + count = DS2438_PAGE_SIZE; + + if (w1_ds2438_get_page(sl, 1, w1_buf) == 0) { + memcpy(buf, &w1_buf, count); + ret = count; + } else + ret = -EIO; + + mutex_unlock(&sl->master->bus_mutex); + + return ret; +} + static ssize_t temperature_read(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count) @@ -390,6 +429,7 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, static BIN_ATTR_RW(iad, 0); static BIN_ATTR_RO(page0, DS2438_PAGE_SIZE); +static BIN_ATTR_RO(page1, DS2438_PAGE_SIZE); static BIN_ATTR_RO(temperature, 0/* real length varies */); static BIN_ATTR_RO(vad, 0/* real length varies */); static BIN_ATTR_RO(vdd, 0/* real length varies */); @@ -397,6 +437,7 @@ static BIN_ATTR_RO(vdd, 0/* real length varies */); static struct bin_attribute *w1_ds2438_bin_attrs[] = { &bin_attr_iad, &bin_attr_page0, + &bin_attr_page1, &bin_attr_temperature, &bin_attr_vad, &bin_attr_vdd, -- 2.30.1
Added a sysfs entry to support writing to the offset register on page1. This register is used to calibrate the chip canceling offset errors in the current ADC. This means that, over time, reading the IAD register will not return the correct current measurement, it will have an offset. Writing to the offset register if the two's complement of the current register while passing zero current to the load will calibrate the measurements. This change was tested on real hardware and it was able to calibrate the chip correctly. Signed-off-by: Luiz Sampaio <sampaio.ime@gmail.com> --- .../ABI/stable/sysfs-driver-w1_ds2438 | 7 +++ Documentation/w1/slaves/w1_ds2438.rst | 11 ++++- drivers/w1/slaves/w1_ds2438.c | 49 +++++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) diff --git a/Documentation/ABI/stable/sysfs-driver-w1_ds2438 b/Documentation/ABI/stable/sysfs-driver-w1_ds2438 index fa47437c11d9..d2e7681cc287 100644 --- a/Documentation/ABI/stable/sysfs-driver-w1_ds2438 +++ b/Documentation/ABI/stable/sysfs-driver-w1_ds2438 @@ -4,3 +4,10 @@ Contact: Luiz Sampaio <sampaio.ime@gmail.com> Description: read the contents of the page1 of the DS2438 see Documentation/w1/slaves/w1_ds2438.rst for detailed information Users: any user space application which wants to communicate with DS2438 + +What: /sys/bus/w1/devices/.../offset +Date: April 2021 +Contact: Luiz Sampaio <sampaio.ime@gmail.com> +Description: write the contents to the offset register of the DS2438 + see Documentation/w1/slaves/w1_ds2438.rst for detailed information +Users: any user space application which wants to communicate with DS2438 diff --git a/Documentation/w1/slaves/w1_ds2438.rst b/Documentation/w1/slaves/w1_ds2438.rst index ac8d0d4b0d0e..5c5573991351 100644 --- a/Documentation/w1/slaves/w1_ds2438.rst +++ b/Documentation/w1/slaves/w1_ds2438.rst @@ -22,7 +22,7 @@ is also often used in weather stations and applications such as: rain gauge, wind speed/direction measuring, humidity sensing, etc. Current support is provided through the following sysfs files (all files -except "iad" are readonly): +except "iad" and "offset" are readonly): "iad" ----- @@ -52,6 +52,15 @@ Internally when this file is read, the additional CRC byte is also obtained from the slave device. If it is correct, the 8 bytes page data are passed to userspace, otherwise an I/O error is returned. +"offset" +------- +This file controls the 2-byte Offset Register of the chip. +Writing a 2-byte value will change the Offset Register, which changes the +current measurement done by the chip. Changing this register to the two's complement +of the current register while forcing zero current through the load will calibrate +the chip, canceling offset errors in the current ADC. + + "temperature" ------------- Opening and reading this file initiates the CONVERT_T (temperature conversion) diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c index 42080ae779f0..01eaa5a17fba 100644 --- a/drivers/w1/slaves/w1_ds2438.c +++ b/drivers/w1/slaves/w1_ds2438.c @@ -193,6 +193,34 @@ static int w1_ds2438_change_config_bit(struct w1_slave *sl, u8 mask, u8 value) return -1; } +static int w1_ds2438_change_offset_register(struct w1_slave *sl, u8 *value) +{ + unsigned int retries = W1_DS2438_RETRIES; + u8 w1_buf[9]; + u8 w1_page1_buf[DS2438_PAGE_SIZE + 1 /*for CRC*/]; + + if (w1_ds2438_get_page(sl, 1, w1_page1_buf) == 0) { + memcpy(&w1_buf[2], w1_page1_buf, DS2438_PAGE_SIZE - 1); /* last register reserved */ + w1_buf[7] = value[0]; /* change only offset register */ + w1_buf[8] = value[1]; + while (retries--) { + if (w1_reset_select_slave(sl)) + continue; + w1_buf[0] = W1_DS2438_WRITE_SCRATCH; + w1_buf[1] = 0x01; /* write to page 1 */ + w1_write_block(sl->master, w1_buf, 9); + + if (w1_reset_select_slave(sl)) + continue; + w1_buf[0] = W1_DS2438_COPY_SCRATCH; + w1_buf[1] = 0x01; + w1_write_block(sl->master, w1_buf, 2); + return 0; + } + } + return -1; +} + static int w1_ds2438_get_voltage(struct w1_slave *sl, int adc_input, uint16_t *voltage) { @@ -364,6 +392,25 @@ static ssize_t page1_read(struct file *filp, struct kobject *kobj, return ret; } +static ssize_t offset_write(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, + loff_t off, size_t count) +{ + struct w1_slave *sl = kobj_to_w1_slave(kobj); + int ret; + + mutex_lock(&sl->master->bus_mutex); + + if (w1_ds2438_change_offset_register(sl, buf) == 0) + ret = count; + else + ret = -EIO; + + mutex_unlock(&sl->master->bus_mutex); + + return ret; +} + static ssize_t temperature_read(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count) @@ -430,6 +477,7 @@ static ssize_t vdd_read(struct file *filp, struct kobject *kobj, static BIN_ATTR_RW(iad, 0); static BIN_ATTR_RO(page0, DS2438_PAGE_SIZE); static BIN_ATTR_RO(page1, DS2438_PAGE_SIZE); +static BIN_ATTR_WO(offset, 2); static BIN_ATTR_RO(temperature, 0/* real length varies */); static BIN_ATTR_RO(vad, 0/* real length varies */); static BIN_ATTR_RO(vdd, 0/* real length varies */); @@ -438,6 +486,7 @@ static struct bin_attribute *w1_ds2438_bin_attrs[] = { &bin_attr_iad, &bin_attr_page0, &bin_attr_page1, + &bin_attr_offset, &bin_attr_temperature, &bin_attr_vad, &bin_attr_vdd, -- 2.30.1