Re: [PATCH v2 6/6] pinctrl: intel: Enumerate PWM device when community has a capabilitty

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Wed, Nov 09, 2022 at 10:08:51AM +0100, Linus Walleij wrote:
> On Tue, Nov 8, 2022 at 3:22 PM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:

...

> > +       static const struct pwm_lpss_boardinfo info = {
> > +               .clk_rate = 19200000,
> > +               .npwm = 1,
> > +               .base_unit_bits = 22,
> > +               .bypass = true,
> > +       };
> > +       struct pwm_lpss_chip *pwm;
> > +
> > +       if (!(community->features & PINCTRL_FEATURE_PWM))
> > +               return 0;
> > +
> > +       pwm = pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info);
> > +       if (IS_ERR(pwm) && PTR_ERR(pwm) != -ENODEV)
> > +               return PTR_ERR(pwm);
> 
> This is alike a boardfile embedded into the pin control driver.

Correct.

> It's a bit backwards since we usually go the other direction, i.e. probe
> a PWM driver or whatever and then that driver request its resources
> that are assigned from e.g. DT or ACPI, and in this case that would
> mean it request its pin control handle and the pins get set up.
> 
> I guess I can be convinced that this hack is the lesser evil :D
> 
> What is it in the platform that makes this kind of hacks necessary?

The PWM capability is discoverable by the looking for it in the pin
control IP MMIO, it's not a separate device, but a sibling (child?)
of the pin control, that's not a separate entity.

Moreover, not every pin control _community_ has that capability (capabilities
are on the Community level and depends on ACPI representation of the
communities themself - single device or device per community - the PWM may or
may not be easily attached.

What you are proposing is to invent at least two additional properties or so
for the pin control device description and then to support old platforms,
create a board file somewhere else, which will go through all pin control
devices, checks the capabilities, then embeds the properties via properties
(Either embedded into DSDT, if done in BIOS, or swnodes).

Do I get you right?

If so, in my opinion it's way more ugly and overkill that the current
approach.

> Inconsistent description in ACPI or is the PWM device simply
> missing from the DSDT (or whatever is the right form in ACPI)
> in already shipped devices that need it?

Right.

> Or is it simply impossible to describe the PWM device in ACPI?

It's a dynamic feature and existing firmwares are carved in stone.
It might be possible to move the above mentioned uglification to
the BIOS. But the horizon of that is 5+ years in case I am able
to convince program managers for it (TBH, I don't believe it's
feasible, since "Windows works!" mantra, they are not engineers).

That said, I agree that this looks not nice, but that's all what
Mika and me can come up with to make all this as little ugly and
intrusive as possible.

-- 
With Best Regards,
Andy Shevchenko