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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Feb 17, 2025 at 12:20:13PM +0100, Mathieu Dubois-Briand wrote:
> On Fri Feb 14, 2025 at 4:59 PM CET, Andy Shevchenko wrote:
> > On Fri, Feb 14, 2025 at 12:49:57PM +0100, Mathieu Dubois-Briand wrote:
> > > Add driver for Maxim Integrated MAX7360 GPIO/GPO controller.

...

> > > +static int max7360_gpo_reg_mask_xlate(struct gpio_regmap *gpio,
> > > +				      unsigned int base, unsigned int offset,
> > > +				      unsigned int *reg, unsigned int *mask)
> > > +{
> > > +	u16 ngpios = gpio_regmap_get_ngpio(gpio);
> > > +
> > > +	*reg = base;
> > > +	*mask = BIT(MAX7360_MAX_KEY_COLS - (ngpios - offset));
> > > +
> > > +	return 0;
> >
> > Does this GPIO controller only capable of servicing keypads?
> > I think no, hence I'm not sure why this split is needed to be
> > here and not in the input driver.
> 
> I would say it's more a keypad controller able to support some GPIOs.
> Some of the keypad columns, if unused, can be used as output-only gpios.
> So I believe the split has its place here, because in the default
> configuration, the split is set to have 8 keypad columns and no gpio. As
> a consequence, the keypad driver can work without having to worry about
> the split; the gpio driver needs to know about it.
> 
> To provide a bit more details, there is basically two set of pins usable
> as GPIOs.
> 
> On one side we have what I refer to as GPIOs:
>   - PORT0 to PORT7 pins of the chip.
>   - Shared with PWM and rotary encoder functionalities. Functionality
>     selection can be made independently for each pin. We have to ensure
>     the same pin is not used by two drivers at the same time. E.g. we
>     cannot have at the same time GPIO4 and PWM4.
>   - Supports input and interrupts.
>   - Outputs may be configured as constant current.
>   - 8 GPIOS supported, so ngpios is fixed to MAX7360_MAX_GPIO.
>   - maxim,max7360-gpio compatible, gpio_function == MAX7360_GPIO_PORT.
> 
> On the other side, we have what I refer to as GPOs:
>   - COL2 to COL7 pins of the chip.
>   - Shared with the keypad functionality. Selections is made by
>     partitioning the pins: first pins for keypad columns, last pins for
>     GPOs. Partition is described by the ngpios property.
>   - Only support outputs.
>   - maxim,max7360-gpo compatible, gpio_function == MAX7360_GPIO_COL.
> 
> > Or you mean that there output only GPIO lines in HW after all?
> > Is there a link to the datasheet?
> 
> 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.

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

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.

So, with this approach the entire handling becomes less hackish and quite
straightforward!


-- 
With Best Regards,
Andy Shevchenko