| Message ID | 20221108142226.63161-5-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pinctrl: intel: Enable PWM optional feature | expand |
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 <andriy.shevchenko@linux.intel.com> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > --- > 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
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
On Thu, Nov 10, 2022 at 9:28 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 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.
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 > <u.kleine-koenig@pengutronix.de> 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
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 > > <u.kleine-koenig@pengutronix.de> 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.
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 */