[v2,0/6] pinctrl: intel: Enable PWM optional feature

This is a continuation of the previously applied PWM LPSS cleanup series.
Now, we would like to enable PWM optional feature that may be embedded
into Intel pin control IPs (starting from Sky Lake platforms).

I would like to route this via Intel pin control tree with issuing
an immutable branch for both PINCTRL and PWM subsystems, but I'm
open for other suggestions.

Hans, I dared to leave your Rb tags, however the patches are slighly
differ, because of the Uwe's suggestion on how to handle the missing
headers. I hope you is okay with that. If not, please comment what
must be ammended then.

Changelog v2:
- added tag (Mika)
- added base-commit to the series, to make sure LKP can test it

Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Andy Shevchenko (6):
  pwm: Add a stub for devm_pwmchip_add()
  pwm: lpss: Rename MAX_PWMS --> LPSS_MAX_PWMS
  pwm: lpss: Include headers we are direct user of
  pwm: lpss: Allow other drivers to enable PWM LPSS
  pwm: lpss: Add pwm_lpss_probe() stub
  pinctrl: intel: Enumerate PWM device when community has a capabilitty

 drivers/pinctrl/intel/pinctrl-intel.c         | 29 +++++++++++++
 drivers/pwm/pwm-lpss.c                        |  2 +-
 drivers/pwm/pwm-lpss.h                        | 34 ++++-----------
 .../linux/platform_data/x86}/pwm-lpss.h       | 41 ++++++++-----------
 include/linux/pwm.h                           |  5 +++
 5 files changed, 61 insertions(+), 50 deletions(-)
 copy {drivers/pwm => include/linux/platform_data/x86}/pwm-lpss.h (51%)


base-commit: 3886bc3523db24814c98c57d74fe66d7a21bf40b

On Tue, Nov 8, 2022 at 3:22 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> I would like to route this via Intel pin control tree with issuing
> an immutable branch for both PINCTRL and PWM subsystems, but I'm
> open for other suggestions.

I'm fine with this approach if it works for Uwe.

Yours,
Linus Walleij

On Tue, Nov 8, 2022 at 3:22 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Some of the Communities may have PWM capability. In such cases,
> enumerate PWM device via respective driver. User is still responsible
> for setting correct pin muxing for the line that needs to output the
> signal.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

So:

> +#include <linux/platform_data/x86/pwm-lpss.h>
(...)
> +static int intel_pinctrl_probe_pwm(struct intel_pinctrl *pctrl,
> +                                  struct intel_community *community)
> +{
> +       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.

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

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

Yours,
Linus Walleij

On Wed, Nov 9, 2022 at 10:56 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, Nov 09, 2022 at 10:08:51AM +0100, Linus Walleij wrote:

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

OK I get it.

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

OK I think I understand it a bit, if ACPI thinks about the PWM
as "some feature of the community" then that is how it is, we have
this bad fit between device tree and Linux internals at times as well,
then spawning a device from another one is the way to go, we need
to consider the option that it is Linux that is weird at times, not the
HW description.

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

No I just wanted to understand things better. This small hack in the
pin controller is way better than a bigger and widespread hack
somewhere else.

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

I can live with it, rough consensus and running code.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

On Tue, Nov 08, 2022 at 04:22:20PM +0200, Andy Shevchenko wrote:
> This is a continuation of the previously applied PWM LPSS cleanup series.
> Now, we would like to enable PWM optional feature that may be embedded
> into Intel pin control IPs (starting from Sky Lake platforms).
> 
> I would like to route this via Intel pin control tree with issuing
> an immutable branch for both PINCTRL and PWM subsystems, but I'm
> open for other suggestions.

I don't have any objections for this to go through the Intel tree as
long as Uwe is happy with this. Most of this is just reworking existing
things and the stub additions look good to me, so:

Acked-by: Thierry Reding <thierry.reding@gmail.com>