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

["Mathieu Dubois-Briand" <mathieu.dubois-briand@xxxxxxxxxxx>]

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

>
> > +}
> > +
> > +static int max7360_gpio_request(struct gpio_chip *gc, unsigned int pin)
> > +{
> > +	/*
> > +	 * GPIOs on PORT pins are shared with the PWM and rotary encoder
> > +	 * drivers: they have to be requested from the MFD driver.
> > +	 */
>
> So, this sounds to me like a pin control approach is needed here.
> This looks like an attempt to hack it in an "easy" way.
>

Linus Walleij had a similar comment on v3, but said he thought it was
fine here. Still, I'm open to discussion.

> > +	return max7360_port_pin_request(gc->parent->parent, pin, true);
> > +}
> > +
> > +static void max7360_gpio_free(struct gpio_chip *gc, unsigned int pin)
> > +{
> > +	max7360_port_pin_request(gc->parent->parent, pin, false);
> > +}
> > +
> > +static int max7360_set_gpos_count(struct device *dev, struct regmap *regmap)
> > +{
> > +	/*
> > +	 * MAX7360 COL0 to COL7 pins can be used either as keypad columns,
> > +	 * general purpose output or a mix of both.
> > +	 */
> > +	unsigned int val;
> > +	u32 columns;
> > +	u32 ngpios;
> > +	int ret;
> > +
> > +	ret = device_property_read_u32(dev, "ngpios", &ngpios);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Missing ngpios OF property\n");
>
> Clean messages from OF, "device property" is established term.
>

Yes

> > +		return ret;
> > +	}
> > +
> > +	/*
> > +	 * Get the number of pins requested by the keypad and ensure our own pin
> > +	 * count is compatible with it.
> > +	 */
> > +	ret = device_property_read_u32(dev->parent, "keypad,num-columns", &columns);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to read columns count\n");
> > +		return ret;
> > +	}
> > +
> > +	if (ngpios > MAX7360_MAX_GPO ||
> > +	    (ngpios + columns > MAX7360_MAX_KEY_COLS)) {
> > +		dev_err(dev, "Incompatible gpos and columns count (%u, %u)\n",
> > +			ngpios, columns);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * 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?
>

My understanding of the set_config() callback is that it's meant to set
the configuration of a single line. Here the configuration applies to
the whole chip.

> > +	return 0;
> > +}
>
> ...
>
> > +		if (irq < 0)
> > +			return dev_err_probe(dev, irq, "Failed to get IRQ\n");
> > +
> > +		irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL);
> > +		if (!irq_chip)
> > +			return -ENOMEM;
> > +
> > +		irq_chip->name = dev_name(dev);
> > +		irq_chip->status_base = MAX7360_REG_GPIOIN;
> > +		irq_chip->num_regs = 1;
> > +		irq_chip->num_irqs = MAX7360_MAX_GPIO;
> > +		irq_chip->irqs = max7360_regmap_irqs;
> > +		irq_chip->handle_mask_sync = max7360_handle_mask_sync;
> > +		irq_chip->status_is_level = true;
> > +		irq_chip->irq_drv_data = regmap;
> > +
> > +		for (unsigned int i = 0; i < MAX7360_MAX_GPIO; i++) {
> > +			regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i),
> > +					  MAX7360_PORT_CFG_INTERRUPT_EDGES,
> > +					  MAX7360_PORT_CFG_INTERRUPT_EDGES);
> > +		}
> > +
> > +		flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
> > +		ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), regmap, irq, flags, 0,
> > +						      irq_chip, &irq_chip_data);
>
> Right.
>
> What I mean in previous discussion is to update gpio-regmap to call this from inside.
> You need to add irq_chip pointer and irq_chip_data pointer to the regmap configuration
> and if they are set (or the first one, I dunno if this is supported by IRQ chip core)
> call this function and assign domain. This should be called after GPIO chip is
> added, but before IRQ domain attachment.
>

OK, I believe I got it now. I will try to work on it in the coming days.

> > +
> > +		regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf);
> > +	}
> > +
> > +	/* Add gpio device. */
> > +	gpio_config.parent = dev;
> > +	gpio_config.regmap = regmap;
>
> > +	if (gpio_function == MAX7360_GPIO_PORT) {
> > +		gpio_config.ngpio = MAX7360_MAX_GPIO;
>
> Why this case can't be managed also via ngpios property? Maybe at the end of
> the day you rather need to have another property to tell where the split is?
>
> This will help a lot and removes unneeded sharing of ngpios here and there.
>
> What I read from this code is like you are trying to put _two_in_one_ semantics
> on the shoulders of "ngpios".
>

It could be managed with ngpios, just there is no specific need as we
will always have 8 gpios here. With (gpio_function ==
MAX7360_GPIO_PORT), there is no split and starting from this version of
my series, there is no reuse on ngpios property.

The split and reuse of ngpios is only used for GPO on keypad columns
(gpio_function == MAX7360_GPIO_COL).

We can introduce a new property to tell where the split is, but the
number of gpio is a direct consequence of the position of the split.


-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com