Re: [PATCH v2 2/2] gpio: max7317: Add gpio expander driver

[Michael Walle]

Am 2023-05-03 10:37, schrieb Edmund Berenson:
> On 4/4/23 16:05, Linus Walleij wrote:
> > On Mon, Apr 3, 2023 at 1:41 PM Edmund Berenson
> > <edmund.berenson@xxxxxxxxx> wrote:
> >
> > > Add driver for maxim MAX7317 SPI-Interfaced 10 Port
> > > GPIO Expander.
> > >
> > > v2: adjust driver to use regmap
> > >
> > > Co-developed-by: Lukasz Zemla <Lukasz.Zemla@xxxxxxxxxxxx>
> > > Signed-off-by: Lukasz Zemla <Lukasz.Zemla@xxxxxxxxxxxx>
> > > Signed-off-by: Edmund Berenson <edmund.berenson@xxxxxxxxx>
> >
> > Notwithstanding the other comments from Bartosz, this seems like
> > a driver that should be using the regmap GPIO helper library.
> > git grep GPIO_REGMAP will show you examples of other drivers
> > that use this and how it is used.
> >
> > Yours,
> > Linus Walleij
>
> Hi,
>
> thanks for the review and suggestion. I tried following your suggestion 
> and use
> GPIO_REGMAP to implement the driver.
>
> Unfortunately I ran into two issues
> 1. reg_set_base == 0: for the devcie reg_set base is 0x0. In 
> gpio-regmap there
> are several tests for !reg_set_base. There doesn't seem a way to 
> distinguish
> between is set to 0 and is not set. :)

That's what GPIO_REGMAP_ADDR(addr) is for :)

> 2. input/output direction: to set a gpio pin to input one has to write 
> 0x1 to
> the corresponding output register. The issue starts when I configure a 
> port to
> be an output, set output to 0x1, check the direction of the pin, doing 
> so trough
> sysfs the system will now assume the pin is an input and I can't set 
> its values
> anymore. Avoiding this I would like to track the direction of the pin 
> separately
> from the device register, which is atm done in the corresponding 
> bespoke in/out
> functions.

I can't follow you here exactly. But I've briefly looked at the 
datasheet
and the output is an open drain one. Just like the one we are currently
discussing in [1]. Here too, there seems to be no direction register,
although it is mentioned in the datasheet. But Table 1. Register Address 
Map
is super confusing, so please correct me if I'm wrong.

Since we now have already two different chips with this OD output and 
always
active input buffer, it makes sense to add support to gpio-regmap for 
these
kind of devices. To me it is still unclear wether we need the direction 
at
all. Linus answered my question yesterday, but I haven't found time to 
dig
into that topic myself. Please go ahead and make some suggestions :)

> I could probably solve both of these issues trough the reg_mask_xlate 
> function
> but I believe this would introduce unneeded obscurity in the driver.
>
> I do not believe there are any other easy obvious/better fixes for 
> this. (or
> maybe you prove me wrong :))
> Would you be okay for this driver to stick with direct regmap usage? 
> (obviously
> fixing the review suggestions)

-michael

[1] https://lore.kernel.org/r/20230502084406.3529645-1-michael@xxxxxxxx/