[PATCH 1/7] w1: ds2433: remove unused definition

[PATCH 1/7] w1: ds2433: remove unused definition

[marc.ferland]

From: Marc Ferland <marc.ferland@sonatest.com>

W1_F23_TIME isn't used anywhere, get rid of it.

Signed-off-by: Marc Ferland <marc.ferland@sonatest.com>
---
 drivers/w1/slaves/w1_ds2433.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
index 9f21fd98f799..e18523ef8c45 100644
--- a/drivers/w1/slaves/w1_ds2433.c
+++ b/drivers/w1/slaves/w1_ds2433.c
@@ -30,8 +30,6 @@
 #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

base-commit: 7475e51b87969e01a6812eac713a1c8310372e8a
-- 
2.34.1

[PATCH 2/7] w1: ds2433: add support for registering multiple families

[marc.ferland]

From: Jean-Francois Dagenais <jeff.dagenais@gmail.com>

This is ground work for supporting both the ds2433 and the
ds28ec20. Inspired by the w1_ds250x driver.

Signed-off-by: Marc Ferland <marc.ferland@sonatest.com>
Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
---
 drivers/w1/slaves/w1_ds2433.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
index e18523ef8c45..e1e45ea1bfa4 100644
--- a/drivers/w1/slaves/w1_ds2433.c
+++ b/drivers/w1/slaves/w1_ds2433.c
@@ -299,7 +299,25 @@ static struct w1_family w1_family_23 = {
 	.fid = W1_EEPROM_DS2433,
 	.fops = &w1_f23_fops,
 };
-module_w1_family(w1_family_23);
+
+static int __init w1_ds2433_init(void)
+{
+	int err;
+
+	err = w1_register_family(&w1_family_23);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static void __exit w1_ds2433_exit(void)
+{
+	w1_unregister_family(&w1_family_23);
+}
+
+module_init(w1_ds2433_init);
+module_exit(w1_ds2433_exit);
 
 MODULE_AUTHOR("Ben Gardner <bgardner@wabtec.com>");
 MODULE_DESCRIPTION("w1 family 23 driver for DS2433, 4kb EEPROM");
-- 
2.34.1

Re: [PATCH 2/7] w1: ds2433: add support for registering multiple families

[Krzysztof Kozlowski]

On 17/11/2023 20:29, marc.ferland@gmail.com wrote:
> From: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
> 
> This is ground work for supporting both the ds2433 and the
> ds28ec20. Inspired by the w1_ds250x driver.
> 
> Signed-off-by: Marc Ferland <marc.ferland@sonatest.com>
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>

Your patchset has build warnings, so only limited review follows. I will
perform full review once you do not have any warnings.

> ---
>  drivers/w1/slaves/w1_ds2433.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
> index e18523ef8c45..e1e45ea1bfa4 100644
> --- a/drivers/w1/slaves/w1_ds2433.c
> +++ b/drivers/w1/slaves/w1_ds2433.c
> @@ -299,7 +299,25 @@ static struct w1_family w1_family_23 = {
>  	.fid = W1_EEPROM_DS2433,
>  	.fops = &w1_f23_fops,
>  };
> -module_w1_family(w1_family_23);
> +
> +static int __init w1_ds2433_init(void)
> +{
> +	int err;
> +
> +	err = w1_register_family(&w1_family_23);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static void __exit w1_ds2433_exit(void)
> +{
> +	w1_unregister_family(&w1_family_23);
> +}

This does not make much sense on its own. I do not see any benefits.

Best regards,
Krzysztof

[PATCH 3/7] w1: ds2433: rename W1_EEPROM_DS2433

[marc.ferland]

From: Marc Ferland <marc.ferland@sonatest.com>

Rename the W1_EEPROM_DS2433 define to W1_F23_EEPROM_DS2433 to make it
clear it is associated with family 0x23.

This is ground work for supporting both the ds2433 and the ds28ec20.

Signed-off-by: Marc Ferland <marc.ferland@sonatest.com>
---
 drivers/w1/slaves/w1_ds2433.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
index e1e45ea1bfa4..6b04fdef2923 100644
--- a/drivers/w1/slaves/w1_ds2433.c
+++ b/drivers/w1/slaves/w1_ds2433.c
@@ -22,7 +22,7 @@
 
 #include <linux/w1.h>
 
-#define W1_EEPROM_DS2433	0x23
+#define W1_F23_EEPROM_DS2433	0x23
 
 #define W1_EEPROM_SIZE		512
 #define W1_PAGE_COUNT		16
@@ -296,7 +296,7 @@ static const struct w1_family_ops w1_f23_fops = {
 };
 
 static struct w1_family w1_family_23 = {
-	.fid = W1_EEPROM_DS2433,
+	.fid = W1_F23_EEPROM_DS2433,
 	.fops = &w1_f23_fops,
 };
 
@@ -322,4 +322,4 @@ module_exit(w1_ds2433_exit);
 MODULE_AUTHOR("Ben Gardner <bgardner@wabtec.com>");
 MODULE_DESCRIPTION("w1 family 23 driver for DS2433, 4kb EEPROM");
 MODULE_LICENSE("GPL");
-MODULE_ALIAS("w1-family-" __stringify(W1_EEPROM_DS2433));
+MODULE_ALIAS("w1-family-" __stringify(W1_F23_EEPROM_DS2433));
-- 
2.34.1

Re: [PATCH 3/7] w1: ds2433: rename W1_EEPROM_DS2433

[Krzysztof Kozlowski]

On 17/11/2023 20:29, marc.ferland@gmail.com wrote:
> From: Marc Ferland <marc.ferland@sonatest.com>
> 
> Rename the W1_EEPROM_DS2433 define to W1_F23_EEPROM_DS2433 to make it
> clear it is associated with family 0x23.
> 
> This is ground work for supporting both the ds2433 and the ds28ec20.

Why do you need to "associate" it with family 0x23? Can ds2433 be 0x43?
If not, family is irrelevant information.

> 
> Signed-off-by: Marc Ferland <marc.ferland@sonatest.com>
> ---
>  drivers/w1/slaves/w1_ds2433.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Best regards,
Krzysztof

[PATCH 4/7] w1: ds2433: introduce a configuration structure

[marc.ferland]

From: Marc Ferland <marc.ferland@sonatest.com>

Replace defines with a ds2433_config structure for parameters that are
different between the ds2433 and the ds28ec20.

This is ground work for supporting both the ds2433 and the ds28ec20.

Signed-off-by: Marc Ferland <marc.ferland@sonatest.com>
---
 drivers/w1/slaves/w1_ds2433.c | 56 +++++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 15 deletions(-)

diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
index 6b04fdef2923..75620f67248f 100644
--- a/drivers/w1/slaves/w1_ds2433.c
+++ b/drivers/w1/slaves/w1_ds2433.c
@@ -24,8 +24,6 @@
 
 #define W1_F23_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 +33,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 = 512,
+	.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 +109,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 +164,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 +200,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 +218,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;
 
@@ -251,10 +262,15 @@ 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 = config_f23.eeprom_size,
+};
 
 static struct bin_attribute *w1_f23_bin_attributes[] = {
-	&bin_attr_eeprom,
+	&bin_attr_f23_eeprom,
 	NULL,
 };
 
@@ -269,24 +285,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

Re: [PATCH 4/7] w1: ds2433: introduce a configuration structure

[kernel test robot]

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on 7475e51b87969e01a6812eac713a1c8310372e8a]

url:    https://github.com/intel-lab-lkp/linux/commits/marc-ferland-gmail-com/w1-ds2433-add-support-for-registering-multiple-families/20231118-034145
base:   7475e51b87969e01a6812eac713a1c8310372e8a
patch link:    https://lore.kernel.org/r/20231117192909.98944-4-marc.ferland%40sonatest.com
patch subject: [PATCH 4/7] w1: ds2433: introduce a configuration structure
config: hexagon-randconfig-001-20231118 (https://download.01.org/0day-ci/archive/20231118/202311181436.TyinUJiw-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231118/202311181436.TyinUJiw-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311181436.TyinUJiw-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/w1/slaves/w1_ds2433.c:269:21: error: initializer element is not a compile-time constant
     269 |         .size = config_f23.eeprom_size,
         |                 ~~~~~~~~~~~^~~~~~~~~~~
   1 error generated.


vim +269 drivers/w1/slaves/w1_ds2433.c

   264	
   265	static struct bin_attribute bin_attr_f23_eeprom = {
   266		.attr = { .name = "eeprom", .mode = 0644 },
   267		.read = eeprom_read,
   268		.write = eeprom_write,
 > 269		.size = config_f23.eeprom_size,
   270	};
   271	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 5/7] w1: ds2433: rename w1_f23_data to w1_data

[marc.ferland]

From: Marc Ferland <marc.ferland@sonatest.com>

Make the code less specific to the ds2433 by renaming the private data
structure.

No functional changes.

Signed-off-by: Marc Ferland <marc.ferland@sonatest.com>
---
 drivers/w1/slaves/w1_ds2433.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
index 75620f67248f..04c3eee9e5d7 100644
--- a/drivers/w1/slaves/w1_ds2433.c
+++ b/drivers/w1/slaves/w1_ds2433.c
@@ -45,7 +45,7 @@ static const struct ds2433_config config_f23 = {
 	.tprog = 5,
 };
 
-struct w1_f23_data {
+struct w1_data {
 #ifdef CONFIG_W1_SLAVE_DS2433_CRC
 	u8	*memory;
 	u32	validcrc;
@@ -69,7 +69,7 @@ static inline size_t w1_f23_fix_count(loff_t off, size_t count, size_t size)
 }
 
 #ifdef CONFIG_W1_SLAVE_DS2433_CRC
-static int w1_f23_refresh_block(struct w1_slave *sl, struct w1_f23_data *data,
+static int w1_f23_refresh_block(struct w1_slave *sl, struct w1_data *data,
 				int block)
 {
 	u8	wrbuf[3];
@@ -103,7 +103,7 @@ static ssize_t eeprom_read(struct file *filp, struct kobject *kobj,
 {
 	struct w1_slave *sl = kobj_to_w1_slave(kobj);
 #ifdef CONFIG_W1_SLAVE_DS2433_CRC
-	struct w1_f23_data *data = sl->family_data;
+	struct w1_data *data = sl->family_data;
 	int i, min_page, max_page;
 #else
 	u8 wrbuf[3];
@@ -164,7 +164,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)
 {
-	struct w1_f23_data *f23 = sl->family_data;
+	struct w1_data *fdata = sl->family_data;
 	u8 wrbuf[4];
 	u8 rdbuf[W1_PAGE_SIZE + 3];
 	u8 es = (addr + len - 1) & 0x1f;
@@ -201,12 +201,12 @@ static int w1_f23_write(struct w1_slave *sl, int addr, int len, const u8 *data)
 	w1_write_block(sl->master, wrbuf, 4);
 
 	/* Sleep for tprog ms to wait for the write to complete */
-	msleep(f23->cfg->tprog);
+	msleep(fdata->cfg->tprog);
 
 	/* Reset the bus to wake up the EEPROM (this may not be needed) */
 	w1_reset_bus(sl->master);
 #ifdef CONFIG_W1_SLAVE_DS2433_CRC
-	f23->validcrc &= ~(1 << (addr >> W1_PAGE_BITS));
+	fdata->validcrc &= ~(1 << (addr >> W1_PAGE_BITS));
 #endif
 	return 0;
 }
@@ -283,11 +283,11 @@ static const struct attribute_group *w1_f23_groups[] = {
 	NULL,
 };
 
-static int w1_f23_add_slave(struct w1_slave *sl)
+static int w1_add_slave(struct w1_slave *sl)
 {
-	struct w1_f23_data *data;
+	struct w1_data *data;
 
-	data = kzalloc(sizeof(struct w1_f23_data), GFP_KERNEL);
+	data = kzalloc(sizeof(struct w1_data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
@@ -305,9 +305,9 @@ static int w1_f23_add_slave(struct w1_slave *sl)
 	return 0;
 }
 
-static void w1_f23_remove_slave(struct w1_slave *sl)
+static void w1_remove_slave(struct w1_slave *sl)
 {
-	struct w1_f23_data *data = sl->family_data;
+	struct w1_data *data = sl->family_data;
 #ifdef CONFIG_W1_SLAVE_DS2433_CRC
 	kfree(data->memory);
 #endif /* CONFIG_W1_SLAVE_DS2433_CRC */
@@ -316,8 +316,8 @@ static void w1_f23_remove_slave(struct w1_slave *sl)
 }
 
 static const struct w1_family_ops w1_f23_fops = {
-	.add_slave      = w1_f23_add_slave,
-	.remove_slave   = w1_f23_remove_slave,
+	.add_slave      = w1_add_slave,
+	.remove_slave   = w1_remove_slave,
 	.groups		= w1_f23_groups,
 };
 
-- 
2.34.1

Re: [PATCH 5/7] w1: ds2433: rename w1_f23_data to w1_data

[Krzysztof Kozlowski]

On 17/11/2023 20:29, marc.ferland@gmail.com wrote:
> From: Marc Ferland <marc.ferland@sonatest.com>
> 
> Make the code less specific to the ds2433 by renaming the private data
> structure.
> 
> No functional changes.

No need for that. Naming does not matter.


Best regards,
Krzysztof

[PATCH 6/7] w1: ds2433: add support for ds28ec20 eeprom

[marc.ferland]

From: Marc Ferland <marc.ferland@sonatest.com>

The ds28ec20 eeprom is (almost) backward compatible with the
ds2433. The only major 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.

Tests:

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:

    #!/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

Test results (CONFIG_W1_SLAVE_DS2433_CRC is not set):

   $ cat /proc/config.gz | gunzip | grep CONFIG_W1_SLAVE_DS2433
   CONFIG_W1_SLAVE_DS2433=m
   # CONFIG_W1_SLAVE_DS2433_CRC is not set

    # ./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!

Test results (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

Signed-off-by: Marc Ferland <marc.ferland@sonatest.com>
---
 drivers/w1/slaves/w1_ds2433.c | 84 +++++++++++++++++++++++++++++++----
 1 file changed, 75 insertions(+), 9 deletions(-)

diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
index 04c3eee9e5d7..69bdf3dba573 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 <bgardner@wabtec.com>
+ * Copyright (c) 2023 Marc Ferland <marc.ferland@sonatest.com>
  */
 
 #include <linux/kernel.h>
@@ -23,6 +24,7 @@
 #include <linux/w1.h>
 
 #define W1_F23_EEPROM_DS2433	0x23
+#define W1_F43_EEPROM_DS28EC20	0x43
 
 #define W1_PAGE_SIZE		32
 #define W1_PAGE_BITS		5
@@ -45,10 +47,16 @@ static const struct ds2433_config config_f23 = {
 	.tprog = 5,
 };
 
+static const struct ds2433_config config_f43 = {
+	.eeprom_size = 2560,
+	.page_count = 80,
+	.tprog = 10,
+};
+
 struct w1_data {
 #ifdef CONFIG_W1_SLAVE_DS2433_CRC
-	u8	*memory;
-	u32	validcrc;
+	u8		*memory;
+	unsigned long	*validcrc;
 #endif
 	const struct ds2433_config *cfg;
 };
@@ -75,11 +83,11 @@ static int w1_f23_refresh_block(struct w1_slave *sl, struct w1_data *data,
 	u8	wrbuf[3];
 	int	off = block * W1_PAGE_SIZE;
 
-	if (data->validcrc & (1 << block))
+	if (test_bit(block, data->validcrc))
 		return 0;
 
 	if (w1_reset_select_slave(sl)) {
-		data->validcrc = 0;
+		bitmap_zero(data->validcrc, data->cfg->page_count);
 		return -EIO;
 	}
 
@@ -91,7 +99,7 @@ static int w1_f23_refresh_block(struct w1_slave *sl, struct w1_data *data,
 
 	/* cache the block if the CRC is valid */
 	if (crc16(CRC16_INIT, &data->memory[off], W1_PAGE_SIZE) == CRC16_VALID)
-		data->validcrc |= (1 << block);
+		set_bit(block, data->validcrc);
 
 	return 0;
 }
@@ -206,7 +214,7 @@ static int w1_f23_write(struct w1_slave *sl, int addr, int len, const u8 *data)
 	/* Reset the bus to wake up the EEPROM (this may not be needed) */
 	w1_reset_bus(sl->master);
 #ifdef CONFIG_W1_SLAVE_DS2433_CRC
-	fdata->validcrc &= ~(1 << (addr >> W1_PAGE_BITS));
+	clear_bit(addr >> W1_PAGE_BITS, fdata->validcrc);
 #endif
 	return 0;
 }
@@ -269,6 +277,13 @@ static struct bin_attribute bin_attr_f23_eeprom = {
 	.size = config_f23.eeprom_size,
 };
 
+static struct bin_attribute bin_attr_f43_eeprom = {
+	.attr = { .name = "eeprom", .mode = 0644 },
+	.read = eeprom_read,
+	.write = eeprom_write,
+	.size = config_f43.eeprom_size,
+};
+
 static struct bin_attribute *w1_f23_bin_attributes[] = {
 	&bin_attr_f23_eeprom,
 	NULL,
@@ -283,6 +298,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_add_slave(struct w1_slave *sl)
 {
 	struct w1_data *data;
@@ -291,7 +320,14 @@ static int w1_add_slave(struct w1_slave *sl)
 	if (!data)
 		return -ENOMEM;
 
-	data->cfg = &config_f23;
+	switch (sl->family->fid) {
+	case W1_F23_EEPROM_DS2433:
+		data->cfg = &config_f23;
+		break;
+	case W1_F43_EEPROM_DS28EC20:
+		data->cfg = &config_f43;
+		break;
+	}
 
 #ifdef CONFIG_W1_SLAVE_DS2433_CRC
 	data->memory = kzalloc(data->cfg->eeprom_size, GFP_KERNEL);
@@ -299,6 +335,13 @@ static int w1_add_slave(struct w1_slave *sl)
 		kfree(data);
 		return -ENOMEM;
 	}
+	data->validcrc = bitmap_zalloc(data->cfg->page_count,
+				       GFP_KERNEL);
+	if (!data->validcrc) {
+		kfree(data->memory);
+		kfree(data);
+		return -ENOMEM;
+	}
 #endif /* CONFIG_W1_SLAVE_DS2433_CRC */
 	sl->family_data = data;
 
@@ -310,6 +353,7 @@ static void w1_remove_slave(struct w1_slave *sl)
 	struct w1_data *data = sl->family_data;
 #ifdef CONFIG_W1_SLAVE_DS2433_CRC
 	kfree(data->memory);
+	bitmap_free(data->validcrc);
 #endif /* CONFIG_W1_SLAVE_DS2433_CRC */
 	kfree(data);
 	sl->family_data = NULL;
@@ -321,11 +365,22 @@ 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_add_slave,
+	.remove_slave   = w1_remove_slave,
+	.groups         = w1_f43_groups,
+};
+
 static struct w1_family w1_family_23 = {
 	.fid = W1_F23_EEPROM_DS2433,
 	.fops = &w1_f23_fops,
 };
 
+static struct w1_family w1_family_43 = {
+	.fid = W1_F43_EEPROM_DS28EC20,
+	.fops = &w1_f43_fops,
+};
+
 static int __init w1_ds2433_init(void)
 {
 	int err;
@@ -334,18 +389,29 @@ static int __init w1_ds2433_init(void)
 	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 <bgardner@wabtec.com>");
-MODULE_DESCRIPTION("w1 family 23 driver for DS2433, 4kb EEPROM");
+MODULE_AUTHOR("Marc Ferland <marc.ferland@sonatest.com>");
+MODULE_DESCRIPTION("w1 family 23/43 driver for DS2433 (4kb) and DS28EC20 (20kb)");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("w1-family-" __stringify(W1_F23_EEPROM_DS2433));
+MODULE_ALIAS("w1-family-" __stringify(W1_F43_EEPROM_DS28EC20));
-- 
2.34.1

Re: [PATCH 6/7] w1: ds2433: add support for ds28ec20 eeprom

[Krzysztof Kozlowski]

On 17/11/2023 20:29, marc.ferland@gmail.com wrote:
> From: Marc Ferland <marc.ferland@sonatest.com>
> 
> The ds28ec20 eeprom is (almost) backward compatible with the
> ds2433. The only major 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.
> 
> Tests:
> 
> 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:
> 
>     #!/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
> 
> Test results (CONFIG_W1_SLAVE_DS2433_CRC is not set):
> 
>    $ cat /proc/config.gz | gunzip | grep CONFIG_W1_SLAVE_DS2433
>    CONFIG_W1_SLAVE_DS2433=m
>    # CONFIG_W1_SLAVE_DS2433_CRC is not set
> 
>     # ./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!
> 
> Test results (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
> 
> Signed-off-by: Marc Ferland <marc.ferland@sonatest.com>
> ---
>  drivers/w1/slaves/w1_ds2433.c | 84 +++++++++++++++++++++++++++++++----
>  1 file changed, 75 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
> index 04c3eee9e5d7..69bdf3dba573 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 <bgardner@wabtec.com>
> + * Copyright (c) 2023 Marc Ferland <marc.ferland@sonatest.com>
>   */
>  
>  #include <linux/kernel.h>
> @@ -23,6 +24,7 @@
>  #include <linux/w1.h>
>  
>  #define W1_F23_EEPROM_DS2433	0x23
> +#define W1_F43_EEPROM_DS28EC20	0x43
>  
>  #define W1_PAGE_SIZE		32
>  #define W1_PAGE_BITS		5
> @@ -45,10 +47,16 @@ static const struct ds2433_config config_f23 = {
>  	.tprog = 5,
>  };
>  
> +static const struct ds2433_config config_f43 = {
> +	.eeprom_size = 2560,
> +	.page_count = 80,
> +	.tprog = 10,
> +};
> +
>  struct w1_data {
>  #ifdef CONFIG_W1_SLAVE_DS2433_CRC
> -	u8	*memory;
> -	u32	validcrc;
> +	u8		*memory;
> +	unsigned long	*validcrc;

Why do you change it? This is actually candidate for its own patch with
its own justification (not "groundwork" but a reason why such code is
better).


Best regards,
Krzysztof

Re: [PATCH 6/7] w1: ds2433: add support for ds28ec20 eeprom

[Marc Ferland]

On Mon, Nov 20, 2023 at 4:26 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 17/11/2023 20:29, marc.ferland@gmail.com wrote:
> > From: Marc Ferland <marc.ferland@sonatest.com>
> >
> > The ds28ec20 eeprom is (almost) backward compatible with the
> > ds2433. The only major 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.
> >
> > Tests:
> >
> > 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:
> >
> >     #!/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
> >
> > Test results (CONFIG_W1_SLAVE_DS2433_CRC is not set):
> >
> >    $ cat /proc/config.gz | gunzip | grep CONFIG_W1_SLAVE_DS2433
> >    CONFIG_W1_SLAVE_DS2433=m
> >    # CONFIG_W1_SLAVE_DS2433_CRC is not set
> >
> >     # ./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!
> >
> > Test results (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
> >
> > Signed-off-by: Marc Ferland <marc.ferland@sonatest.com>
> > ---
> >  drivers/w1/slaves/w1_ds2433.c | 84 +++++++++++++++++++++++++++++++----
> >  1 file changed, 75 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
> > index 04c3eee9e5d7..69bdf3dba573 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 <bgardner@wabtec.com>
> > + * Copyright (c) 2023 Marc Ferland <marc.ferland@sonatest.com>
> >   */
> >
> >  #include <linux/kernel.h>
> > @@ -23,6 +24,7 @@
> >  #include <linux/w1.h>
> >
> >  #define W1_F23_EEPROM_DS2433 0x23
> > +#define W1_F43_EEPROM_DS28EC20       0x43
> >
> >  #define W1_PAGE_SIZE         32
> >  #define W1_PAGE_BITS         5
> > @@ -45,10 +47,16 @@ static const struct ds2433_config config_f23 = {
> >       .tprog = 5,
> >  };
> >
> > +static const struct ds2433_config config_f43 = {
> > +     .eeprom_size = 2560,
> > +     .page_count = 80,
> > +     .tprog = 10,
> > +};
> > +
> >  struct w1_data {
> >  #ifdef CONFIG_W1_SLAVE_DS2433_CRC
> > -     u8      *memory;
> > -     u32     validcrc;
> > +     u8              *memory;
> > +     unsigned long   *validcrc;
>
> Why do you change it? This is actually candidate for its own patch with
> its own justification (not "groundwork" but a reason why such code is
> better).

Noted. Thanks for the review. I'll include the fixes in a v2.

Regards,
Marc

[PATCH 7/7] w1: ds2490: support buffer sizes bigger than 128

[marc.ferland]

From: Marc Ferland <marc.ferland@sonatest.com>

The ds_read_block and ds_write_block functions only support buffer
sizes up to 128 bytes, which is the depth of the fifo on the ds2490.

Sending bigger buffers will fail with a: -110 (ETIMEDOUT) from
usb_control_msg():

    usb 5-1: Failed to write 1-wire data to ep0x2: err=-110.

Also, from the 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 buffer in 128 bytes blocks and simply calling the
original code sequence has solved the issue for me.

Tested with a DS1490F usb<->one-wire adapter and a DS28EC20 eeprom
memory.

Signed-off-by: Marc Ferland <marc.ferland@sonatest.com>
---
 drivers/w1/masters/ds2490.c | 45 ++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/w1/masters/ds2490.c b/drivers/w1/masters/ds2490.c
index 5f5b97e24700..d0a1a0b8b7ff 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(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,7 +639,25 @@ static int ds_read_block(struct ds_device *dev, u8 *buf, int len)
 	return err;
 }
 
-static int ds_write_block(struct ds_device *dev, u8 *buf, int len)
+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(dev, &buf[len - rem], to_read);
+		if (err < 0)
+			return err;
+		rem -= to_read;
+	} while (rem);
+
+	return err;
+}
+
+static int __write_block(struct ds_device *dev, u8 *buf, int len)
 {
 	int err;
 	struct ds_status st;
@@ -665,6 +682,24 @@ static int ds_write_block(struct ds_device *dev, u8 *buf, int len)
 	return !(err == len);
 }
 
+static int ds_write_block(struct ds_device *dev, u8 *buf, int len)
+{
+	int err, to_write, rem = len;
+
+	if (len > 64*1024)
+		return -E2BIG;
+
+	do {
+		to_write = rem <= FIFO_SIZE ? rem : FIFO_SIZE;
+		err = __write_block(dev, &buf[len - rem], to_write);
+		if (err < 0)
+			return err;
+		rem -= to_write;
+	} while (rem);
+
+	return err;
+}
+
 static void ds9490r_search(void *data, struct w1_master *master,
 	u8 search_type, w1_slave_found_callback callback)
 {
-- 
2.34.1

[PATCH v2 0/5] Add support for the ds28ec20 one-wire eeprom

[marc.ferland]

From: Marc Ferland <marc.ferland@sonatest.com>

Hi,

Here is v2 of my ds2433 driver patch series, see [1].

Changes:
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/20231117192909.98944-1-marc.ferland@sonatest.com

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

[PATCH v2 1/5] w1: ds2490: support block sizes larger than 128 bytes in ds_read_block

[marc.ferland]

From: Marc Ferland <marc.ferland@sonatest.com>

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_ arises 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 buffer in 128 bytes blocks 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.

Note: The v1 of this patch changed both the ds_read_block and
ds_write_block functions, but since I don't have any way to test the
'write' part with writes bigger than a page size (maximum accepted by
my eeprom), I preferred not to make any assumptions and I just
removed that part.

Signed-off-by: Marc Ferland <marc.ferland@sonatest.com>
---
 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..b6e244992c15 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(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(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

[PATCH v2 2/5] w1: ds2433: remove unused definition

[marc.ferland]

From: Marc Ferland <marc.ferland@sonatest.com>

W1_F23_TIME isn't used anywhere, get rid of it.

Signed-off-by: Marc Ferland <marc.ferland@sonatest.com>
---
 drivers/w1/slaves/w1_ds2433.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
index 9f21fd98f799..e18523ef8c45 100644
--- a/drivers/w1/slaves/w1_ds2433.c
+++ b/drivers/w1/slaves/w1_ds2433.c
@@ -30,8 +30,6 @@
 #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

[PATCH v2 3/5] w1: ds2433: introduce a configuration structure

[marc.ferland]

From: Marc Ferland <marc.ferland@sonatest.com>

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 <marc.ferland@sonatest.com>
---
 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

[PATCH v2 4/5] w1: ds2433: use the kernel bitmap implementation

[marc.ferland]

From: Marc Ferland <marc.ferland@sonatest.com>

The ds2433 driver uses the 'validcrc' variable to mark out which pages
have been successfully (crc is valid) retrieved from the eeprom and
placed in the internal 'memory' buffer (see CONFIG_W1_SLAVE_DS2433_CRC).

The current implementation assumes that the number of pages will never
go beyond 32 pages (bit field is a u32). This is fine for the ds2433
since it only has 16 pages.

Use a dynamically allocated bitmap so that we can support eeproms with
more than 32 pages which is the case for the ds28ec20 (80 pages).

As an added bonus, the code also gets easier on the eye.

Signed-off-by: Marc Ferland <marc.ferland@sonatest.com>
---
 drivers/w1/slaves/w1_ds2433.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
index 7d4d9fc1a9c4..63ed03191137 100644
--- a/drivers/w1/slaves/w1_ds2433.c
+++ b/drivers/w1/slaves/w1_ds2433.c
@@ -49,8 +49,8 @@ static const struct ds2433_config config_f23 = {
 
 struct w1_f23_data {
 #ifdef CONFIG_W1_SLAVE_DS2433_CRC
-	u8	*memory;
-	u32	validcrc;
+	u8		*memory;
+	unsigned long	*validcrc;
 #endif
 	const struct ds2433_config *cfg;
 };
@@ -77,11 +77,11 @@ static int w1_f23_refresh_block(struct w1_slave *sl, struct w1_f23_data *data,
 	u8	wrbuf[3];
 	int	off = block * W1_PAGE_SIZE;
 
-	if (data->validcrc & (1 << block))
+	if (test_bit(block, data->validcrc))
 		return 0;
 
 	if (w1_reset_select_slave(sl)) {
-		data->validcrc = 0;
+		bitmap_zero(data->validcrc, data->cfg->page_count);
 		return -EIO;
 	}
 
@@ -93,7 +93,7 @@ static int w1_f23_refresh_block(struct w1_slave *sl, struct w1_f23_data *data,
 
 	/* cache the block if the CRC is valid */
 	if (crc16(CRC16_INIT, &data->memory[off], W1_PAGE_SIZE) == CRC16_VALID)
-		data->validcrc |= (1 << block);
+		set_bit(block, data->validcrc);
 
 	return 0;
 }
@@ -208,7 +208,7 @@ static int w1_f23_write(struct w1_slave *sl, int addr, int len, const u8 *data)
 	/* Reset the bus to wake up the EEPROM (this may not be needed) */
 	w1_reset_bus(sl->master);
 #ifdef CONFIG_W1_SLAVE_DS2433_CRC
-	f23->validcrc &= ~(1 << (addr >> W1_PAGE_BITS));
+	clear_bit(addr >> W1_PAGE_BITS, f23->validcrc);
 #endif
 	return 0;
 }
@@ -296,6 +296,13 @@ static int w1_f23_add_slave(struct w1_slave *sl)
 		kfree(data);
 		return -ENOMEM;
 	}
+	data->validcrc = bitmap_zalloc(data->cfg->page_count,
+				       GFP_KERNEL);
+	if (!data->validcrc) {
+		kfree(data->memory);
+		kfree(data);
+		return -ENOMEM;
+	}
 #endif /* CONFIG_W1_SLAVE_DS2433_CRC */
 	sl->family_data = data;
 
@@ -307,6 +314,7 @@ 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(data->memory);
+	bitmap_free(data->validcrc);
 #endif /* CONFIG_W1_SLAVE_DS2433_CRC */
 	kfree(data);
 	sl->family_data = NULL;
-- 
2.34.1

[PATCH v2 5/5] w1: ds2433: add support for ds28ec20 eeprom

[marc.ferland]

From: Marc Ferland <marc.ferland@sonatest.com>

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 <marc.ferland@sonatest.com>
Co-developed-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
---
 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 <bgardner@wabtec.com>
+ * Copyright (c) 2023 Marc Ferland <marc.ferland@sonatest.com>
  */
 
 #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 <bgardner@wabtec.com>");
-MODULE_DESCRIPTION("w1 family 23 driver for DS2433, 4kb EEPROM");
+MODULE_AUTHOR("Marc Ferland <marc.ferland@sonatest.com>");
+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

Re: [PATCH v2 0/5] Add support for the ds28ec20 one-wire eeprom

[Krzysztof Kozlowski]

On 27/11/2023 21:18, marc.ferland@gmail.com wrote:
> From: Marc Ferland <marc.ferland@sonatest.com>
> 
> Hi,
> 
> Here is v2 of my ds2433 driver patch series, see [1].

Do not attach (thread) your patchsets to some other threads (unrelated
or older versions). This buries them deep in the mailbox and might
interfere with applying entire sets.

Best regards,
Krzysztof