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

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%)

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

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.