Re: [PATCH 3/4] misc: eeprom_93xx46: Compute bits based on addrlen

[Jonathan Neuschäfer]

> [PATCH 3/4] misc: eeprom_93xx46: Compute bits based on addrlen

It's not obvious what "bits" and "addrlen" mean, without reading the
code first — can you perhaps rephrase this in a more meaningful (to the
uninitiated) way?

Maybe:   Compute command length based on address length

On Sat, Apr 24, 2021 at 02:30:32PM +0200, Emmanuel Gil Peyrot wrote:
> In the read case, this also moves it out of the loop.

I think this patch could use a slightly longer description:

- What's the rough aim of it?
- Is it purely a refactoring, or does it result in different observable
  behavior?

> 
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@xxxxxxxxxxxx>
> ---
>  drivers/misc/eeprom/eeprom_93xx46.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
> index 39375255e22a..2f4b39417873 100644
> --- a/drivers/misc/eeprom/eeprom_93xx46.c
> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
> @@ -86,6 +86,7 @@ static int eeprom_93xx46_read(void *priv, unsigned int off,
>  	struct eeprom_93xx46_dev *edev = priv;
>  	char *buf = val;
>  	int err = 0;
> +	int bits;
>  
>  	if (unlikely(off >= edev->size))
>  		return 0;
> @@ -99,21 +100,21 @@ static int eeprom_93xx46_read(void *priv, unsigned int off,
>  	if (edev->pdata->prepare)
>  		edev->pdata->prepare(edev);
>  
> +	/* The opcode in front of the address is three bits. */
> +	bits = edev->addrlen + 3;
> +
>  	while (count) {
>  		struct spi_message m;
>  		struct spi_transfer t[2] = { { 0 } };
>  		u16 cmd_addr = OP_READ << edev->addrlen;
>  		size_t nbytes = count;
> -		int bits;
>  
>  		if (edev->addrlen == 7) {
>  			cmd_addr |= off & 0x7f;
> -			bits = 10;
>  			if (has_quirk_single_word_read(edev))
>  				nbytes = 1;
>  		} else {
>  			cmd_addr |= (off >> 1) & 0x3f;
> -			bits = 9;
>  			if (has_quirk_single_word_read(edev))
>  				nbytes = 2;
>  		}

The if/else looks bogus as there are now more than two different address
lengths. This if/else seems to conflate two things:

- how the command/address bits should be shifted around to form a proper
  command
- whether we're dealing with 8-bit or 16-bit words (nbytes = ...)

> @@ -168,13 +169,14 @@ static int eeprom_93xx46_ew(struct eeprom_93xx46_dev *edev, int is_on)
>  	int bits, ret;
>  	u16 cmd_addr;
>  
> +	/* The opcode in front of the address is three bits. */
> +	bits = edev->addrlen + 3;
> +
>  	cmd_addr = OP_START << edev->addrlen;
>  	if (edev->addrlen == 7) {
>  		cmd_addr |= (is_on ? ADDR_EWEN : ADDR_EWDS) << 1;
> -		bits = 10;
>  	} else {
>  		cmd_addr |= (is_on ? ADDR_EWEN : ADDR_EWDS);
> -		bits = 9;
>  	}

dito.

>  
>  	if (has_quirk_instruction_length(edev)) {
> @@ -221,15 +223,16 @@ eeprom_93xx46_write_word(struct eeprom_93xx46_dev *edev,
>  	int bits, data_len, ret;
>  	u16 cmd_addr;
>  
> +	/* The opcode in front of the address is three bits. */
> +	bits = edev->addrlen + 3;
> +
>  	cmd_addr = OP_WRITE << edev->addrlen;
>  
>  	if (edev->addrlen == 7) {
>  		cmd_addr |= off & 0x7f;
> -		bits = 10;
>  		data_len = 1;
>  	} else {
>  		cmd_addr |= (off >> 1) & 0x3f;
> -		bits = 9;
>  		data_len = 2;
>  	}

dito.

>  
> @@ -311,13 +314,14 @@ static int eeprom_93xx46_eral(struct eeprom_93xx46_dev *edev)
>  	int bits, ret;
>  	u16 cmd_addr;
>  
> +	/* The opcode in front of the address is three bits. */
> +	bits = edev->addrlen + 3;
> +
>  	cmd_addr = OP_START << edev->addrlen;
>  	if (edev->addrlen == 7) {
>  		cmd_addr |= ADDR_ERAL << 1;
> -		bits = 10;
>  	} else {
>  		cmd_addr |= ADDR_ERAL;
> -		bits = 9;
>  	}

dito.



Thank you for cleaning up this driver!

Jonathan

Attachment: signature.asc
Description: PGP signature