Re: [PATCH v11 char-misc-next 2/2] misc: microchip: pci1xxxx: Add support to read and write into PCI1XXXX EEPROM via NVMEM sysfs

[<VaibhaavRam.TL@xxxxxxxxxxxxx>]

On Mon, 2023-05-08 at 08:44 +0200, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Hi,
> 
> > @@ -219,6 +348,22 @@ static int pci1xxxx_otp_eeprom_probe(struct
> > auxiliary_device *aux_dev,
> >               return -ENOMEM;
> > 
> >       priv->pdev = aux_dev;
> > +     priv->nvmem_config_eeprom.type = NVMEM_TYPE_EEPROM;
> > +     priv->nvmem_config_eeprom.name = EEPROM_NAME;
> > +     priv->nvmem_config_eeprom.dev = &aux_dev->dev;
> > +     priv->nvmem_config_eeprom.owner = THIS_MODULE;
> > +     priv->nvmem_config_eeprom.reg_read = pci1xxxx_eeprom_read;
> > +     priv->nvmem_config_eeprom.reg_write = pci1xxxx_eeprom_write;
> > +     priv->nvmem_config_eeprom.priv = priv;
> > +     priv->nvmem_config_eeprom.stride = 1;
> > +     priv->nvmem_config_eeprom.word_size = 1;
> > +     priv->nvmem_config_eeprom.size = EEPROM_SIZE_BYTES;
> > +
> > +     priv->nvmem_eeprom = devm_nvmem_register(&aux_dev->dev,
> > +                                              &priv-
> > >nvmem_config_eeprom);
> > +     if (!priv->nvmem_eeprom)
> > +             return -ENOMEM;
> > +
> >       priv->nvmem_config_otp.type = NVMEM_TYPE_OTP;
> >       priv->nvmem_config_otp.name = OTP_NAME;
> >       priv->nvmem_config_otp.dev = &aux_dev->dev;
> > @@ -258,6 +403,9 @@ static int pci1xxxx_otp_eeprom_probe(struct
> > auxiliary_device *aux_dev,
> > 
> >       dev_set_drvdata(&aux_dev->dev, priv);
> > 
> > +     if (is_eeprom_responsive(priv))
> > +             priv->is_eeprom_present = true;
> 
> What's this? The eeprom isn't there (or in whatever state), then you
> still register the nvmem device, but read and write doesn't do
> anything
> useful. You shouldn't register the device in the first place if it
> is not functional.

Actually, in previous architectures, for checking bin attributes, this
flag is used. But I think, this can be removed in this arch. Thanks