Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>]

On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote:
> Add driver for Maxim Integrated MAX7360 GPIO/GPO controller.
> 
> Two sets of GPIOs are provided by the device:
> - Up to 8 GPIOs, shared with the PWM and rotary encoder functionalities.
>   These GPIOs also provide interrupts on input changes.
> - Up to 6 GPOs, on unused keypad columns pins.

...

> +	depends on OF_GPIO

This is wrong. You don't use it.

...

> +#include <linux/of.h>

Shouldn't be here. Instead it should be linux/property.h.

...

> +	/* MAX7360 generates interrupts but does not report which pins changed:
> +	 * compare the pin value with the value they had in previous interrupt
> +	 * and report interrupt if the change match the set IRQ type.
> +	 */

/*
 * Wrong multi-line comment style. Consider using
 * this one as an example. This applies to all the comments
 * in the entire series.
 */

> +	pending = val ^ max7360_gpio->in_values;
> +	for_each_set_bit(irqn, &pending, max7360_gpio->chip.ngpio) {
> +		type = max7360_gpio->irq_types[irqn];
> +		if (max7360_gpio->masked_interrupts & BIT(irqn))
> +			continue;
> +		if ((val & BIT(irqn)) && type == IRQ_TYPE_EDGE_FALLING)
> +			continue;
> +		if (!(val & BIT(irqn)) && type == IRQ_TYPE_EDGE_RISING)
> +			continue;
> +		gpio_irq = irq_find_mapping(max7360_gpio->chip.irq.domain, irqn);
> +		handle_nested_irq(gpio_irq);
> +		count++;

You can also look how gpio-pca953x.c does this. It uses bitmaps all over the
place. But with the gpio-regmap.c in use this should be much more simpler.

> +	}

> +	max7360_gpio->in_values = val;

> +	if (count == 0)

count is redundant, Checking pending against 0 is enough (or in case of
bitmaps, if it's longer than unsigned long, bitmap_empty() is what is here).

> +		return IRQ_NONE;
> +
> +	return IRQ_HANDLED;

...

> +static int max7360_gpio_probe(struct platform_device *pdev)
> +{
> +	struct max7360_gpio *max7360_gpio;
> +	struct platform_device *parent;
> +	unsigned int ngpios;
> +	unsigned int outconf;
> +	struct gpio_irq_chip *girq;
> +	unsigned long flags;
> +	int irq;
> +	int ret;

> +	if (!pdev->dev.parent) {
> +		dev_err(&pdev->dev, "no parent device\n");
> +		return -ENODEV;
> +	}

Probably redundant as one of the calls below may fail if parent is absent (see
below for more).

> +	parent = to_platform_device(pdev->dev.parent);

Why do you need this? Can't the fwnode be propagated to the children and then
the respective APIs to be used?

> +	max7360_gpio = devm_kzalloc(&pdev->dev, sizeof(*max7360_gpio),
> +				    GFP_KERNEL);

With

	struct device *dev = &pdev->dev;

at the beginning of the function this and other lines will become neater and
shorter, in particular this becomes one line even for the strict 80 characters
limit (however we have it being relaxed to 100 nowadays).

> +	if (!max7360_gpio)
> +		return -ENOMEM;

> +	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
> +		dev_err(&pdev->dev, "Missing ngpios OF property\n");
> +		return -ENODEV;
> +	}

This is not needed, it is already done in GPIOLIB core.

> +	max7360_gpio->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!max7360_gpio->regmap) {
> +		dev_err(&pdev->dev, "could not get parent regmap\n");
> +		return -ENODEV;

Here and everywhere in the ->probe() and related use

	return dev_err_probe(...);

And drop unneeded curly braces.

> +	}

> +	max7360_gpio->dev = &pdev->dev;

So, why do you need this dup? You can easily get it via GPIO chip, no?

> +	max7360_gpio->chip = max7360_gpio_chip;
> +	max7360_gpio->chip.ngpio = ngpios;
> +	max7360_gpio->chip.parent = &pdev->dev;
> +	max7360_gpio->chip.base = -1;
> +	max7360_gpio->gpio_function = (uintptr_t)device_get_match_data(&pdev->dev);
> +
> +	dev_dbg(&pdev->dev, "gpio count: %d\n", max7360_gpio->chip.ngpio);
> +
> +	if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT) {
> +		/* Port GPIOs: set output mode configuration (constant-current
> +		 * or not).
> +		 * This property is optional.
> +		 */
> +		outconf = 0;
> +		ret = of_property_read_u32(pdev->dev.of_node,
> +					   "maxim,constant-current-disable",
> +					   &outconf);

device_property_read_u32()

> +		if (ret && (ret != -EINVAL)) {
> +			dev_err(&pdev->dev,
> +				"Failed to read maxim,constant-current-disable OF property\n");
> +			return -ENODEV;
> +		}
> +
> +	    regmap_write(max7360_gpio->regmap, MAX7360_REG_GPIOOUTM, outconf);
> +	}
> +
> +	if (max7360_gpio->gpio_function == MAX7360_GPIO_PORT &&
> +	    of_property_read_bool(pdev->dev.of_node, "interrupt-controller")) {
> +		/* Port GPIOs: declare IRQ chip, if configuration was provided.
> +		 */

> +		irq = platform_get_irq_byname(parent, "inti");

fwnode_irq_get_byname() with the propagated firmware node will give you
the same result.

> +		if (irq < 0)
> +			return dev_err_probe(&pdev->dev, irq,
> +					     "Failed to get IRQ\n");
> +
> +		flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
> +		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +						max7360_gpio_irq, flags,
> +						"max7360-gpio", max7360_gpio);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "Failed to register interrupt\n");
> +
> +		girq = &max7360_gpio->chip.irq;
> +		gpio_irq_chip_set_chip(girq, &max7360_gpio_irqchip);
> +		girq->parent_handler = NULL;
> +		girq->num_parents = 0;
> +		girq->parents = NULL;
> +		girq->threaded = true;
> +		girq->default_type = IRQ_TYPE_NONE;
> +		girq->handler = handle_simple_irq;
> +	}
> +
> +	ret = devm_gpiochip_add_data(&pdev->dev, &max7360_gpio->chip, max7360_gpio);
> +	if (ret) {
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "unable to add gpiochip\n");
> +	}
> +
> +	return 0;
> +}

...

> +static struct platform_driver max7360_gpio_driver = {
> +	.driver = {
> +		.name	= MAX7360_DRVNAME_GPIO,

> +		.of_match_table = of_match_ptr(max7360_gpio_of_match),

No of_match_ptr() or ACPI_PTR() in new code, please. It appears to be more
harmful than helpful.

> +	},
> +	.probe		= max7360_gpio_probe,
> +};
> +module_platform_driver(max7360_gpio_driver);

...

> +MODULE_ALIAS("platform:" MAX7360_DRVNAME_GPIO);

Why? It doesn't look like it can be instantiated via board files.

...

Overall seems many of my comments are applicable to your entire series (all
patches), please address and feel free to Cc me on v4 for review.

-- 
With Best Regards,
Andy Shevchenko