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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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/



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux