Re: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Sun, Apr 9, 2023 at 5:25 PM Sahin, Okan <Okan.Sahin@analog.com> wrote:
> >Fri, Apr 07, 2023 at 03:48:25PM +0200, Linus Walleij kirjoitti:
> >> On Wed, Apr 5, 2023 at 3:57 PM Michael Walle <michael@walle.cc> wrote:

...

> >> If the chip is *only* doing GPIO and nvram it can be a GPIO-only
> >> device I think.

I have read a datasheet, it has the pre-boot settings, but it doesn't
matter from the Linux point of view. The only thing which we need to
take care of is to have the EEPROM disabled during GPIO interaction.
However, there is a question as to how this device should actually
commit into EEPROM the state to survive the cold/warm power cycle.
What is the persistent flag for, btw, I haven't been familiar with it?

> >> The precedent is a ton of ethernet drivers with nvram for storing e.g.
> >> the MAC address. We don't make all of those into MFDs, as the nvram is
> >> closely tied to the one and only function of the block.
> >
> >I agree with Linus. This should be part of the actual (main) driver for the chip as many
> >do (like USB to serial adapters that have GPIO capability).
> >Also this code lacks of proper locking and has style issues.

...

> Could you give more detail related to locking and style issues?

You use a few IO accessors in a row without locking, which means at
any point of this flow some other CPUs may access the chip using the
same or other APIs of this driver.

-- 
With Best Regards,
Andy Shevchenko