On Tue, May 21, 2024 at 03:05:53PM +0200, Uwe Kleine-König wrote:
> Hello,
>
> [dropping Alexandru Ardelean from Cc as their address bounces]
>
> On Tue, May 21, 2024 at 01:09:22PM +0300, Laurent Pinchart wrote:
> > On Tue, May 21, 2024 at 10:51:26AM +0200, Uwe Kleine-König wrote:
> > > On Mon, May 20, 2024 at 10:59:41PM +0300, Laurent Pinchart wrote:
> > > > + ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
> > > > + ADP5585_OSC_EN, ADP5585_OSC_EN);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + return 0;
> > >
> > > The last four lines are equivalent to
> > >
> > > return ret;
> >
> > I prefer the existing code but can also change it.
>
> Well, I see the upside of your approach. If this was my only concern I
> wouldn't refuse to apply the patch.

While I have my preferences, I also favour consistency, so I'm happy to
comply with the preferred coding style for the subsystem :-) I'll
update this in the next version.

> > > > + regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
> > > > + ADP5585_OSC_EN, 0);
> > > > +}
> > > > +
> > > > +static int pwm_adp5585_apply(struct pwm_chip *chip,
> > > > + struct pwm_device *pwm,
> > > > + const struct pwm_state *state)
> > > > +{
> > > > + struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > > > + u32 on, off;
> > > > + int ret;
> > > > +
> > > > + if (!state->enabled) {
> > > > + guard(mutex)(&adp5585_pwm->lock);
> > > > +
> > > > + return regmap_update_bits(adp5585_pwm->regmap, ADP5585_PWM_CFG,
> > > > + ADP5585_PWM_EN, 0);
> > > > + }
> > > > +
> > > > + if (state->period < ADP5585_PWM_MIN_PERIOD_NS ||
> > > > + state->period > ADP5585_PWM_MAX_PERIOD_NS)
> > > > + return -EINVAL;
> > >
> > > Make this:
> > >
> > > if (state->period < ADP5585_PWM_MIN_PERIOD_NS)
> > > return -EINVAL;
> > >
> > > period = min(ADP5585_PWM_MAX_PERIOD_NS, state->period)
> > > duty_cycle = min(period, state->period);
> >
> > I haven't been able to find documentation about the expected behaviour.
> > What's the rationale for returning an error if the period is too low,
> > but silently clamping it if it's too high ?
>
> Well, it's only implicitly documented in the implementation of
> PWM_DEBUG. The reasoning is a combination of the following thoughts:
>
> - Requiring exact matches is hard to work with, so some deviation
> between request and configured value should be allowed.
> - Rounding in both directions has strange and surprising effects. The
> corner cases (for all affected parties (=consumer, lowlevel driver
> and pwm core)) are easier if you only round in one direction.
> One ugly corner case in your suggested patch is:
> ADP5585_PWM_MAX_PERIOD_NS corresponds to 0xffff clock ticks.
> If the consumer requests period=64000.2 clock ticks, you configure
> for 64000. If the consumer requests period=65535.2 clock ticks you
> return -EINVAL.
> Another strange corner case is: Consider a hardware that can
> implement the following periods 499.7 ns, 500.2 ns, 500.3 ns and then
> only values >502 ns.
> If you configure for 501 ns, you'd get 500.3 ns. get_state() would
> tell you it's running at 500 ns. If you then configure 500 ns you
> won't get 500.3 ns any more.
> - If you want to allow 66535.2 clock ticks (and return 65535), what
> should be the maximal value that should yield 65535? Each cut-off
> value is arbitrary, so using \infty looks reasonable (to me at
> least).
> - Rounding down is easier than rounding up, because that's what C's /
> does. (Well, this is admittedly a bit arbitrary, because if you round
> down in .apply() you have to round up in .get_state().)

Thank you for the detailed explanation.

> > > round-closest is wrong. Testing with PWM_DEBUG should point that out.
> > > The right algorithm is:
> > >
> > > on = duty_cycle / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ)
> > > off = period / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) - on
> > >
> > >
> > > > + if (state->polarity == PWM_POLARITY_INVERSED)
> > > > + swap(on, off);
> > >
> > > Uhh, no. Either you can do inverted polarity or you cannot. Don't claim
> > > you can.
> >
> > OK, but what's the rationale ? This is also an area where I couldn't
> > find documentation.
>
> I don't have a good rationale here. IMHO this inverted polarity stuff is
> only a convenience for consumers because the start of the period isn't
> visible from the output wave form (apart from (maybe) the moment where
> you change the configuration) and so
>
> .period = 5000, duty_cycle = 1000, polarity = PWM_POLARITY_NORMAL
>
> isn't distinguishable from
>
> .period = 5000, duty_cycle = 4000, polarity = PWM_POLARITY_INVERSED
>
> . But it's a historic assumption of the pwm core that there is a
> relevant difference between the two polarities and I want at least a
> consistent behaviour among the lowlevel drivers. BTW, this convenience
> is the reason I'm not yet clear how I want to implemement a duty_offset.

Consistency is certainly good. Inverting the duty cycle to implement
inverted polarity would belong in the PWM core if we wanted to implement
it in software I suppose. I'll drop it from the driver.

> > > > + ret = devm_pwmchip_add(&pdev->dev, &adp5585_pwm->chip);
> > > > + if (ret) {
> > > > + mutex_destroy(&adp5585_pwm->lock);
> > > > + return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void adp5585_pwm_remove(struct platform_device *pdev)
> > > > +{
> > > > + struct adp5585_pwm_chip *adp5585_pwm = platform_get_drvdata(pdev);
> > > > +
> > > > + mutex_destroy(&adp5585_pwm->lock);
> > >
> > > Huh, this is a bad idea. The mutex is gone while the pwmchip is still
> > > registered. AFAIK calling mutex_destroy() is optional, and
> > > adp5585_pwm_remove() can just be dropped. Ditto in the error paths of
> > > .probe().
> >
> > mutex_destroy() is a no-op when !CONFIG_DEBUG_MUTEXES. When the config
> > option is selected, it gets more useful. I would prefer moving away from
> > the devm_* registration, and unregister the pwm_chip in .remove()
> > manually, before destroying the mutex.
>
> In that case I'd prefer a devm_mutex_init()?!

Maybe that would be useful :-) Let's see if I can drop the mutex though.

--
Regards,

Laurent Pinchart

2024-05-22 10:23:55

On Wed, May 29, 2024 at 12:02:39AM +0200, Saravana Kannan wrote:
> On Tue, May 28, 2024 at 8:08 PM Laurent Pinchart wrote:
> > 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 <[email protected]>
> > > > > >>>>> ---
> > > > > >>>>> 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 <[email protected]>
> > > > > >>>>> +
> > > > > >>>>> +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.

As far as I understand, the driver core will set dev->fwnode and
dev->of_node to NULL when the device is destroyed. As dev->of_node is
set in the probe function, an unbind-rebind cycle would result in a
reference leak if I don't put the dev->of_node reference in the
remove() handler. And if I do so, I don't want to leave dev->of_node
and dev->fwnode pointing to nodes whose reference has been released, as
they could then possibly disappear (in theory at least).

In any case, I've changed the DT bindings in v2, and the child devices
now reuse the OF node of the parent MFD, instead of using subnodes. This
allows using device_set_of_node_from_dev(), which seems to do fix this
whole issue.

> > > > > >>> 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