lkml.org 
[lkml]   [2021]   [Apr]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 1/3] misc: eeprom_93xx46: Remove hardcoded bit lengths
    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@linkmauve.fr>
    > ---

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

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


    > 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
    >
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2021-04-26 09:57    [W:4.754 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site