Message-ID: <20240523231641.GJ6640@pendragon.ideasonboard.com>
Date: Fri, 24 May 2024 02:16:41 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Uwe Kleine-König <ukleinek@...nel.org>
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	linux-gpio@...r.kernel.org, linux-pwm@...r.kernel.org,
	Alexandru Ardelean <alexandru.ardelean@...log.com>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Conor Dooley <conor+dt@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>
Subject: Re: [PATCH 2/5] dt-bindings: Add bindings for the Analog Devices
 ADP5585

Hi Krzysztof,

(There's a question for the GPIO and PWM maintainers below)

On Wed, May 22, 2024 at 09:40:02AM +0200, Krzysztof Kozlowski wrote:
> On 22/05/2024 09:22, Laurent Pinchart wrote:
> > On Wed, May 22, 2024 at 08:57:56AM +0200, Krzysztof Kozlowski wrote:
> >> On 21/05/2024 21:43, Laurent Pinchart wrote:
> >>> On Tue, May 21, 2024 at 09:05:50PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 20/05/2024 21:59, Laurent Pinchart wrote:
> >>>>> The ADP5585 is a 10/11 input/output port expander with a built in keypad
> >>>>> matrix decoder, programmable logic, reset generator, and PWM generator.
> >>>>> These bindings model the device as an MFD, and support the GPIO expander
> >>>>> and PWM functions.
> >>>>>
> >>>>> These bindings support the GPIO and PWM functions.
> >>>>>
> >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> >>>>> ---
> >>>>> I've limited the bindings to GPIO and PWM as I lack hardware to design,
> >>>>> implement and test the rest of the features the chip supports.
> >>>>> ---
> >>>>>  .../bindings/gpio/adi,adp5585-gpio.yaml       |  36 ++++++
> >>>>>  .../devicetree/bindings/mfd/adi,adp5585.yaml  | 117 ++++++++++++++++++
> >>>>>  .../bindings/pwm/adi,adp5585-pwm.yaml         |  35 ++++++
> >>>>>  MAINTAINERS                                   |   7 ++
> >>>>>  4 files changed, 195 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> >>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml
> >>>>>  create mode 100644 Documentation/devicetree/bindings/pwm/adi,adp5585-pwm.yaml
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> >>>>> new file mode 100644
> >>>>> index 000000000000..210e4d53e764
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/gpio/adi,adp5585-gpio.yaml
> >>>>> @@ -0,0 +1,36 @@
> >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>> +%YAML 1.2
> >>>>> +---
> >>>>> +$id: http://devicetree.org/schemas/gpio/adi,adp5585-gpio.yaml#
> >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>> +
> >>>>> +title: Analog Devices ADP5585 GPIO Expander
> >>>>> +
> >>>>> +maintainers:
> >>>>> +  - Laurent Pinchart <laurent.pinchart@...asonboard.com>
> >>>>> +
> >>>>> +description: |
> >>>>> +  The Analog Devices ADP5585 has up to 11 GPIOs represented by a "gpio" child
> >>>>> +  node of the parent MFD device. See
> >>>>> +  Documentation/devicetree/bindings/mfd/adi,adp5585.yaml for further details as
> >>>>> +  well as an example.
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    const: adi,adp5585-gpio
> >>>>> +
> >>>>> +  gpio-controller: true
> >>>>> +
> >>>>> +  '#gpio-cells':
> >>>>> +    const: 2
> >>>>> +
> >>>>> +  gpio-reserved-ranges: true
> >>>>
> >>>> There are no resources here, so new compatible is not really warranted.
> >>>> Squash the node into parent.
> >>>
> >>> Child nodes seem (to me) to be the standard way to model functions in
> >>> MFD devices. Looking at mfd_add_device(), for OF-based systems, the
> >>> function iterates over child nodes. I don't mind going a different
> >>
> >> Only to assign of node, which could be skipped as well.
> > 
> > It has to be assigned somehow, otherwise the GPIO and PWM lookups won't
> > work. That doesn't have to be done in mfd_add_device() though, it can
> > also be done manually by the driver. Looking at the example you gave,
> > cs42l43_pin_probe() handles that assignment. I would have considered
> > that a bit of a hack, but if that's your preferred approach, I'm fine
> > with it. Could you confirm you're OK with that ?
> 
> I am fine with the drivers doing that. It's not a hack, for all
> sub-devices (e.g. also auxiliary bus) you won't have automatic of_node
> assignment.

I gave this a try, and here's what I came up with to drop the compatible
string. Please ignore for a moment the fact that the child nodes are
still there, that's an orthogonal question which I can address
separately. What I would like is feedback on how the OF nodes are
handled.

diff --git a/drivers/gpio/gpio-adp5585.c b/drivers/gpio/gpio-adp5585.c
index 9696a4cdcfc1..8480ceef05ce 100644
--- a/drivers/gpio/gpio-adp5585.c
+++ b/drivers/gpio/gpio-adp5585.c
@@ -174,6 +174,7 @@ static int adp5585_gpio_probe(struct platform_device *pdev)
 	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
 	struct adp5585_gpio_dev *adp5585_gpio;
 	struct device *dev = &pdev->dev;
+	struct device_node *node;
 	struct gpio_chip *gc;
 	int ret;

@@ -187,6 +188,13 @@ static int adp5585_gpio_probe(struct platform_device *pdev)

 	mutex_init(&adp5585_gpio->lock);

+	node = of_get_child_by_name(dev->parent->of_node, "gpio");
+	if (!node)
+		return dev_err_probe(dev, -ENXIO, "'gpio' child node not found\n");
+
+	dev->of_node = node;
+	dev->fwnode = &node->fwnode;
+
 	gc = &adp5585_gpio->gpio_chip;
 	gc->parent = dev;
 	gc->direction_input = adp5585_gpio_direction_input;
@@ -204,6 +212,9 @@ static int adp5585_gpio_probe(struct platform_device *pdev)
 	ret = devm_gpiochip_add_data(&pdev->dev, &adp5585_gpio->gpio_chip,
 				     adp5585_gpio);
 	if (ret) {
+		of_node_put(dev->of_node);
+		dev->of_node = NULL;
+		dev->fwnode = NULL;
 		mutex_destroy(&adp5585_gpio->lock);
 		return dev_err_probe(&pdev->dev, ret, "failed to add GPIO chip\n");
 	}
@@ -215,6 +226,10 @@ static void adp5585_gpio_remove(struct platform_device *pdev)
 {
 	struct adp5585_gpio_dev *adp5585_gpio = platform_get_drvdata(pdev);

+	of_node_put(pdev->dev.of_node);
+	pdev->dev.of_node = NULL;
+	pdev->dev.fwnode = NULL;
+
 	mutex_destroy(&adp5585_gpio->lock);
 }

diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c
index e39a6ea5f794..3b190567ea0b 100644
--- a/drivers/pwm/pwm-adp5585.c
+++ b/drivers/pwm/pwm-adp5585.c
@@ -146,6 +146,8 @@ static const struct pwm_ops adp5585_pwm_ops = {
 static int adp5585_pwm_probe(struct platform_device *pdev)
 {
 	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct device_node *node;
 	struct pwm_chip *chip;
 	int ret;

@@ -153,16 +155,34 @@ static int adp5585_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);

+	node = of_get_child_by_name(dev->parent->of_node, "pwm");
+	if (!node)
+		return dev_err_probe(dev, -ENXIO, "'pwm' child node not found\n");
+
+	dev->of_node = node;
+	dev->fwnode = &node->fwnode;
+
 	pwmchip_set_drvdata(chip, adp5585->regmap);
 	chip->ops = &adp5585_pwm_ops;

 	ret = devm_pwmchip_add(&pdev->dev, chip);
-	if (ret)
+	if (ret) {
+		of_node_put(dev->of_node);
+		dev->of_node = NULL;
+		dev->fwnode = NULL;
 		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
+	}

 	return 0;
 }

+static void adp5585_pwm_remove(struct platform_device *pdev)
+{
+	of_node_put(pdev->dev.of_node);
+	pdev->dev.of_node = NULL;
+	pdev->dev.fwnode = NULL;
+}
+
 static struct platform_driver adp5585_pwm_driver = {
 	.driver	= {
 		.name = "adp5585-pwm",

Is this acceptable ? I'm a bit concerned about poking the internals of
struct device directly from drivers.

I have also refrained from setting fnode->dev to point back to the
device as fone by cs42l43_pin_probe(), as a comment in struct
fwnode_handle indicates that the dev field is for device links only and
shouldn't be touched by anything else. I'm not sure if I should set it.

> >>> routes, could you indicate what you have in mind, perhaps pointing to an
> >>> existing driver as an example ?
> >>
> >> Most of them? OK, let's take the last added driver in MFD directory:
> >> cirrus,cs42l43
> >> It has three children and only two nodes, because only these two devices
> >> actually need/use/benefit the subnodes.
> > 
> > Still trying to understand what bothers you here, is it the child nodes,
> > or the fact that they have a compatible string and are documented in a
> > separate binding ? Looking at the cirrus,cs42l43 bindings and the
> 
> What bothers me (and as expressed in many reviews by us) is representing
> driver structure directly in DT. People model DT based how their Linux
> drivers are represented. I don't care about driver stuff here, but DT/DTS.

DT models the hardware as seen from a software point of view. It
shouldn't reflect the structure of Linux drivers, but it has to be
usable by drivers.

> > corresponding drivers, the pinctrl child node serves the purpose of
> > grouping properties related to the pinctrl function, and allows
> > referencing pinctrl entries from other DT nodes. All those properties
> 
> If you have sub-subnodes, it warrants for me such child. Why? Because it
> makes DTS easier to read.
> 
> > could have been placed in the parent node. Are you fine with the
> > adi,adp5585 having gpio and pwm child nodes, as long as they don't have
> > compatible strings, and are documented in a single binding ?
> 
> As well not, because then you have even less reasons to have them as
> separate nodes. With compatible, one could at least try to argue that
> sub-devices are re-usable across families.

I'll reuse your argument, I think the child nodes make the DTS easier to
read :-)

> >>>>> +required:
> >>>>> +  - compatible
> >>>>> +  - reg
> >>>>> +  - gpio
> >>>>> +  - pwm
> >>>>> +
> >>>>> +allOf:
> >>>>> +  - if:
> >>>>> +      properties:
> >>>>> +        compatible:
> >>>>> +          contains:
> >>>>> +            const: adi,adp5585-01
> >>>>> +    then:
> >>>>> +      properties:
> >>>>> +        gpio:
> >>>>> +          properties:
> >>>>> +            gpio-reserved-ranges: false
> >>>>
> >>>> This also points to fact your child node is pointless. It does not stand
> >>>> on its own...
> >>>
> >>> That doesn't make the child pointless just for that reason. There are
> >>> numerous examples of child nodes that don't stand on their own.
> >>
> >> No, your if-then must be in the schema defining it. This is just
> >> unmaintianable code. It proves that child's compatible means nothing. If
> >> you cannot use child's compatible to make any meaningful choices, then
> >> it is useless.
> > 
> > The compatible string may not be very useful. The child nodes have a
> > use.
> 
> What is their use? Grouping few properties? As mentioned above -
> grouping subnodes like pinctrl does, is argument on its own for code
> readability. Grouping few properties, which in many other devices are in
> top-node (see last 100 reviews of new drivers doing exactly the same),
> is not that argument.
> 
> OTOH, my first, main argument was:
> 
> They do not have any resources on their own. Otherwise please point me -
> which property represents their resource, like clock, reset, gpio,
> suppy, IO address?

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists