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@xxxxxxxxxx> wrote:
>
> 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).

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

Regards,
Marc