Re: [PATCH v2 1/3] misc: eeprom_93xx46: Remove hardcoded bit lengths

[Jonathan Neuschäfer]

On Sat, Apr 24, 2021 at 11:25:41PM +0200, Emmanuel Gil Peyrot wrote:
> This avoids using magic numbers based on the length of an address or a
> command, while we only want to differentiate between 8-bit and 16-bit.
> 
> The driver was previously wrapping around the offset in the write
> operation, this now returns -EINVAL instead (but should never happen in
> the first place).
> 
> If two pointer indirections are too many, we could move the flags to the
> main struct instead, but I doubt it’s going to make any sensible
> difference on any hardware.
> 
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@xxxxxxxxxxxx>
> ---

I'm not an EEPROM driver expert, but FWIW:

Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@xxxxxxx>


>  drivers/misc/eeprom/eeprom_93xx46.c | 57 ++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
> index 80114f4c80ad..ffdb8e5a26e0 100644
> --- a/drivers/misc/eeprom/eeprom_93xx46.c
> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
> @@ -9,6 +9,7 @@
>  #include <linux/device.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
> +#include <linux/log2.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/of.h>
> @@ -70,6 +71,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;
> @@ -83,21 +85,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 (edev->pdata->flags & EE_ADDR8) {
> +			cmd_addr |= off;
>  			if (has_quirk_single_word_read(edev))
>  				nbytes = 1;
>  		} else {
> -			cmd_addr |= (off >> 1) & 0x3f;
> -			bits = 9;
> +			cmd_addr |= (off >> 1);
>  			if (has_quirk_single_word_read(edev))
>  				nbytes = 2;
>  		}
> @@ -152,14 +154,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) {
> +	if (edev->pdata->flags & EE_ADDR8)
>  		cmd_addr |= (is_on ? ADDR_EWEN : ADDR_EWDS) << 1;
> -		bits = 10;
> -	} else {
> +	else
>  		cmd_addr |= (is_on ? ADDR_EWEN : ADDR_EWDS);
> -		bits = 9;
> -	}
>  
>  	if (has_quirk_instruction_length(edev)) {
>  		cmd_addr <<= 2;
> @@ -205,15 +207,19 @@ eeprom_93xx46_write_word(struct eeprom_93xx46_dev *edev,
>  	int bits, data_len, ret;
>  	u16 cmd_addr;
>  
> +	if (unlikely(off >= edev->size))
> +		return -EINVAL;
> +
> +	/* 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;
> +	if (edev->pdata->flags & EE_ADDR8) {
> +		cmd_addr |= off;
>  		data_len = 1;
>  	} else {
> -		cmd_addr |= (off >> 1) & 0x3f;
> -		bits = 9;
> +		cmd_addr |= (off >> 1);
>  		data_len = 2;
>  	}
>  
> @@ -253,7 +259,7 @@ static int eeprom_93xx46_write(void *priv, unsigned int off,
>  		return count;
>  
>  	/* only write even number of bytes on 16-bit devices */
> -	if (edev->addrlen == 6) {
> +	if (edev->pdata->flags & EE_ADDR16) {
>  		step = 2;
>  		count &= ~1;
>  	}
> @@ -295,14 +301,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) {
> +	if (edev->pdata->flags & EE_ADDR8)
>  		cmd_addr |= ADDR_ERAL << 1;
> -		bits = 10;
> -	} else {
> +	else
>  		cmd_addr |= ADDR_ERAL;
> -		bits = 9;
> -	}
>  
>  	if (has_quirk_instruction_length(edev)) {
>  		cmd_addr <<= 2;
> @@ -455,10 +461,12 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
>  	if (!edev)
>  		return -ENOMEM;
>  
> +	edev->size = 128;
> +
>  	if (pd->flags & EE_ADDR8)
> -		edev->addrlen = 7;
> +		edev->addrlen = ilog2(edev->size);
>  	else if (pd->flags & EE_ADDR16)
> -		edev->addrlen = 6;
> +		edev->addrlen = ilog2(edev->size) - 1;
>  	else {
>  		dev_err(&spi->dev, "unspecified address type\n");
>  		return -EINVAL;
> @@ -469,7 +477,6 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
>  	edev->spi = spi;
>  	edev->pdata = pd;
>  
> -	edev->size = 128;
>  	edev->nvmem_config.type = NVMEM_TYPE_EEPROM;
>  	edev->nvmem_config.name = dev_name(&spi->dev);
>  	edev->nvmem_config.dev = &spi->dev;
> -- 
> 2.31.1
>

Attachment: signature.asc
Description: PGP signature