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

[Michael Walle]

Am 2023-04-24 17:39, schrieb Sahin, Okan:
> > Am 2023-04-09 16:25, schrieb Sahin, Okan:
> > > > Fri, Apr 07, 2023 at 03:48:25PM +0200, Linus Walleij kirjoitti:
> > > > > On Wed, Apr 5, 2023 at 3:57 PM Michael Walle <michael@xxxxxxxx>
> > > > > wrote:
> > > > >
> > > > > > OTOH I'm not sure the driver is doing it correctly, because it also
> > > > > > seems to switch the pullup resisters together with the direction.
> > > > > > I'm not sure that is correct. So there might be just one register
> > > > > > involved after all and the GPIO_REGMAP should work again.
> > > > >
> > > > > I'm pretty sure that should be in the .set_config() callback.
> > > > >
> > > > > > Also, according to the datasheet this has some nv memory (to set the
> > > > > > initial state of the GPIOs [?]). So it should really be a
> > > > > > multi-function device. I'm not sure if this has to be considered
> > > > > > right from the beginning or if the device support can start with
> > > > > > GPIO only and later be transitioned to a full featured MFD (probably with
> > nvmem
> > > > support).
> > > > > That's a bit of a soft definition.
> > > > >
> > > > > If the chip is *only* doing GPIO and nvram it can be a GPIO-only
> > > > > device I think.
> > > > >
> > > > > 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).
> >
> > You mean the gpio driver is calling nvmem_register()? Yeah I agree, 
> > that
> > should work.
> >
> > > I think gpio_regmap is not suitable for this driver as Michael 
> > > stated.
> > > https://www.analog.com/media/en/technical-documentation/data-
> > sheets/ds4520.pdf
> > > Please check block diagram. There are two input registers that 
> > > control
> > > gpio state
> > > so gpio_regmap does not look ok for this. Am I missing something?
> >
> > You mean F8/F9? That will work as they are for different GPIOs. What
> > doesn't work with gpio-regmap is when you need to modify two different
> > registers for one GPIO. Have a look at gpio_regmap_get() and
> > gpio_regmap_set(). If the default gpio_regmap_simple_xlate() doesn't
> > work
> > you can use your own .xlate() op.
>
> Actually, I checked the functions that you suggested, but as far as I 
> understand
> they might work if there would be one bit to set direction or value. 
> However,
> this is not the case for ds4520. In other words, if I want to set the
> gpio direction
> as output, I need to set a corresponding bit for both F0 and F1 
> registers.

I can't follow. F0/F1 is for the pull-up. That was actually my initial
question and Linus said, that should probably be done in a seperate
.set_config operation not together with a direction change.

> In the document, you can see block diagram. I do not know why, but 
> design is
> not standard that’s why I think I can not use gpio-regmap.
>
> > > Also, at this point I am not planning to add nvmem support.
> >
> > That is a pity, because that is the whole use case for this gpio
> > expander,
> > no? "Programmable Replacement for Mechanical Jumpers and Switches"
>
> I can set "SEE" bit as "0" in the Configuration Register to write
> EEPROM so it might solve
> issue.

If you do that unconditionally, that might wear out the EEPROM,
though.

-michael