From: Marc Ferland <[email protected]>
Hi,
Here is v4 of my ds2433 driver patch series, see [1] for v3.
Changes:
v4: Drop last paragraph from the ds2490 patch commit message
(suggested by Krzysztof).
Rename the __ds_read_block function to read_block_chunk.
Statically allocate the validcrc bitmap, suggested by David
Laight.
Remove both W1_PAGE_COUNT and W1_F23_TIME defines from the same
patch (was previously in separate patches, suggested by
Krzysztof).
Nullify pointer earlier in w1_f23_remove_slave to better match
w1_f23_add_slave (suggested by Krzysztof).
v3: Do not use in-reply-to when sending a new patch series.
v2: Incorporate suggestions from Krzysztof Kozlowski: drop the 'w1:
ds2433: rename W1_EEPROM_DS2433' and 'w1: ds2433: rename
w1_f23_data to w1_data' patches.
Create a separate patch for the validcrc bitmap change (also suggested
by Krzysztof).
Fix build error: initializer element is not a compile-time constant.
Rework the ds2490 patch and remove the ds_write_block changes: I
have no way of reliably test this change with my current setup,
and I did not experience any write failures. Let's not try to fix
what already works.
Rearrange commit order for a more logical order.
Tested with the ds2433 eeprom.
Rebased on v6.7-rc2.
[1] https://lore.kernel.org/lkml/[email protected]/
Marc Ferland (5):
w1: ds2490: support block sizes larger than 128 bytes in ds_read_block
w1: ds2433: remove unused definitions
w1: ds2433: introduce a configuration structure
w1: ds2433: use the kernel bitmap implementation
w1: ds2433: add support for ds28ec20 eeprom
drivers/w1/masters/ds2490.c | 25 +++++-
drivers/w1/slaves/w1_ds2433.c | 162 ++++++++++++++++++++++++++++------
2 files changed, 157 insertions(+), 30 deletions(-)
base-commit: 3f7168591ebf7bbdb91797d02b1afaf00a4289b1
--
2.34.1
From: Marc Ferland <[email protected]>
The current ds_read_block function only supports block sizes up to
128 bytes, which is the depth of the 'data out' fifo on the ds2490.
Reading larger blocks will fail with a: -110 (ETIMEDOUT) from
usb_control_msg(). Example:
$ dd if=/sys/bus/w1/devices/43-000000478756/eeprom bs=256 count=1
yields to the following message from the kernel:
usb 5-1: Failed to write 1-wire data to ep0x2: err=-110.
I discovered this issue while implementing support for the ds28ec20
eeprom in the w1-2433 driver. This driver accepts reading blocks of
sizes up to the size of the entire memory (2560 bytes in the case of
the ds28ec20). Note that this issue _does not_ arise when the kernel
is configured with CONFIG_W1_SLAVE_DS2433_CRC enabled since in this
mode the driver reads one 32 byte block at a time (a single memory
page).
Also, from the ds2490 datasheet (2995.pdf, page 22, BLOCK I/O
command):
For a block write sequence the EP2 FIFO must be pre-filled with
data before command execution. Additionally, for block sizes
greater then the FIFO size, the FIFO content status must be
monitored by host SW so that additional data can be sent to the
FIFO when necessary. A similar EP3 FIFO content monitoring
requirement exists for block read sequences. During a block read
the number of bytes loaded into the EP3 FIFO must be monitored so
that the data can be read before the FIFO overflows.
Breaking the block in smaller 128 bytes chunks and simply calling the
original code sequence has solved the issue for me.
Tested with a DS1490F usb<->one-wire adapter and both the DS28EC20 and
DS2433 eeprom memories.
Signed-off-by: Marc Ferland <[email protected]>
---
drivers/w1/masters/ds2490.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index 5f5b97e24700..4b285d4944aa 100644
--- a/drivers/w1/masters/ds2490.c
+++ b/drivers/w1/masters/ds2490.c
@@ -98,6 +98,8 @@
#define ST_EPOF 0x80
/* Status transfer size, 16 bytes status, 16 byte result flags */
#define ST_SIZE 0x20
+/* 1-wire data i/o fifo size, 128 bytes */
+#define FIFO_SIZE 0x80
/* Result Register flags */
#define RR_DETECT 0xA5 /* New device detected */
@@ -614,14 +616,11 @@ static int ds_read_byte(struct ds_device *dev, u8 *byte)
return 0;
}
-static int ds_read_block(struct ds_device *dev, u8 *buf, int len)
+static int read_block_chunk(struct ds_device *dev, u8 *buf, int len)
{
struct ds_status st;
int err;
- if (len > 64*1024)
- return -E2BIG;
-
memset(buf, 0xFF, len);
err = ds_send_data(dev, buf, len);
@@ -640,6 +639,24 @@ static int ds_read_block(struct ds_device *dev, u8 *buf, int len)
return err;
}
+static int ds_read_block(struct ds_device *dev, u8 *buf, int len)
+{
+ int err, to_read, rem = len;
+
+ if (len > 64*1024)
+ return -E2BIG;
+
+ do {
+ to_read = rem <= FIFO_SIZE ? rem : FIFO_SIZE;
+ err = read_block_chunk(dev, &buf[len - rem], to_read);
+ if (err < 0)
+ return err;
+ rem -= to_read;
+ } while (rem);
+
+ return err;
+}
+
static int ds_write_block(struct ds_device *dev, u8 *buf, int len)
{
int err;
--
2.34.1
From: Marc Ferland <[email protected]>
Both W1_F23_TIME and W1_PAGE_COUNT are unused, get rid of them.
Signed-off-by: Marc Ferland <[email protected]>
---
drivers/w1/slaves/w1_ds2433.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
index 9f21fd98f799..cd99eceac1ae 100644
--- a/drivers/w1/slaves/w1_ds2433.c
+++ b/drivers/w1/slaves/w1_ds2433.c
@@ -25,13 +25,10 @@
#define W1_EEPROM_DS2433 0x23
#define W1_EEPROM_SIZE 512
-#define W1_PAGE_COUNT 16
#define W1_PAGE_SIZE 32
#define W1_PAGE_BITS 5
#define W1_PAGE_MASK 0x1F
-#define W1_F23_TIME 300
-
#define W1_F23_READ_EEPROM 0xF0
#define W1_F23_WRITE_SCRATCH 0x0F
#define W1_F23_READ_SCRATCH 0xAA
--
2.34.1
From: Marc Ferland <[email protected]>
Add a ds2433_config structure for parameters that are different
between the ds2433 and the ds28ec20. The goal is to reuse the same
code for both chips.
A pointer to this config structure is added to w1_f23_data and the
CONFIG_W1_SLAVE_DS2433_CRC ifdefs are adjusted since now both driver
configurations (with or without crc support) will make use of
w1_f23_data.
Also, the 'memory' buffer is now dynamically allocated based on the
size specififed in the config structure to help support memories of
different sizes.
Signed-off-by: Marc Ferland <[email protected]>
---
drivers/w1/slaves/w1_ds2433.c | 47 ++++++++++++++++++++++++++---------
1 file changed, 35 insertions(+), 12 deletions(-)
diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
index cd99eceac1ae..0c67082c8bb7 100644
--- a/drivers/w1/slaves/w1_ds2433.c
+++ b/drivers/w1/slaves/w1_ds2433.c
@@ -34,9 +34,24 @@
#define W1_F23_READ_SCRATCH 0xAA
#define W1_F23_COPY_SCRATCH 0x55
+struct ds2433_config {
+ size_t eeprom_size; /* eeprom size in bytes */
+ unsigned int page_count; /* number of 256 bits pages */
+ unsigned int tprog; /* time in ms for page programming */
+};
+
+static const struct ds2433_config config_f23 = {
+ .eeprom_size = W1_EEPROM_SIZE,
+ .page_count = 16,
+ .tprog = 5,
+};
+
struct w1_f23_data {
- u8 memory[W1_EEPROM_SIZE];
+#ifdef CONFIG_W1_SLAVE_DS2433_CRC
+ u8 *memory;
u32 validcrc;
+#endif
+ const struct ds2433_config *cfg;
};
/*
@@ -95,7 +110,7 @@ static ssize_t eeprom_read(struct file *filp, struct kobject *kobj,
u8 wrbuf[3];
#endif
- count = w1_f23_fix_count(off, count, W1_EEPROM_SIZE);
+ count = w1_f23_fix_count(off, count, bin_attr->size);
if (!count)
return 0;
@@ -150,9 +165,7 @@ static ssize_t eeprom_read(struct file *filp, struct kobject *kobj,
*/
static int w1_f23_write(struct w1_slave *sl, int addr, int len, const u8 *data)
{
-#ifdef CONFIG_W1_SLAVE_DS2433_CRC
struct w1_f23_data *f23 = sl->family_data;
-#endif
u8 wrbuf[4];
u8 rdbuf[W1_PAGE_SIZE + 3];
u8 es = (addr + len - 1) & 0x1f;
@@ -188,8 +201,8 @@ static int w1_f23_write(struct w1_slave *sl, int addr, int len, const u8 *data)
wrbuf[3] = es;
w1_write_block(sl->master, wrbuf, 4);
- /* Sleep for 5 ms to wait for the write to complete */
- msleep(5);
+ /* Sleep for tprog ms to wait for the write to complete */
+ msleep(f23->cfg->tprog);
/* Reset the bus to wake up the EEPROM (this may not be needed) */
w1_reset_bus(sl->master);
@@ -206,7 +219,7 @@ static ssize_t eeprom_write(struct file *filp, struct kobject *kobj,
struct w1_slave *sl = kobj_to_w1_slave(kobj);
int addr, len, idx;
- count = w1_f23_fix_count(off, count, W1_EEPROM_SIZE);
+ count = w1_f23_fix_count(off, count, bin_attr->size);
if (!count)
return 0;
@@ -268,24 +281,34 @@ static const struct attribute_group *w1_f23_groups[] = {
static int w1_f23_add_slave(struct w1_slave *sl)
{
-#ifdef CONFIG_W1_SLAVE_DS2433_CRC
struct w1_f23_data *data;
data = kzalloc(sizeof(struct w1_f23_data), GFP_KERNEL);
if (!data)
return -ENOMEM;
+
+ data->cfg = &config_f23;
+
+#ifdef CONFIG_W1_SLAVE_DS2433_CRC
+ data->memory = kzalloc(data->cfg->eeprom_size, GFP_KERNEL);
+ if (!data->memory) {
+ kfree(data);
+ return -ENOMEM;
+ }
+#endif /* CONFIG_W1_SLAVE_DS2433_CRC */
sl->family_data = data;
-#endif /* CONFIG_W1_SLAVE_DS2433_CRC */
return 0;
}
static void w1_f23_remove_slave(struct w1_slave *sl)
{
-#ifdef CONFIG_W1_SLAVE_DS2433_CRC
- kfree(sl->family_data);
+ struct w1_f23_data *data = sl->family_data;
sl->family_data = NULL;
-#endif /* CONFIG_W1_SLAVE_DS2433_CRC */
+#ifdef CONFIG_W1_SLAVE_DS2433_CRC
+ kfree(data->memory);
+#endif /* CONFIG_W1_SLAVE_DS2433_CRC */
+ kfree(data);
}
static const struct w1_family_ops w1_f23_fops = {
--
2.34.1
From: Marc Ferland <[email protected]>
The ds28ec20 eeprom is (almost) backward compatible with the
ds2433. The only differences are:
- the eeprom size is now 2560 bytes instead of 512;
- the number of pages is now 80 (same page size as the ds2433: 256 bits);
- the programming time has increased from 5ms to 10ms;
This patch adds support for the ds28ec20 to the ds2433 driver. From
the datasheet: The DS28EC20 provides a high degree of backward
compatibility with the DS2433. Besides the different family codes, the
only protocol change that is required on an existing DS2433
implementation is a lengthening of the programming duration (tPROG)
from 5ms to 10ms.
dmesg now returns:
w1_master_driver w1_bus_master1: Attaching one wire slave 43.000000478756 crc e0
instead of:
w1_master_driver w1_bus_master1: Attaching one wire slave 43.000000478756 crc e0
w1_master_driver w1_bus_master1: Family 43 for 43.000000478756.e0 is not registered.
Test script writing/reading random data (CONFIG_W1_SLAVE_DS2433_CRC is
not set):
#!/bin/sh
EEPROM=/sys/bus/w1/devices/43-000000478756/eeprom
BINFILE1=/home/root/file1.bin
BINFILE2=/home/root/file2.bin
for BS in 1 2 3 4 8 16 32 64 128 256 512 1024 2560; do
dd if=/dev/random of=${BINFILE1} bs=${BS} count=1 status=none
dd if=${BINFILE1} of=${EEPROM} status=none
dd if=${EEPROM} of=${BINFILE2} bs=${BS} count=1 status=none
if ! cmp --silent ${BINFILE1} ${BINFILE2}; then
echo file1
hexdump ${BINFILE1}
echo file2
hexdump ${BINFILE2}
echo FAIL
exit 1
fi
echo "${BS} OK!"
done
Results:
# ./test.sh
1 OK!
2 OK!
3 OK!
4 OK!
8 OK!
16 OK!
32 OK!
64 OK!
128 OK!
256 OK!
512 OK!
1024 OK!
2560 OK!
Tests with CONFIG_W1_SLAVE_DS2433_CRC=y:
$ cat /proc/config.gz | gunzip | grep CONFIG_W1_SLAVE_DS2433
CONFIG_W1_SLAVE_DS2433=m
CONFIG_W1_SLAVE_DS2433_CRC=y
# create a 32 bytes block with a crc, i.e.:
00000000 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f 40 |123456789:;<=>?@|
00000010 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e ba 63 |ABCDEFGHIJKLMN.c|
# fill all 80 blocks
$ dd if=test.bin of=/sys/bus/w1/devices/43-000000478756/eeprom bs=32 count=80
# read back all blocks, i.e.:
$ hexdump -C /sys/bus/w1/devices/43-000000478756/eeprom
00000000 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f 40 |123456789:;<=>?@|
00000010 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e ba 63 |ABCDEFGHIJKLMN.c|
00000020 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f 40 |123456789:;<=>?@|
00000030 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e ba 63 |ABCDEFGHIJKLMN.c|
...
000009e0 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f 40 |123456789:;<=>?@|
000009f0 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e ba 63 |ABCDEFGHIJKLMN.c|
00000a00
Note: both memories (ds2433 and ds28ec20) have been tested with the
new driver.
Signed-off-by: Marc Ferland <[email protected]>
Co-developed-by: Jean-Francois Dagenais <[email protected]>
Signed-off-by: Jean-Francois Dagenais <[email protected]>
---
drivers/w1/slaves/w1_ds2433.c | 101 +++++++++++++++++++++++++++++++---
1 file changed, 92 insertions(+), 9 deletions(-)
diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
index 87df76094e03..250b7f7ec429 100644
--- a/drivers/w1/slaves/w1_ds2433.c
+++ b/drivers/w1/slaves/w1_ds2433.c
@@ -1,8 +1,9 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
- * w1_ds2433.c - w1 family 23 (DS2433) driver
+ * w1_ds2433.c - w1 family 23 (DS2433) & 43 (DS28EC20) eeprom driver
*
* Copyright (c) 2005 Ben Gardner <[email protected]>
+ * Copyright (c) 2023 Marc Ferland <[email protected]>
*/
#include <linux/kernel.h>
@@ -23,12 +24,15 @@
#include <linux/w1.h>
#define W1_EEPROM_DS2433 0x23
+#define W1_EEPROM_DS28EC20 0x43
+
+#define W1_EEPROM_DS2433_SIZE 512
+#define W1_EEPROM_DS28EC20_SIZE 2560
-#define W1_EEPROM_SIZE 512
#define W1_PAGE_SIZE 32
#define W1_PAGE_BITS 5
#define W1_PAGE_MASK 0x1F
-#define W1_VALIDCRC_MAX 32
+#define W1_VALIDCRC_MAX 96
#define W1_F23_READ_EEPROM 0xF0
#define W1_F23_WRITE_SCRATCH 0x0F
@@ -42,11 +46,17 @@ struct ds2433_config {
};
static const struct ds2433_config config_f23 = {
- .eeprom_size = W1_EEPROM_SIZE,
+ .eeprom_size = W1_EEPROM_DS2433_SIZE,
.page_count = 16,
.tprog = 5,
};
+static const struct ds2433_config config_f43 = {
+ .eeprom_size = W1_EEPROM_DS28EC20_SIZE,
+ .page_count = 80,
+ .tprog = 10,
+};
+
struct w1_f23_data {
#ifdef CONFIG_W1_SLAVE_DS2433_CRC
u8 *memory;
@@ -264,10 +274,22 @@ static ssize_t eeprom_write(struct file *filp, struct kobject *kobj,
return count;
}
-static BIN_ATTR_RW(eeprom, W1_EEPROM_SIZE);
+static struct bin_attribute bin_attr_f23_eeprom = {
+ .attr = { .name = "eeprom", .mode = 0644 },
+ .read = eeprom_read,
+ .write = eeprom_write,
+ .size = W1_EEPROM_DS2433_SIZE,
+};
+
+static struct bin_attribute bin_attr_f43_eeprom = {
+ .attr = { .name = "eeprom", .mode = 0644 },
+ .read = eeprom_read,
+ .write = eeprom_write,
+ .size = W1_EEPROM_DS28EC20_SIZE,
+};
static struct bin_attribute *w1_f23_bin_attributes[] = {
- &bin_attr_eeprom,
+ &bin_attr_f23_eeprom,
NULL,
};
@@ -280,6 +302,20 @@ static const struct attribute_group *w1_f23_groups[] = {
NULL,
};
+static struct bin_attribute *w1_f43_bin_attributes[] = {
+ &bin_attr_f43_eeprom,
+ NULL,
+};
+
+static const struct attribute_group w1_f43_group = {
+ .bin_attrs = w1_f43_bin_attributes,
+};
+
+static const struct attribute_group *w1_f43_groups[] = {
+ &w1_f43_group,
+ NULL,
+};
+
static int w1_f23_add_slave(struct w1_slave *sl)
{
struct w1_f23_data *data;
@@ -288,7 +324,14 @@ static int w1_f23_add_slave(struct w1_slave *sl)
if (!data)
return -ENOMEM;
- data->cfg = &config_f23;
+ switch (sl->family->fid) {
+ case W1_EEPROM_DS2433:
+ data->cfg = &config_f23;
+ break;
+ case W1_EEPROM_DS28EC20:
+ data->cfg = &config_f43;
+ break;
+ }
#ifdef CONFIG_W1_SLAVE_DS2433_CRC
if (data->cfg->page_count > W1_VALIDCRC_MAX) {
@@ -324,13 +367,53 @@ static const struct w1_family_ops w1_f23_fops = {
.groups = w1_f23_groups,
};
+static const struct w1_family_ops w1_f43_fops = {
+ .add_slave = w1_f23_add_slave,
+ .remove_slave = w1_f23_remove_slave,
+ .groups = w1_f43_groups,
+};
+
static struct w1_family w1_family_23 = {
.fid = W1_EEPROM_DS2433,
.fops = &w1_f23_fops,
};
-module_w1_family(w1_family_23);
+
+static struct w1_family w1_family_43 = {
+ .fid = W1_EEPROM_DS28EC20,
+ .fops = &w1_f43_fops,
+};
+
+static int __init w1_ds2433_init(void)
+{
+ int err;
+
+ err = w1_register_family(&w1_family_23);
+ if (err)
+ return err;
+
+ err = w1_register_family(&w1_family_43);
+ if (err)
+ goto err_43;
+
+ return 0;
+
+err_43:
+ w1_unregister_family(&w1_family_23);
+ return err;
+}
+
+static void __exit w1_ds2433_exit(void)
+{
+ w1_unregister_family(&w1_family_23);
+ w1_unregister_family(&w1_family_43);
+}
+
+module_init(w1_ds2433_init);
+module_exit(w1_ds2433_exit);
MODULE_AUTHOR("Ben Gardner <[email protected]>");
-MODULE_DESCRIPTION("w1 family 23 driver for DS2433, 4kb EEPROM");
+MODULE_AUTHOR("Marc Ferland <[email protected]>");
+MODULE_DESCRIPTION("w1 family 23/43 driver for DS2433 (4kb) and DS28EC20 (20kb)");
MODULE_LICENSE("GPL");
MODULE_ALIAS("w1-family-" __stringify(W1_EEPROM_DS2433));
+MODULE_ALIAS("w1-family-" __stringify(W1_EEPROM_DS28EC20));
--
2.34.1
On Mon, 18 Dec 2023 10:02:25 -0500, [email protected] wrote:
> From: Marc Ferland <[email protected]>
>
> Hi,
>
> Here is v4 of my ds2433 driver patch series, see [1] for v3.
>
> Changes:
> v4: Drop last paragraph from the ds2490 patch commit message
> (suggested by Krzysztof).
> Rename the __ds_read_block function to read_block_chunk.
> Statically allocate the validcrc bitmap, suggested by David
> Laight.
> Remove both W1_PAGE_COUNT and W1_F23_TIME defines from the same
> patch (was previously in separate patches, suggested by
> Krzysztof).
> Nullify pointer earlier in w1_f23_remove_slave to better match
> w1_f23_add_slave (suggested by Krzysztof).
> v3: Do not use in-reply-to when sending a new patch series.
> v2: Incorporate suggestions from Krzysztof Kozlowski: drop the 'w1:
> ds2433: rename W1_EEPROM_DS2433' and 'w1: ds2433: rename
> w1_f23_data to w1_data' patches.
> Create a separate patch for the validcrc bitmap change (also suggested
> by Krzysztof).
> Fix build error: initializer element is not a compile-time constant.
> Rework the ds2490 patch and remove the ds_write_block changes: I
> have no way of reliably test this change with my current setup,
> and I did not experience any write failures. Let's not try to fix
> what already works.
> Rearrange commit order for a more logical order.
> Tested with the ds2433 eeprom.
> Rebased on v6.7-rc2.
>
> [...]
Applied, thanks!
[1/5] w1: ds2490: support block sizes larger than 128 bytes in ds_read_block
https://git.kernel.org/krzk/linux-w1/c/d605ba72e9c04efc35fcf225df59d4ccb1d4061f
[2/5] w1: ds2433: remove unused definitions
https://git.kernel.org/krzk/linux-w1/c/86626c06d651c72bc10c25f263e98fa90655b5ae
[3/5] w1: ds2433: introduce a configuration structure
https://git.kernel.org/krzk/linux-w1/c/75f0c1c78d709f258004562a540c83bc05bfb962
[4/5] w1: ds2433: use the kernel bitmap implementation
https://git.kernel.org/krzk/linux-w1/c/3fe3a1bfef75efcdfbcca955fe1d47ec07215110
[5/5] w1: ds2433: add support for ds28ec20 eeprom
https://git.kernel.org/krzk/linux-w1/c/93c4bb3666a3d463c73a66ab3cc78a4c4b83631a
Best regards,
--
Krzysztof Kozlowski <[email protected]>