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

[Krzysztof Kozlowski]

On 17/11/2023 20:29, marc.ferland@xxxxxxxxx wrote:
> From: Marc Ferland <marc.ferland@xxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>
> + * Copyright (c) 2023 Marc Ferland <marc.ferland@xxxxxxxxxxxx>
>   */
>  
>  #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