Re: [PATCH v4 07/10] gpio: max7360: Add MAX7360 gpio support

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

Thu, Mar 13, 2025 at 05:43:00PM +0100, Mathieu Dubois-Briand kirjoitti:
> On Mon Feb 17, 2025 at 9:08 PM CET, Andy Shevchenko wrote:
> > On Mon, Feb 17, 2025 at 12:20:13PM +0100, Mathieu Dubois-Briand wrote:

...

> > > A datasheet is available on https://www.analog.com/en/products/max7360.html
> >
> > Thank you for this good elaboration!
> > I will check on the datasheet later on, having one week off.

Note, I have only briefly looked at it, not a deep study and TBH I am not sure
I will have time to invest into that.

> Thanks for your feedback! Sorry I haven't been able to work on this
> series for the last few weeks, but I finally had the opportunity to
> integrate your comments.

No rush, this will miss v6.15 anyway, so we still have a couple of months.

> > But what I have read above sounds to me like the following:
> >
> > 1) the PORT0-PORT7 should be just a regular pin control with the respective
> > function being provided (see pinctrl-cy8c95x0.c as an example);
> 
> Ok, so I created a pin control driver for the PORT pins. This will
> effectively help to prevent concurrent use of pins in place of the
> request()/free() callbacks.
> 
> My only concern is: as there is no real pin muxing on the chip, my
> .set_mux callabck in pinmux_ops structure is not doing anything. It
> looks like I'm not the only one
> (drivers/pinctrl/pinctrl-microchip-sgpio.c does the same thing), but I
> hope this is OK.

Hmm... This is strange. The PWM/GPIO block has 3 functions (GPIO/PWM/rotary),
How comes you have no switch between them?

As far as I read in the datasheet this is controlled by register 0x40
(and seems implicitly by other registers when it's in PWM mode).

> > 2) the COL2 COL7 case can be modeled as a simplest GPIO (GPO) driver with
> > reserved lines property (this will set valid mask and let GPIOLIB to refuse any
> > use of the keypad connected pins.
> 
> I mostly went that way, just a few notes.
> 
> I chose to not use the reserved lines property in the device tree, but
> instead implemented a gpiolib init_valid_mask() callback. In believe
> this is better, as:
> - We can automatically generate the valid gpios mask, based on the
>   number of columns used.
> - It allows to get rid of the compatibility check between the number of
>   columns and the number of GPIOs provided by the device tree: DT
>   provides the number of columns, we deduct the number of GPIOs.

If I understood it correctly it should work as well. But let's discuss that
when you issue a new version.

> I chose to number GPIOs from 0 to 7.
> - This might be a bit questionable, as GPIO 0 and 1 will always be
>   invalid: pins 0 and 1 of the chip cannot be used as GPIOs. I'm
>   definitely open to discussion on this point.
> - Yet I believe it simplifies everything for the user: pin numbers and
>   GPIO numbers are the same instead of having an offset of 2.
> - It also simplifies a bit the GPIO driver code.

In general you should follow the datasheet and mask the GPIOs that may not be
uses as a such due to HW limitation / specific configuration.

-- 
With Best Regards,
Andy Shevchenko