Re: [PATCH v6 03/12] pinctrl: Add MAX7360 pinctrl driver

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

On Wed Apr 9, 2025 at 4:55 PM CEST, Mathieu Dubois-Briand wrote:
> Add driver for Maxim Integrated MAX7360 pinctrl on the PORT pins. Pins
> can be used either for GPIO, PWM or rotary encoder functionalities.
>
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ...
> diff --git a/drivers/pinctrl/pinctrl-max7360.c b/drivers/pinctrl/pinctrl-max7360.c
> ...
> +static int max7360_pinctrl_probe(struct platform_device *pdev)
> +{
> +	struct regmap *regmap;
> +	struct pinctrl_desc *pd;
> +	struct max7360_pinctrl *chip;
> +	struct device *dev = &pdev->dev;
> +
> +	regmap = dev_get_regmap(dev->parent, NULL);
> +	if (!regmap)
> +		dev_err_probe(dev, -ENODEV, "Could not get parent regmap\n");
> +
> +	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	pd = &chip->pinctrl_desc;
> +
> +	pd->pctlops = &max7360_pinctrl_ops;
> +	pd->pmxops = &max7360_pmxops;
> +	pd->name = dev_name(dev);
> +	pd->pins = max7360_pins;
> +	pd->npins = MAX7360_MAX_GPIO;
> +	pd->owner = THIS_MODULE;
> +
> +	device_set_of_node_from_dev(dev, dev->parent);

Ok, so this goes a bit against what I said I was going to do on my
previous series, let me explain why. Same reasoning applies for both
uses, in PWM and pinctrl drivers.

With my previous experiments, I came to the conclusion that:
- Either we should use device_set_of_node_from_dev() as I do here.
- Or we should add more subnodes in the device tree binding.
- Also, copying the fwnode with device_set_node() was not possible, as
  the kernel would then try to apply pinctrl on both the parent and
  child device.

I previously said the second solution was probably the way to go, but I
changed my mind for two reasons.

First having more subnodes in the device tree was already rejected in
the past in the reviews of the dt-bindings patch. This do makes sense as
it would be describing device internals (which should not be made in
DT), just to ease one specific software implementation (which should
also be avoided). So I believe this change would again be rejected.
https://lore.kernel.org/lkml/58c80c2a-2532-4bc5-9c9f-52480b3af52a@kernel.org/

But the the second reason is, doing
'git grep "device_set_of_node_from_dev.*parent"', I found several
drivers using device_set_of_node_from_dev() for a similar need. Some of
these uses are also for MFD child devices:
- gpio-adp5585.c / pwm-adp5585.c,
- pwm-ntxec.c,
- max77620-regulator.c / max77620_thermal.c.

So, based on this, I believe using device_set_of_node_from_dev() in
these two drivers is the way to go.

> +	chip->pctldev = devm_pinctrl_register(dev, pd, chip);
> +	if (IS_ERR(chip->pctldev))
> +		return dev_err_probe(dev, PTR_ERR(chip->pctldev), "can't register controller\n");
> +
> +	return 0;
> +}
> +




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