1. Patchew
  2. linux
  3. [PATCH v2 0/9] pwm: lpss: Clean up and convert to a pure library
  4. View patch

[PATCH v2 9/9] pwm: lpss: Allow other drivers to enable PWM LPSS

Andy Shevchenko posted 9 patches 3 years, 6 months ago
There is a newer version of this series
[PATCH v2 9/9] pwm: lpss: Allow other drivers to enable PWM LPSS
Posted by Andy Shevchenko 3 years, 6 months ago 
The PWM LPSS device can be embedded in another device.
In order to enable it, allow that drivers to probe
a corresponding device.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pwm/pwm-lpss.h                        | 26 ++--------------
 .../linux/platform_data/x86}/pwm-lpss.h       | 30 ++++---------------
 2 files changed, 7 insertions(+), 49 deletions(-)
 copy {drivers/pwm => include/linux/platform_data/x86}/pwm-lpss.h (52%)

diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 0249c01befd5..fe32e336db8e 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -13,11 +13,9 @@
 #include <linux/pwm.h>
 #include <linux/types.h>
 
-#define MAX_PWMS			4
-
-struct device;
+#include <linux/platform_data/x86/pwm-lpss.h>
 
-struct pwm_lpss_boardinfo;
+#define MAX_PWMS			4
 
 struct pwm_lpss_chip {
 	struct pwm_chip chip;
@@ -25,29 +23,9 @@ struct pwm_lpss_chip {
 	const struct pwm_lpss_boardinfo *info;
 };
 
-struct pwm_lpss_boardinfo {
-	unsigned long clk_rate;
-	unsigned int npwm;
-	unsigned long base_unit_bits;
-	/*
-	 * Some versions of the IP may stuck in the state machine if enable
-	 * bit is not set, and hence update bit will show busy status till
-	 * the reset. For the rest it may be otherwise.
-	 */
-	bool bypass;
-	/*
-	 * On some devices the _PS0/_PS3 AML code of the GPU (GFX0) device
-	 * messes with the PWM0 controllers state,
-	 */
-	bool other_devices_aml_touches_pwm_regs;
-};
-
 extern const struct pwm_lpss_boardinfo pwm_lpss_byt_info;
 extern const struct pwm_lpss_boardinfo pwm_lpss_bsw_info;
 extern const struct pwm_lpss_boardinfo pwm_lpss_bxt_info;
 extern const struct pwm_lpss_boardinfo pwm_lpss_tng_info;
 
-struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
-				     const struct pwm_lpss_boardinfo *info);
-
 #endif	/* __PWM_LPSS_H */
diff --git a/drivers/pwm/pwm-lpss.h b/include/linux/platform_data/x86/pwm-lpss.h
similarity index 52%
copy from drivers/pwm/pwm-lpss.h
copy to include/linux/platform_data/x86/pwm-lpss.h
index 0249c01befd5..296bd837ddbb 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/include/linux/platform_data/x86/pwm-lpss.h
@@ -1,29 +1,14 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Intel Low Power Subsystem PWM controller driver
- *
- * Copyright (C) 2014, Intel Corporation
- *
- * Derived from the original pwm-lpss.c
- */
+/* Intel Low Power Subsystem PWM controller driver */
 
-#ifndef __PWM_LPSS_H
-#define __PWM_LPSS_H
+#ifndef __PLATFORM_DATA_X86_PWM_LPSS_H
+#define __PLATFORM_DATA_X86_PWM_LPSS_H
 
-#include <linux/pwm.h>
 #include <linux/types.h>
 
-#define MAX_PWMS			4
-
 struct device;
 
-struct pwm_lpss_boardinfo;
-
-struct pwm_lpss_chip {
-	struct pwm_chip chip;
-	void __iomem *regs;
-	const struct pwm_lpss_boardinfo *info;
-};
+struct pwm_lpss_chip;
 
 struct pwm_lpss_boardinfo {
 	unsigned long clk_rate;
@@ -42,12 +27,7 @@ struct pwm_lpss_boardinfo {
 	bool other_devices_aml_touches_pwm_regs;
 };
 
-extern const struct pwm_lpss_boardinfo pwm_lpss_byt_info;
-extern const struct pwm_lpss_boardinfo pwm_lpss_bsw_info;
-extern const struct pwm_lpss_boardinfo pwm_lpss_bxt_info;
-extern const struct pwm_lpss_boardinfo pwm_lpss_tng_info;
-
 struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
 				     const struct pwm_lpss_boardinfo *info);
 
-#endif	/* __PWM_LPSS_H */
+#endif	/* __PLATFORM_DATA_X86_PWM_LPSS_H */
-- 
2.35.1
Re: [PATCH v2 9/9] pwm: lpss: Allow other drivers to enable PWM LPSS
Posted by Uwe Kleine-König 3 years, 6 months ago 
Hello Andy,

On Thu, Sep 08, 2022 at 04:56:58PM +0300, Andy Shevchenko wrote:
> The PWM LPSS device can be embedded in another device.
> In order to enable it, allow that drivers to probe
> a corresponding device.

There is no in-tree user of this. Do you plan to add one?
 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pwm/pwm-lpss.h                        | 26 ++--------------
>  .../linux/platform_data/x86}/pwm-lpss.h       | 30 ++++---------------
>  2 files changed, 7 insertions(+), 49 deletions(-)
>  copy {drivers/pwm => include/linux/platform_data/x86}/pwm-lpss.h (52%)
> 
> diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
> index 0249c01befd5..fe32e336db8e 100644
> --- a/drivers/pwm/pwm-lpss.h
> +++ b/drivers/pwm/pwm-lpss.h
> @@ -13,11 +13,9 @@
>  #include <linux/pwm.h>
>  #include <linux/types.h>
>  
> -#define MAX_PWMS			4
> -
> -struct device;
> +#include <linux/platform_data/x86/pwm-lpss.h>
>  
> -struct pwm_lpss_boardinfo;
> +#define MAX_PWMS			4

Side-note orthogonal to this patch series: IMHO this is a bad name for a
driver specific constant. Without a driver prefix you could easily
misjudge this as e.g. maximal number of PWMs a machine can have. This
should better be named LPSS_MAX_PWMS or similar.

>  
>  struct pwm_lpss_chip {
>  	struct pwm_chip chip;
> @@ -25,29 +23,9 @@ struct pwm_lpss_chip {
>  	const struct pwm_lpss_boardinfo *info;
>  };
>  
> -struct pwm_lpss_boardinfo {
> -	unsigned long clk_rate;
> -	unsigned int npwm;
> -	unsigned long base_unit_bits;
> -	/*
> -	 * Some versions of the IP may stuck in the state machine if enable
> -	 * bit is not set, and hence update bit will show busy status till
> -	 * the reset. For the rest it may be otherwise.
> -	 */
> -	bool bypass;
> -	/*
> -	 * On some devices the _PS0/_PS3 AML code of the GPU (GFX0) device
> -	 * messes with the PWM0 controllers state,
> -	 */
> -	bool other_devices_aml_touches_pwm_regs;
> -};
> -
>  extern const struct pwm_lpss_boardinfo pwm_lpss_byt_info;
>  extern const struct pwm_lpss_boardinfo pwm_lpss_bsw_info;
>  extern const struct pwm_lpss_boardinfo pwm_lpss_bxt_info;
>  extern const struct pwm_lpss_boardinfo pwm_lpss_tng_info;
>  
> -struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
> -				     const struct pwm_lpss_boardinfo *info);
> -
>  #endif	/* __PWM_LPSS_H */
> diff --git a/drivers/pwm/pwm-lpss.h b/include/linux/platform_data/x86/pwm-lpss.h
> similarity index 52%
> copy from drivers/pwm/pwm-lpss.h
> copy to include/linux/platform_data/x86/pwm-lpss.h
> index 0249c01befd5..296bd837ddbb 100644
> --- a/drivers/pwm/pwm-lpss.h
> +++ b/include/linux/platform_data/x86/pwm-lpss.h
> @@ -1,29 +1,14 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Intel Low Power Subsystem PWM controller driver
> - *
> - * Copyright (C) 2014, Intel Corporation
> - *
> - * Derived from the original pwm-lpss.c
> - */
> +/* Intel Low Power Subsystem PWM controller driver */
>  
> -#ifndef __PWM_LPSS_H
> -#define __PWM_LPSS_H
> +#ifndef __PLATFORM_DATA_X86_PWM_LPSS_H
> +#define __PLATFORM_DATA_X86_PWM_LPSS_H
>  
> -#include <linux/pwm.h>
>  #include <linux/types.h>
>  
> -#define MAX_PWMS			4
> -
>  struct device;
>  
> -struct pwm_lpss_boardinfo;
> -

So the declaration you added before and I doubted is gone again.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
Re: [PATCH v2 9/9] pwm: lpss: Allow other drivers to enable PWM LPSS
Posted by Andy Shevchenko 3 years, 6 months ago 
On Sat, Sep 24, 2022 at 12:14:49PM +0200, Uwe Kleine-König wrote:
> On Thu, Sep 08, 2022 at 04:56:58PM +0300, Andy Shevchenko wrote:
> > The PWM LPSS device can be embedded in another device.
> > In order to enable it, allow that drivers to probe
> > a corresponding device.
> 
> There is no in-tree user of this. Do you plan to add one?

Yes.

...

> > +#define MAX_PWMS			4
> 
> Side-note orthogonal to this patch series: IMHO this is a bad name for a
> driver specific constant. Without a driver prefix you could easily
> misjudge this as e.g. maximal number of PWMs a machine can have. This
> should better be named LPSS_MAX_PWMS or similar.

Agree. But it was before my series. I can, of course, fix it at some point.

...

> > -struct pwm_lpss_boardinfo;
> 
> So the declaration you added before and I doubted is gone again.

I agree that it looks not nice in the same series, so I will drop this change
till I have a user. Then it will be clear that intention of the previous patch
is to make sure we don't abuse C programming language.

-- 
With Best Regards,
Andy Shevchenko

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.