diff mbox series

[v5,6/7] pwm: lpss: Add devm_pwm_lpss_probe() stub

Message ID 20221117110806.65470-7-andriy.shevchenko@linux.intel.com
State Handled Elsewhere
Headers show
Series pinctrl: intel: Enable PWM optional feature | expand

Commit Message

Andy Shevchenko Nov. 17, 2022, 11:08 a.m. UTC 
In case the PWM LPSS module is not provided, allow users to be
compiled with the help of the devm_pwm_lpss_probe() stub.

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>
---
 include/linux/platform_data/x86/pwm-lpss.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Uwe Kleine-König Nov. 22, 2022, 4:47 p.m. UTC | #1 
On Thu, Nov 17, 2022 at 01:08:05PM +0200, Andy Shevchenko wrote:
> In case the PWM LPSS module is not provided, allow users to be
> compiled with the help of the devm_pwm_lpss_probe() stub.
> 
> 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>
> ---
>  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 c852fe24fe2a..6ef21b8baec7 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 *devm_pwm_lpss_probe(struct device *dev, void __iomem *base,
>  					  const struct pwm_lpss_boardinfo *info);
> +#else
> +static inline
> +struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base,
> +					  const struct pwm_lpss_boardinfo *info)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +#endif	/* CONFIG_PWM_LPSS */

Hmm, this is actually never used, because if
!IS_REACHABLE(CONFIG_PWM_LPSS), the only caller (that is added in patch
7) has:

	if (!IS_REACHABLE(CONFIG_PWM_LPSS))
		return 0;

before devm_pwm_lpss_probe() is called.

Not sure if it's safe to just drop this patch. The return value is
neither -ENOSYS (which I would expect for a stub function like that) nor
-EINVAL (which for reasons unknown to me is used in the stub for
pwmchip_add()).

I would have a better feeling with -ENOSYS in your stub, but I don't
feel really strong here.

Best regards
Uwe
Andy Shevchenko Nov. 22, 2022, 5:35 p.m. UTC | #2 
On Tue, Nov 22, 2022 at 05:47:03PM +0100, Uwe Kleine-König wrote:
> On Thu, Nov 17, 2022 at 01:08:05PM +0200, Andy Shevchenko wrote:
> > In case the PWM LPSS module is not provided, allow users to be
> > compiled with the help of the devm_pwm_lpss_probe() stub.

...

> > +static inline
> > +struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base,
> > +					  const struct pwm_lpss_boardinfo *info)
> > +{
> > +	return ERR_PTR(-ENODEV);
> > +}
> > +#endif	/* CONFIG_PWM_LPSS */
> 
> Hmm, this is actually never used, because if
> !IS_REACHABLE(CONFIG_PWM_LPSS), the only caller (that is added in patch
> 7) has:
> 
> 	if (!IS_REACHABLE(CONFIG_PWM_LPSS))
> 		return 0;
> 
> before devm_pwm_lpss_probe() is called.
> 
> Not sure if it's safe to just drop this patch.

How is it supposed to be compiled and linked then?

>	The return value is
> neither -ENOSYS (which I would expect for a stub function like that)

This is not a generic library registration / addition of the device.
I don't see how ENOSYS and esp. EINVAL fits here.

>	nor
> -EINVAL (which for reasons unknown to me is used in the stub for
> pwmchip_add()).

This I explained already that _add() != _probe() semantically, I do not see the
link between their error codes.

> I would have a better feeling with -ENOSYS in your stub, but I don't
> feel really strong here.
Uwe Kleine-König Nov. 22, 2022, 6:14 p.m. UTC | #3 
Hello Andy,

On Tue, Nov 22, 2022 at 07:35:38PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 22, 2022 at 05:47:03PM +0100, Uwe Kleine-König wrote:
> > On Thu, Nov 17, 2022 at 01:08:05PM +0200, Andy Shevchenko wrote:
> > > In case the PWM LPSS module is not provided, allow users to be
> > > compiled with the help of the devm_pwm_lpss_probe() stub.
> 
> ...
> 
> > > +static inline
> > > +struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base,
> > > +					  const struct pwm_lpss_boardinfo *info)
> > > +{
> > > +	return ERR_PTR(-ENODEV);
> > > +}
> > > +#endif	/* CONFIG_PWM_LPSS */
> > 
> > Hmm, this is actually never used, because if
> > !IS_REACHABLE(CONFIG_PWM_LPSS), the only caller (that is added in patch
> > 7) has:
> > 
> > 	if (!IS_REACHABLE(CONFIG_PWM_LPSS))
> > 		return 0;
> > 
> > before devm_pwm_lpss_probe() is called.
> > 
> > Not sure if it's safe to just drop this patch.
> 
> How is it supposed to be compiled and linked then?

The compiler optimizes everything away after that return 0 and so
doesn't need that symbol at all.

I just tested compiling your series without patch #6, x86_64 allmodconfig + PWM=n.

nm doesn't report the need for devm_pwm_lpss_probe in
drivers/pinctrl/intel/pinctrl-intel.o.

The build isn't done yet, but I don't expect surprises.

Best regards
Uwe
Andy Shevchenko Nov. 22, 2022, 6:32 p.m. UTC | #4 
On Tue, Nov 22, 2022 at 07:14:44PM +0100, Uwe Kleine-König wrote:
> On Tue, Nov 22, 2022 at 07:35:38PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 22, 2022 at 05:47:03PM +0100, Uwe Kleine-König wrote:
> > > On Thu, Nov 17, 2022 at 01:08:05PM +0200, Andy Shevchenko wrote:

...

> > > > +static inline
> > > > +struct pwm_lpss_chip *devm_pwm_lpss_probe(struct device *dev, void __iomem *base,
> > > > +					  const struct pwm_lpss_boardinfo *info)
> > > > +{
> > > > +	return ERR_PTR(-ENODEV);
> > > > +}
> > > > +#endif	/* CONFIG_PWM_LPSS */
> > > 
> > > Hmm, this is actually never used, because if
> > > !IS_REACHABLE(CONFIG_PWM_LPSS), the only caller (that is added in patch
> > > 7) has:
> > > 
> > > 	if (!IS_REACHABLE(CONFIG_PWM_LPSS))
> > > 		return 0;
> > > 
> > > before devm_pwm_lpss_probe() is called.
> > > 
> > > Not sure if it's safe to just drop this patch.
> > 
> > How is it supposed to be compiled and linked then?
> 
> The compiler optimizes everything away after that return 0 and so
> doesn't need that symbol at all.
> 
> I just tested compiling your series without patch #6, x86_64 allmodconfig + PWM=n.
> 
> nm doesn't report the need for devm_pwm_lpss_probe in
> drivers/pinctrl/intel/pinctrl-intel.o.

Sounds like you are right. I tried a brief test with m/m, y/m, m/y, and m/n
variants between PINCTRL_INTEL and PWM_LPSS and all were built fine.

That said, I will drop this patch and send a PR with the rest.

Thank you for thorough review!
Uwe Kleine-König Nov. 22, 2022, 8:21 p.m. UTC | #5 
Hello Andy,

On Tue, Nov 22, 2022 at 08:32:53PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 22, 2022 at 07:14:44PM +0100, Uwe Kleine-König wrote:
> > I just tested compiling your series without patch #6, x86_64 allmodconfig + PWM=n.
> > 
> > nm doesn't report the need for devm_pwm_lpss_probe in
> > drivers/pinctrl/intel/pinctrl-intel.o.
> 
> Sounds like you are right. I tried a brief test with m/m, y/m, m/y, and m/n
> variants between PINCTRL_INTEL and PWM_LPSS and all were built fine.
> 
> That said, I will drop this patch and send a PR with the rest.

Sounds good.

> Thank you for thorough review!

Thank you for the interesting thread. I like our conversations.

Best regards
Uwe
diff mbox series

Patch

diff --git a/include/linux/platform_data/x86/pwm-lpss.h b/include/linux/platform_data/x86/pwm-lpss.h
index c852fe24fe2a..6ef21b8baec7 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 *devm_pwm_lpss_probe(struct device *dev, void __iomem *base,
 					  const struct pwm_lpss_boardinfo *info);
+#else
+static inline
+struct pwm_lpss_chip *devm_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 */