| Message ID | 20221114165545.56088-8-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pinctrl: intel: Enable PWM optional feature | expand |
Hello, On Mon, Nov 14, 2022 at 06:55:45PM +0200, Andy Shevchenko wrote: > Some of the Communities may have PWM capability. In such cases, Is "Communities" is proper name in this context? If not, I'd not capitalize it. > enumerate the PWM device via respective driver. User is still s/User/A user/ ? > 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> > Acked-by: Thierry Reding <thierry.reding@gmail.com> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Acked-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/pinctrl/intel/pinctrl-intel.c | 32 +++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c > index 52ecd66ce357..d61c22e9d531 100644 > --- a/drivers/pinctrl/intel/pinctrl-intel.c > +++ b/drivers/pinctrl/intel/pinctrl-intel.c > @@ -21,6 +21,8 @@ > #include <linux/pinctrl/pinconf.h> > #include <linux/pinctrl/pinconf-generic.h> > > +#include <linux/platform_data/x86/pwm-lpss.h> > + > #include "../core.h" > #include "pinctrl-intel.h" > > @@ -46,6 +48,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 > @@ -1499,6 +1503,30 @@ 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; > + > + if (!IS_REACHABLE(CONFIG_PWM_LPSS)) > + return 0; > + > + pwm = devm_pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info); > + if (IS_ERR(pwm)) > + return PTR_ERR(pwm); > + > + return 0; The last 3 codelines can be replaced by return PTR_ERR_OR_ZERO(pwm); (but I know it's subjective if you like that or not, so I won't insist; see also b784c77075023e1a71bc06e6b4f711acb99e9c73). > +} > + > static int intel_pinctrl_probe(struct platform_device *pdev, > const struct intel_pinctrl_soc_data *soc_data) > { > @@ -1584,6 +1612,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); intel_pinctrl_add_padgroups_by_size() doesn't need cleanup in the error path, so this hunk is fine. All in all this is all very minor, so: Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> even if you keep the patch as is. Best regards Uwe
On Thu, Nov 17, 2022 at 10:06:05AM +0100, Uwe Kleine-König wrote: > On Mon, Nov 14, 2022 at 06:55:45PM +0200, Andy Shevchenko wrote: > > Some of the Communities may have PWM capability. In such cases, > > Is "Communities" is proper name in this context? If not, I'd not > capitalize it. Intel pin control is a set of so called Communities, which are divided by groups of pins. (There is an intermediate division, but it doesn't affect software anyhow, so I haven't mentioned it). > > enumerate the PWM device via respective driver. User is still > > s/User/A user/ ? OK! > > responsible for setting correct pin muxing for the line that > > needs to output the signal. ... > > + pwm = devm_pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info); > > + if (IS_ERR(pwm)) > > + return PTR_ERR(pwm); > > + > > + return 0; > > The last 3 codelines can be replaced by > > return PTR_ERR_OR_ZERO(pwm); > > (but I know it's subjective if you like that or not, so I won't insist; > see also b784c77075023e1a71bc06e6b4f711acb99e9c73). Yes, it used to be like that in some of my previous attempts (maybe not public), but have been changed due to an additional error check which is gone, so it can be reinstantiated now. ... > All in all this is all very minor, so: > > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > even if you keep the patch as is. Thank you, I will amend as you suggested.
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c index 52ecd66ce357..d61c22e9d531 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -21,6 +21,8 @@ #include <linux/pinctrl/pinconf.h> #include <linux/pinctrl/pinconf-generic.h> +#include <linux/platform_data/x86/pwm-lpss.h> + #include "../core.h" #include "pinctrl-intel.h" @@ -46,6 +48,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 @@ -1499,6 +1503,30 @@ 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; + + if (!IS_REACHABLE(CONFIG_PWM_LPSS)) + return 0; + + pwm = devm_pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info); + if (IS_ERR(pwm)) + return PTR_ERR(pwm); + + return 0; +} + static int intel_pinctrl_probe(struct platform_device *pdev, const struct intel_pinctrl_soc_data *soc_data) { @@ -1584,6 +1612,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);