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 <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
---
drivers/pwm/pwm-lpss.h | 22 +--------------
.../linux/platform_data/x86}/pwm-lpss.h | 28 ++++---------------
2 files changed, 6 insertions(+), 44 deletions(-)
copy {drivers/pwm => include/linux/platform_data/x86}/pwm-lpss.h (53%)
diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 4561d229b27d..b721532c6c3c 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -13,27 +13,10 @@
#include <linux/pwm.h>
#include <linux/types.h>
-struct device;
+#include <linux/platform_data/x86/pwm-lpss.h>
#define LPSS_MAX_PWMS 4
-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;
@@ -45,7 +28,4 @@ struct pwm_lpss_chip {
const struct pwm_lpss_boardinfo *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 53%
copy from drivers/pwm/pwm-lpss.h
copy to include/linux/platform_data/x86/pwm-lpss.h
index 4561d229b27d..296bd837ddbb 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/include/linux/platform_data/x86/pwm-lpss.h
@@ -1,21 +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>
struct device;
-#define LPSS_MAX_PWMS 4
+struct pwm_lpss_chip;
struct pwm_lpss_boardinfo {
unsigned long clk_rate;
@@ -34,18 +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 {
- struct pwm_chip chip;
- void __iomem *regs;
- const struct pwm_lpss_boardinfo *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
On Tue, Nov 08, 2022 at 04:22:24PM +0200, 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.
"probe" isn't the right term here. The other driver only creates the
device.
> Signed-off-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Mika Westerberg <[email protected]>
> Reviewed-by: Hans de Goede <[email protected]>
> ---
> drivers/pwm/pwm-lpss.h | 22 +--------------
> .../linux/platform_data/x86}/pwm-lpss.h | 28 ++++---------------
> 2 files changed, 6 insertions(+), 44 deletions(-)
> copy {drivers/pwm => include/linux/platform_data/x86}/pwm-lpss.h (53%)
>
> diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
> index 4561d229b27d..b721532c6c3c 100644
> --- a/drivers/pwm/pwm-lpss.h
> +++ b/drivers/pwm/pwm-lpss.h
> @@ -13,27 +13,10 @@
> #include <linux/pwm.h>
> #include <linux/types.h>
>
> -struct device;
> +#include <linux/platform_data/x86/pwm-lpss.h>
>
> #define LPSS_MAX_PWMS 4
>
> -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;
> -};
> -
Now that pwm_lpss_boardinfo lives in a different file, this makes the
move of pwm_lpss_chip in patch 3 somewhat redundant.
> 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;
> @@ -45,7 +28,4 @@ struct pwm_lpss_chip {
> const struct pwm_lpss_boardinfo *info;
> };
>
> -struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, void __iomem *base,
> - const struct pwm_lpss_boardinfo *info);
> -
> #endif /* __PWM_LPSS_H */
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Thu, Nov 10, 2022 at 08:28:10AM +0100, Uwe Kleine-K?nig wrote:
> On Tue, Nov 08, 2022 at 04:22:24PM +0200, 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.
>
> "probe" isn't the right term here. The other driver only creates the
> device.
Ah, I didn't look carefully enough for this judgement. My (wrong)
expectation was, that the containing device creates a platform-device
with .parent=$self and then drivers/pwm/pwm-lpss-platform.c does all the
necessary stuff automatically.
Hmm, I can live with that, so I drop this concern.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Thu, Nov 10, 2022 at 9:28 AM Uwe Kleine-König
<[email protected]> wrote:
> On Tue, Nov 08, 2022 at 04:22:24PM +0200, 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.
...
> Now that pwm_lpss_boardinfo lives in a different file, this makes the
> move of pwm_lpss_chip in patch 3 somewhat redundant.
But they are independent changes. At each stage (after each patch) we
should have a finished step, right? Not touching that makes me feel
that the step is half-baked. If you insist I can drop that move from
the other patch.
--
With Best Regards,
Andy Shevchenko
On Thu, Nov 10, 2022 at 11:58:53AM +0200, Andy Shevchenko wrote:
> On Thu, Nov 10, 2022 at 9:28 AM Uwe Kleine-K?nig
> <[email protected]> wrote:
> > On Tue, Nov 08, 2022 at 04:22:24PM +0200, 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.
>
> ...
>
> > Now that pwm_lpss_boardinfo lives in a different file, this makes the
> > move of pwm_lpss_chip in patch 3 somewhat redundant.
>
> But they are independent changes. At each stage (after each patch) we
> should have a finished step, right? Not touching that makes me feel
> that the step is half-baked. If you insist I can drop that move from
> the other patch.
Given that the move is something you do just en passant in the earlier
patch , I consider my suggestion cleaner. I'd call that 0.5 * insist.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Thu, Nov 10, 2022 at 11:23:17AM +0100, Uwe Kleine-K?nig wrote:
> On Thu, Nov 10, 2022 at 11:58:53AM +0200, Andy Shevchenko wrote:
> > On Thu, Nov 10, 2022 at 9:28 AM Uwe Kleine-K?nig
> > <[email protected]> wrote:
> > > On Tue, Nov 08, 2022 at 04:22:24PM +0200, 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.
...
> > > Now that pwm_lpss_boardinfo lives in a different file, this makes the
> > > move of pwm_lpss_chip in patch 3 somewhat redundant.
> >
> > But they are independent changes. At each stage (after each patch) we
> > should have a finished step, right? Not touching that makes me feel
> > that the step is half-baked. If you insist I can drop that move from
> > the other patch.
>
> Given that the move is something you do just en passant in the earlier
> patch , I consider my suggestion cleaner. I'd call that 0.5 * insist.
I have looked again and I have noticed that in the current state we have
sturct pwm_lpss_chip {
...
};
struct pwm_lpss_boardinfo {
...
};
extern struct pwm_lpss_boardinfo ...;
In the proposed change (with move included) it becomes
#include <...>
extern struct pwm_lpss_boardinfo ...;
sturct pwm_lpss_chip {
...
};
and without
#include <...>
sturct pwm_lpss_chip {
...
};
extern struct pwm_lpss_boardinfo ...;
And I consider that my way is slightly better in terms of ordering.
That said, I will leave it as is for v3. We may continue discussing
it further there.
--
With Best Regards,
Andy Shevchenko