Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 26 Apr 2021 09:46:21 +0200
From:   Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>
To:     Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.ne@posteo.net>,
        linux-kernel@vger.kernel.org,
        Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>,
        Rob Herring <robh+dt@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Aswath Govindraju <a-govindraju@ti.com>,
        Vadym Kochan <vadym.kochan@plvision.eu>,
        devicetree@vger.kernel.org
Subject: Re: [PATCH v2 1/3] misc: eeprom_93xx46: Remove hardcoded bit lengths
Message-ID: <YIZvzVDGaVS2M/ow@latitude>
References: <20210424212543.13929-1-linkmauve@linkmauve.fr>
 <20210424212543.13929-2-linkmauve@linkmauve.fr>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="oKEwqg3MVUhuPv5O"
Content-Disposition: inline
In-Reply-To: <20210424212543.13929-2-linkmauve@linkmauve.fr>
Precedence: bulk


--oKEwqg3MVUhuPv5O
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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).
>=20
> If two pointer indirections are too many, we could move the flags to the
> main struct instead, but I doubt it=E2=80=99s going to make any sensible
> difference on any hardware.
>=20
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---

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

Reviewed-by: Jonathan Neusch=C3=A4fer <j.neuschaefer@gmx.net>


>  drivers/misc/eeprom/eeprom_93xx46.c | 57 ++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 25 deletions(-)
>=20
> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/ee=
prom_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 =3D priv;
>  	char *buf =3D val;
>  	int err =3D 0;
> +	int bits;
> =20
>  	if (unlikely(off >=3D edev->size))
>  		return 0;
> @@ -83,21 +85,21 @@ static int eeprom_93xx46_read(void *priv, unsigned in=
t off,
>  	if (edev->pdata->prepare)
>  		edev->pdata->prepare(edev);
> =20
> +	/* The opcode in front of the address is three bits. */
> +	bits =3D edev->addrlen + 3;
> +
>  	while (count) {
>  		struct spi_message m;
>  		struct spi_transfer t[2] =3D { { 0 } };
>  		u16 cmd_addr =3D OP_READ << edev->addrlen;
>  		size_t nbytes =3D count;
> -		int bits;
> =20
> -		if (edev->addrlen =3D=3D 7) {
> -			cmd_addr |=3D off & 0x7f;
> -			bits =3D 10;
> +		if (edev->pdata->flags & EE_ADDR8) {
> +			cmd_addr |=3D off;
>  			if (has_quirk_single_word_read(edev))
>  				nbytes =3D 1;
>  		} else {
> -			cmd_addr |=3D (off >> 1) & 0x3f;
> -			bits =3D 9;
> +			cmd_addr |=3D (off >> 1);
>  			if (has_quirk_single_word_read(edev))
>  				nbytes =3D 2;
>  		}
> @@ -152,14 +154,14 @@ static int eeprom_93xx46_ew(struct eeprom_93xx46_de=
v *edev, int is_on)
>  	int bits, ret;
>  	u16 cmd_addr;
> =20
> +	/* The opcode in front of the address is three bits. */
> +	bits =3D edev->addrlen + 3;
> +
>  	cmd_addr =3D OP_START << edev->addrlen;
> -	if (edev->addrlen =3D=3D 7) {
> +	if (edev->pdata->flags & EE_ADDR8)
>  		cmd_addr |=3D (is_on ? ADDR_EWEN : ADDR_EWDS) << 1;
> -		bits =3D 10;
> -	} else {
> +	else
>  		cmd_addr |=3D (is_on ? ADDR_EWEN : ADDR_EWDS);
> -		bits =3D 9;
> -	}
> =20
>  	if (has_quirk_instruction_length(edev)) {
>  		cmd_addr <<=3D 2;
> @@ -205,15 +207,19 @@ eeprom_93xx46_write_word(struct eeprom_93xx46_dev *=
edev,
>  	int bits, data_len, ret;
>  	u16 cmd_addr;
> =20
> +	if (unlikely(off >=3D edev->size))
> +		return -EINVAL;
> +
> +	/* The opcode in front of the address is three bits. */
> +	bits =3D edev->addrlen + 3;
> +
>  	cmd_addr =3D OP_WRITE << edev->addrlen;
> =20
> -	if (edev->addrlen =3D=3D 7) {
> -		cmd_addr |=3D off & 0x7f;
> -		bits =3D 10;
> +	if (edev->pdata->flags & EE_ADDR8) {
> +		cmd_addr |=3D off;
>  		data_len =3D 1;
>  	} else {
> -		cmd_addr |=3D (off >> 1) & 0x3f;
> -		bits =3D 9;
> +		cmd_addr |=3D (off >> 1);
>  		data_len =3D 2;
>  	}
> =20
> @@ -253,7 +259,7 @@ static int eeprom_93xx46_write(void *priv, unsigned i=
nt off,
>  		return count;
> =20
>  	/* only write even number of bytes on 16-bit devices */
> -	if (edev->addrlen =3D=3D 6) {
> +	if (edev->pdata->flags & EE_ADDR16) {
>  		step =3D 2;
>  		count &=3D ~1;
>  	}
> @@ -295,14 +301,14 @@ static int eeprom_93xx46_eral(struct eeprom_93xx46_=
dev *edev)
>  	int bits, ret;
>  	u16 cmd_addr;
> =20
> +	/* The opcode in front of the address is three bits. */
> +	bits =3D edev->addrlen + 3;
> +
>  	cmd_addr =3D OP_START << edev->addrlen;
> -	if (edev->addrlen =3D=3D 7) {
> +	if (edev->pdata->flags & EE_ADDR8)
>  		cmd_addr |=3D ADDR_ERAL << 1;
> -		bits =3D 10;
> -	} else {
> +	else
>  		cmd_addr |=3D ADDR_ERAL;
> -		bits =3D 9;
> -	}
> =20
>  	if (has_quirk_instruction_length(edev)) {
>  		cmd_addr <<=3D 2;
> @@ -455,10 +461,12 @@ static int eeprom_93xx46_probe(struct spi_device *s=
pi)
>  	if (!edev)
>  		return -ENOMEM;
> =20
> +	edev->size =3D 128;
> +
>  	if (pd->flags & EE_ADDR8)
> -		edev->addrlen =3D 7;
> +		edev->addrlen =3D ilog2(edev->size);
>  	else if (pd->flags & EE_ADDR16)
> -		edev->addrlen =3D 6;
> +		edev->addrlen =3D 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 =3D spi;
>  	edev->pdata =3D pd;
> =20
> -	edev->size =3D 128;
>  	edev->nvmem_config.type =3D NVMEM_TYPE_EEPROM;
>  	edev->nvmem_config.name =3D dev_name(&spi->dev);
>  	edev->nvmem_config.dev =3D &spi->dev;
> --=20
> 2.31.1
>=20

--oKEwqg3MVUhuPv5O
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEvHAHGBBjQPVy+qvDCDBEmo7zX9sFAmCGb8UACgkQCDBEmo7z
X9tjZBAAyxa25kPUJbax5Dzsz5lhOgS4PpOVUY2tf+YlwG+OauEnXzpVx+WECCvi
AiQlzf8QqVx4xna55KXwzyBEsjjeiMAjAyJV+IKWqhMnWJzjB69jkT+tbMHDzmS0
D6o2/CpiiZJ+uJEmfeh69q9yyOM5Z2Dg6JT33x7XlXU/0oEFyBbjMGKeu51WEhVc
UkP2E1LGySDdDn/qpVGmxn58n+zQckFKKeGBx6A6XIBNsEovK6wSwPgqGFcEaYtx
tpSzxTMlAHyoKKPCEpOsBbWR8WrBkTVKR73RbthImAfBUSQHH1s6+Hyx8Dg4V+iY
8O6O1oQ97VKAc9SBnS2nFBs+iJrS9E0CpLQxzS3Xa+gpAU+jiUVTP9GjB6AkqQIo
kf3Hd2Vxu1o7UXgo/WP3A4YY1gmX1cvOAyBeKCryfZfoIv72yUj+tH1jXvEAVoS3
kfdMqifPqOZl6oYYG0RQkp7fOcXblqaKbMJ+6baA7/cdi39rEgWpdv2N6ozsatbP
dLpAlpbfpBEUa8k4doIvD6+eFmW9nORcwS66d1FwVG4hXqF8zfLkjd24SYx2MV/e
plqP6CVY0XB+75JIfnoagR1viXdPbV9jBcSwkB94mVcp4yHRK6awr0emeutND9aq
TsPGNgQ2eqR3B5ifldiexzKICjKU1GFegkM5yhWZxmoTuju30XI=
=H6un
-----END PGP SIGNATURE-----

--oKEwqg3MVUhuPv5O--