On Tue, May 28, 2024 at 8:08 PM Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > Hi Rob, > > On Tue, May 28, 2024 at 10:12:51AM -0500, Rob Herring wrote: > > On Fri, May 24, 2024 at 02:16:41AM +0300, Laurent Pinchart wrote: > > > 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@xxxxxxxxxxxxxxxx> > > > > >>>>> --- > > > > >>>>> 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@xxxxxxxxxxxxxxxx> > > > > >>>>> + > > > > >>>>> +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; > > > > Use device_set_of_node_from_dev(). > > That only works without child nodes in DT. Here I'm assigning the gpio > child node, not the node of the parent device. > > > > + > > > 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); > > > > Wouldn't the driver core do this already? It's not going to know how or > > when of_node was set, so should be doing a put regardless. > > It does, but only when the struct device is being destroyed. Unbinding > and rebinding would leak a reference. Using > device_set_of_node_from_dev() solves that problem, but doesn't work with > child nodes. > > I'm going to send a v2 that squashes everything in a single DT node, > which allows usage of device_set_of_node_from_dev(). Let's see if it > will be more palatable. > > > > + 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. > > > > I think no, but best for Saravana to comment. Don't set fwnode->dev. But I'd actually go even further and say don't set dev->fwnode to NULL. If it ever needs to be set to NULL the driver core will take care of it. And when it does that is when it'll set fwnode->dev to NULL too. So, you setting dev->fwnode to NULL is actually not good once you add the device. -Saravana > > > > > > >>> 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. > > > > True, but it's for all software's PoV, not some specific s/w. > > Agreed. > > > > It > > > shouldn't reflect the structure of Linux drivers, but it has to be > > > usable by drivers. > > > > Either way is usable. > > -- > Regards, > > Laurent Pinchart
