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

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

On Mon, Mar 17, 2025 at 03:13:07PM +0100, Mathieu Dubois-Briand wrote:
> On Fri Mar 14, 2025 at 9:14 AM CET, Andy Shevchenko wrote:
> > Thu, Mar 13, 2025 at 06:07:03PM +0100, Mathieu Dubois-Briand kirjoitti:
> > > 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:

...

> > > > > +	/*
> > > > > +	 * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce
> > > > > +	 * timings and gpos/keypad columns repartition. Only the later is
> > > > > +	 * modified here.
> > > > > +	 */
> > > > > +	val = FIELD_PREP(MAX7360_PORTS, ngpios);
> > > > > +	ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val);
> > > > > +	if (ret) {
> > > > > +		dev_err(dev, "Failed to write max7360 columns/gpos configuration");
> > > > > +		return ret;
> > > > > +	}
> > > >
> > > > Shouldn't this be configured via ->set_config() callback?
> > > 
> > > I believe this comment has been a bit outdated by our discussion on
> > > using GPIO valid mask, but I believe we could not use the ->set_config()
> > > callback here: this callback is made to configure a single pin while the
> > > gpos/keypad columns repartition is global.
> >
> > Yeah, we have similar desing in Intel Bay Trail (see pinctrl-baytrail.c) and it
> > requires some software driven heuristics on how individual setting may affect
> > the global one. But the Q here is is the debounce affects only keypad? Then it
> > should be configured via keypad matrix driver. Btw, have you checked
> > drivers/input/keyboard/matrix_keypad.c? Is there anything that can be useful
> > here?
> >
> 
> Hum, maybe the comment is not clear enough? Not sure, but please tell
> me.

I see it now, yes, the comment seems point too much attention on the register
(and hence its name) then content.

I would start this comment with something like:
"Configure which GPIOs will be used for keypad."

> So yes, this register is named "debounce" but controls two different
> things:
> - The keypad debounce: we do not touch it here.
> - The partition between keypad columns and gpos. This is the value we do
>   modify here.

-- 
With Best Regards,
Andy Shevchenko