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

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:

quoted

On Mon, Feb 17, 2025 at 12:20:13PM +0100, Mathieu Dubois-Briand wrote:

...

quoted

quoted

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.

quoted

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).

quoted

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