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 <[email protected]>
Cc: Mika Westerberg <[email protected]>
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
--
2.35.1
devm_pwmchip_add() can be called by a module that optionally
instantiates PWM chip. In case of CONFIG_PWM=n, the compilation
can't be performed. Hence, add a necessary stub.
Signed-off-by: Andy Shevchenko <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
---
include/linux/pwm.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index d70c6e5a839d..bba492eea96c 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -478,6 +478,11 @@ static inline int pwmchip_remove(struct pwm_chip *chip)
return -EINVAL;
}
+static inline int devm_pwmchip_add(struct device *dev, struct pwm_chip *chip)
+{
+ return -EINVAL;
+}
+
static inline struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
unsigned int index,
const char *label)
--
2.35.1
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 <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
---
drivers/pinctrl/intel/pinctrl-intel.c | 29 +++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 6e630e87fed6..6b685ff7041f 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -24,6 +24,8 @@
#include <linux/pinctrl/pinctrl.h>
#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_data/x86/pwm-lpss.h>
+
#include "../core.h"
#include "pinctrl-intel.h"
@@ -49,6 +51,8 @@
#define PADOWN_MASK(p) (GENMASK(3, 0) << PADOWN_SHIFT(p))
#define PADOWN_GPP(p) ((p) / 8)
+#define PWMC 0x204
+
/* Offset from pad_regs */
#define PADCFG0 0x000
#define PADCFG0_RXEVCFG_SHIFT 25
@@ -1502,6 +1506,27 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
return 0;
}
+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);
+
+ return 0;
+}
+
static int intel_pinctrl_probe(struct platform_device *pdev,
const struct intel_pinctrl_soc_data *soc_data)
{
@@ -1588,6 +1613,10 @@ static int intel_pinctrl_probe(struct platform_device *pdev,
ret = intel_pinctrl_add_padgroups_by_size(pctrl, community);
if (ret)
return ret;
+
+ ret = intel_pinctrl_probe_pwm(pctrl, community);
+ if (ret)
+ return ret;
}
irq = platform_get_irq(pdev, 0);
--
2.35.1
In case the PWM LPSS module is not provided, allow users to be
compiled with a help of a pwm_lpss_probe() stub.
Signed-off-by: Andy Shevchenko <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
---
include/linux/platform_data/x86/pwm-lpss.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/include/linux/platform_data/x86/pwm-lpss.h b/include/linux/platform_data/x86/pwm-lpss.h
index 296bd837ddbb..c868b396ed2c 100644
--- a/include/linux/platform_data/x86/pwm-lpss.h
+++ b/include/linux/platform_data/x86/pwm-lpss.h
@@ -4,6 +4,8 @@
#ifndef __PLATFORM_DATA_X86_PWM_LPSS_H
#define __PLATFORM_DATA_X86_PWM_LPSS_H
+#include <linux/err.h>
+#include <linux/kconfig.h>
#include <linux/types.h>
struct device;
@@ -27,7 +29,16 @@ struct pwm_lpss_boardinfo {
bool other_devices_aml_touches_pwm_regs;
};
+#if IS_REACHABLE(CONFIG_PWM_LPSS)
struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
const struct pwm_lpss_boardinfo *info);
+#else
+static inline
+struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
+ const struct pwm_lpss_boardinfo *info)
+{
+ return ERR_PTR(-ENODEV);
+}
+#endif /* CONFIG_PWM_LPSS */
#endif /* __PLATFORM_DATA_X86_PWM_LPSS_H */
--
2.35.1
The MAX_PWMS definition is already being used by PWM core.
Using the same name in the certain driver confuses people
and potentially can clash with it.
Hence, rename it by adding LPSS prefix.
Reported-by: Uwe Kleine-König <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
---
drivers/pwm/pwm-lpss.c | 2 +-
drivers/pwm/pwm-lpss.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index accdef5dd58e..b8739cd2c235 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -252,7 +252,7 @@ struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
int i, ret;
u32 ctrl;
- if (WARN_ON(info->npwm > MAX_PWMS))
+ if (WARN_ON(info->npwm > LPSS_MAX_PWMS))
return ERR_PTR(-ENODEV);
lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL);
diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 8e82eb5a7e00..2c746c51b883 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -13,7 +13,7 @@
#include <linux/device.h>
#include <linux/pwm.h>
-#define MAX_PWMS 4
+#define LPSS_MAX_PWMS 4
struct pwm_lpss_chip {
struct pwm_chip chip;
--
2.35.1
On Tue, Nov 08, 2022 at 04:22:22PM +0200, Andy Shevchenko wrote:
> The MAX_PWMS definition is already being used by PWM core.
> Using the same name in the certain driver confuses people
> and potentially can clash with it.
>
> Hence, rename it by adding LPSS prefix.
>
> Reported-by: Uwe Kleine-K?nig <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Mika Westerberg <[email protected]>
Reviewed-by: Uwe Kleine-K?nig <[email protected]>
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Tue, Nov 8, 2022 at 3:22 PM Andy Shevchenko
<[email protected]> 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
<[email protected]> 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 <[email protected]>
> Reviewed-by: Mika Westerberg <[email protected]>
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
<[email protected]> 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 <[email protected]>
Yours,
Linus Walleij
On Wed, Nov 09, 2022 at 11:08:04AM +0100, Linus Walleij wrote:
> On Wed, Nov 9, 2022 at 10:56 AM Andy Shevchenko
> <[email protected]> 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.
The problem here is not the impossibility to do the things. The problem is
that things are done and validated on a Windows system. After that it close
to impossible to update the firmware or perform any architectural changes.
OTOH, announcing the separate device out of the existing MMIO space doesn't
sound right from the software point of view that should follow the hardware
representation.
Ideally, this should be an adaptive MFD-like device, but it makes things
even more complicated than has been discussed already. (Note, that some
of the pin control drivers are enumerated as platform devices, and that
code should also be taken into account)
...
> > 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 <[email protected]>
Thank you!
--
With Best Regards,
Andy Shevchenko
On Wed, Nov 09, 2022 at 10:08:51AM +0100, Linus Walleij wrote:
> On Tue, Nov 8, 2022 at 3:22 PM Andy Shevchenko
> <[email protected]> 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
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 <[email protected]>
On Wed, Nov 09, 2022 at 06:40:02PM +0100, Thierry Reding wrote:
> 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.
So far Uwe acknowledged patch 2 only, hopefully he will have time to go
thru the rest.
> Most of this is just reworking existing
> things and the stub additions look good to me, so:
>
> Acked-by: Thierry Reding <[email protected]>
Thank you!
--
With Best Regards,
Andy Shevchenko
Hello
On Tue, Nov 08, 2022 at 04:22:21PM +0200, Andy Shevchenko wrote:
> devm_pwmchip_add() can be called by a module that optionally
> instantiates PWM chip. In case of CONFIG_PWM=n, the compilation
> can't be performed. Hence, add a necessary stub.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Mika Westerberg <[email protected]>
> ---
> include/linux/pwm.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index d70c6e5a839d..bba492eea96c 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -478,6 +478,11 @@ static inline int pwmchip_remove(struct pwm_chip *chip)
> return -EINVAL;
> }
>
> +static inline int devm_pwmchip_add(struct device *dev, struct pwm_chip *chip)
> +{
> + return -EINVAL;
> +}
> +
I'm a bit surprised to see this returning -EINVAL and not -ENOSYS. But
that's in line with the other stubs, so:
Acked-by: Uwe Kleine-K?nig <[email protected]>
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Tue, Nov 08, 2022 at 04:22:25PM +0200, Andy Shevchenko wrote:
> In case the PWM LPSS module is not provided, allow users to be
> compiled with a help of a pwm_lpss_probe() stub.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Mika Westerberg <[email protected]>
> ---
> include/linux/platform_data/x86/pwm-lpss.h | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/include/linux/platform_data/x86/pwm-lpss.h b/include/linux/platform_data/x86/pwm-lpss.h
> index 296bd837ddbb..c868b396ed2c 100644
> --- a/include/linux/platform_data/x86/pwm-lpss.h
> +++ b/include/linux/platform_data/x86/pwm-lpss.h
> @@ -4,6 +4,8 @@
> #ifndef __PLATFORM_DATA_X86_PWM_LPSS_H
> #define __PLATFORM_DATA_X86_PWM_LPSS_H
>
> +#include <linux/err.h>
> +#include <linux/kconfig.h>
> #include <linux/types.h>
>
> struct device;
> @@ -27,7 +29,16 @@ struct pwm_lpss_boardinfo {
> bool other_devices_aml_touches_pwm_regs;
> };
>
> +#if IS_REACHABLE(CONFIG_PWM_LPSS)
> struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
> const struct pwm_lpss_boardinfo *info);
> +#else
> +static inline
> +struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
> + const struct pwm_lpss_boardinfo *info)
> +{
> + return ERR_PTR(-ENODEV);
Would it be more consistent to return the same value as the pwmchip_add
stub does?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Tue, Nov 08, 2022 at 04:22:26PM +0200, Andy Shevchenko 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 <[email protected]>
> Reviewed-by: Mika Westerberg <[email protected]>
> ---
> drivers/pinctrl/intel/pinctrl-intel.c | 29 +++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 6e630e87fed6..6b685ff7041f 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -24,6 +24,8 @@
> #include <linux/pinctrl/pinctrl.h>
> #include <linux/pinctrl/pinmux.h>
>
> +#include <linux/platform_data/x86/pwm-lpss.h>
> +
> #include "../core.h"
> #include "pinctrl-intel.h"
>
> @@ -49,6 +51,8 @@
> #define PADOWN_MASK(p) (GENMASK(3, 0) << PADOWN_SHIFT(p))
> #define PADOWN_GPP(p) ((p) / 8)
>
> +#define PWMC 0x204
> +
> /* Offset from pad_regs */
> #define PADCFG0 0x000
> #define PADCFG0_RXEVCFG_SHIFT 25
> @@ -1502,6 +1506,27 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
> return 0;
> }
>
> +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);
Linus and Andy already agreed that this patch is ugly. I wonder if this
here would be a bit less ugly if you do:
if (IS_REACHABLE(...)) {
pwm = pwm_lpss_probe(...);
...
}
and drop the check PTR_ERR(pwm) != -ENODEV (which might have a different
semantic than "the pwm driver isn't available").
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Thu, Nov 10, 2022 at 9:38 AM Uwe Kleine-König
<[email protected]> wrote:
> On Tue, Nov 08, 2022 at 04:22:25PM +0200, Andy Shevchenko wrote:
> > In case the PWM LPSS module is not provided, allow users to be
> > compiled with a help of a pwm_lpss_probe() stub.
...
> > +static inline
> > +struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
> > + const struct pwm_lpss_boardinfo *info)
> > +{
> > + return ERR_PTR(-ENODEV);
>
> Would it be more consistent to return the same value as the pwmchip_add
> stub does?
Then I will lose the ability to distinguish between absent driver (or
device) and actual error during the probing of it. Any suggestions on
how to do that better?
--
With Best Regards,
Andy Shevchenko
On Thu, Nov 10, 2022 at 9:07 AM Uwe Kleine-König
<[email protected]> wrote:
> On Tue, Nov 08, 2022 at 04:22:21PM +0200, Andy Shevchenko wrote:
...
> > +static inline int devm_pwmchip_add(struct device *dev, struct pwm_chip *chip)
> > +{
> > + return -EINVAL;
> > +}
> > +
>
> I'm a bit surprised to see this returning -EINVAL and not -ENOSYS. But
> that's in line with the other stubs, so:
Exactly my thoughts when I created such a change.
> Acked-by: Uwe Kleine-König <[email protected]>
Thank you!
--
With Best Regards,
Andy Shevchenko
On Thu, Nov 10, 2022 at 9:45 AM Uwe Kleine-König
<[email protected]> wrote:
> On Tue, Nov 08, 2022 at 04:22:26PM +0200, Andy Shevchenko 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.
...
> > + pwm = pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info);
> > + if (IS_ERR(pwm) && PTR_ERR(pwm) != -ENODEV)
> > + return PTR_ERR(pwm);
>
> Linus and Andy already agreed that this patch is ugly. I wonder if this
> here would be a bit less ugly if you do:
>
> if (IS_REACHABLE(...)) {
> pwm = pwm_lpss_probe(...);
> ...
>
>
> }
>
> and drop the check PTR_ERR(pwm) != -ENODEV (which might have a different
> semantic than "the pwm driver isn't available").
I will think about it (in such case the comment against the previous
patch makes more sense to me).
Thank you for the review!
--
With Best Regards,
Andy Shevchenko
On Thu, Nov 10, 2022 at 12:01:50PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 10, 2022 at 9:38 AM Uwe Kleine-K?nig
> <[email protected]> wrote:
> > On Tue, Nov 08, 2022 at 04:22:25PM +0200, Andy Shevchenko wrote:
> > > In case the PWM LPSS module is not provided, allow users to be
> > > compiled with a help of a pwm_lpss_probe() stub.
...
> > > +static inline
> > > +struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
> > > + const struct pwm_lpss_boardinfo *info)
> > > +{
> > > + return ERR_PTR(-ENODEV);
> >
> > Would it be more consistent to return the same value as the pwmchip_add
> > stub does?
>
> Then I will lose the ability to distinguish between absent driver (or
> device) and actual error during the probing of it. Any suggestions on
> how to do that better?
Independently on the above, I think that _probe() != _chip_add() semantically
and having the same, and we know weird, return code doesn't make it anyhow
better. I believe that -ENODEV is the best fit here.
That said, I leave it as is for v3 and we may continue discussing it there.
--
With Best Regards,
Andy Shevchenko