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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <D9276VFZ6GYJ.EO9D1V7C4JSI@bootlin.com>
Date: Wed, 09 Apr 2025 17:03:02 +0200
From: "Mathieu Dubois-Briand" <mathieu.dubois-briand@...tlin.com>
To: "Lee Jones" <lee@...nel.org>, "Rob Herring" <robh@...nel.org>,
 "Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Conor Dooley"
 <conor+dt@...nel.org>, "Kamel Bouhara" <kamel.bouhara@...tlin.com>, "Linus
 Walleij" <linus.walleij@...aro.org>, "Bartosz Golaszewski" <brgl@...ev.pl>,
 "Dmitry Torokhov" <dmitry.torokhov@...il.com>,
 Uwe Kleine-König <ukleinek@...nel.org>, "Michael Walle"
 <mwalle@...nel.org>, "Mark Brown" <broonie@...nel.org>, "Greg
 Kroah-Hartman" <gregkh@...uxfoundation.org>, "Rafael J. Wysocki"
 <rafael@...nel.org>, "Danilo Krummrich" <dakr@...nel.org>
Cc: <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-gpio@...r.kernel.org>, <linux-input@...r.kernel.org>,
 <linux-pwm@...r.kernel.org>, <andriy.shevchenko@...el.com>,
 Grégory Clement <gregory.clement@...tlin.com>, "Thomas
 Petazzoni" <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v6 03/12] pinctrl: Add MAX7360 pinctrl driver

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@...tlin.com>
> Reviewed-by: Linus Walleij <linus.walleij@...aro.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