From: Marc Ferland <[email protected]>
Hi,
Here is v3 of my ds2433 driver patch series, see [1] for v2.
Changes:
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 definition
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 | 161 ++++++++++++++++++++++++++++------
2 files changed, 157 insertions(+), 29 deletions(-)
base-commit: 98b1cc82c4affc16f5598d4fa14b1858671b2263
--
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 e18523ef8c45..7d4d9fc1a9c4 100644
--- a/drivers/w1/slaves/w1_ds2433.c
+++ b/drivers/w1/slaves/w1_ds2433.c
@@ -25,7 +25,7 @@
#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
@@ -35,9 +35,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;
};
/*
@@ -96,7 +111,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;
@@ -151,9 +166,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;
@@ -189,8 +202,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);
@@ -207,7 +220,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;
@@ -269,24 +282,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)
{
+ struct w1_f23_data *data = sl->family_data;
#ifdef CONFIG_W1_SLAVE_DS2433_CRC
- kfree(sl->family_data);
+ kfree(data->memory);
+#endif /* CONFIG_W1_SLAVE_DS2433_CRC */
+ kfree(data);
sl->family_data = NULL;
-#endif /* CONFIG_W1_SLAVE_DS2433_CRC */
}
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 | 98 ++++++++++++++++++++++++++++++++---
1 file changed, 90 insertions(+), 8 deletions(-)
diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
index 63ed03191137..ab1491a7854a 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,8 +24,10 @@
#include <linux/w1.h>
#define W1_EEPROM_DS2433 0x23
+#define W1_EEPROM_DS28EC20 0x43
-#define W1_EEPROM_SIZE 512
+#define W1_EEPROM_DS2433_SIZE 512
+#define W1_EEPROM_DS28EC20_SIZE 2560
#define W1_PAGE_SIZE 32
#define W1_PAGE_BITS 5
@@ -42,11 +45,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 +273,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 +301,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 +323,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
data->memory = kzalloc(data->cfg->eeprom_size, GFP_KERNEL);
@@ -326,13 +368,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 30/11/2023 14:52, [email protected] wrote:
> 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)
> {
> + struct w1_f23_data *data = sl->family_data;
> #ifdef CONFIG_W1_SLAVE_DS2433_CRC
> - kfree(sl->family_data);
> + kfree(data->memory);
> +#endif /* CONFIG_W1_SLAVE_DS2433_CRC */
> + kfree(data);
> sl->family_data = NULL;
This should be before kfree(), to match w1_f23_add_slave(). You can move
it now, since you store the pointer in data.
> -#endif /* CONFIG_W1_SLAVE_DS2433_CRC */
> }
>
> static const struct w1_family_ops w1_f23_fops = {
Best regards,
Krzysztof