* [PATCH RESEND 0/3] pwm support for Broadcom iProc SoC's
@ 2016-03-29 14:22 Yendapally Reddy Dhananjaya Reddy
2016-03-29 14:22 ` [PATCH RESEND 1/3] Documentation: dt: Add Broadcom iproc pwm controller binding Yendapally Reddy Dhananjaya Reddy
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Yendapally Reddy Dhananjaya Reddy @ 2016-03-29 14:22 UTC (permalink / raw)
To: Thierry Reding, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Russell King, Ray Jui, Scott Branden,
Jon Mason
Cc: linux-pwm, devicetree, linux-kernel, linux-arm-kernel,
bcm-kernel-feedback-list, Yendapally Reddy Dhananjaya Reddy
This patchset contains the pwm support for the Broadcom's iProc SoC's.
The first patch provides the documentation details and the
second patch contains the controller support details. The third patch
contains the enable method for Northstar Plus SoC.
This patch series has been tested on NSP bcm958625HR board with oscilloscope.
The pwm is not enabled for this board as the pins are open and not used.
This patch series is based on v4.6.0-rc1 and is available from github
repo: https://github.com/Broadcom/cygnus-linux.git
branch: iproc-pwm-v1
This patch series was originally sent on 03/01/16.
See https://lkml.org/lkml/2016/3/1/487
Yendapally Reddy Dhananjaya Reddy (3):
Documentation: dt: Add Broadcom iproc pwm controller binding
pwm: kona: Add support for Broadcom iproc pwm controller
ARM: dts: NSP: Add PWM Support to DT
.../devicetree/bindings/pwm/brcm,kona-pwm.txt | 2 +-
arch/arm/boot/dts/bcm-nsp.dtsi | 8 +
drivers/pwm/Kconfig | 6 +-
drivers/pwm/pwm-bcm-kona.c | 183 ++++++++++++++++++---
4 files changed, 172 insertions(+), 27 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH RESEND 1/3] Documentation: dt: Add Broadcom iproc pwm controller binding 2016-03-29 14:22 [PATCH RESEND 0/3] pwm support for Broadcom iProc SoC's Yendapally Reddy Dhananjaya Reddy @ 2016-03-29 14:22 ` Yendapally Reddy Dhananjaya Reddy 2016-03-29 14:22 ` [PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller Yendapally Reddy Dhananjaya Reddy ` (2 subsequent siblings) 3 siblings, 0 replies; 10+ messages in thread From: Yendapally Reddy Dhananjaya Reddy @ 2016-03-29 14:22 UTC (permalink / raw) To: Thierry Reding, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Russell King, Ray Jui, Scott Branden, Jon Mason Cc: linux-pwm, devicetree, linux-kernel, Yendapally Reddy Dhananjaya Reddy, bcm-kernel-feedback-list, linux-arm-kernel Add a binding for Broadcom iproc pwm controller Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com> Acked-by: Rob Herring <robh@kernel.org> --- Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt b/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt index 8eae9fe..5036f63 100644 --- a/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt +++ b/Documentation/devicetree/bindings/pwm/brcm,kona-pwm.txt @@ -3,7 +3,7 @@ Broadcom Kona PWM controller device tree bindings This controller has 6 channels. Required Properties : -- compatible: should contain "brcm,kona-pwm" +- compatible: should contain "brcm,kona-pwm" or "brcm,iproc-pwm" - reg: physical base address and length of the controller's registers - clocks: phandle + clock specifier pair for the external clock - #pwm-cells: Should be 3. See pwm.txt in this directory for a -- 2.1.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller 2016-03-29 14:22 [PATCH RESEND 0/3] pwm support for Broadcom iProc SoC's Yendapally Reddy Dhananjaya Reddy 2016-03-29 14:22 ` [PATCH RESEND 1/3] Documentation: dt: Add Broadcom iproc pwm controller binding Yendapally Reddy Dhananjaya Reddy @ 2016-03-29 14:22 ` Yendapally Reddy Dhananjaya Reddy 2016-05-10 14:40 ` Thierry Reding 2016-03-29 14:22 ` [PATCH RESEND 3/3] ARM: dts: NSP: Add PWM Support to DT Yendapally Reddy Dhananjaya Reddy 2016-05-10 13:55 ` [PATCH RESEND 0/3] pwm support for Broadcom iProc SoC's Yendapally Reddy Dhananjaya Reddy 3 siblings, 1 reply; 10+ messages in thread From: Yendapally Reddy Dhananjaya Reddy @ 2016-03-29 14:22 UTC (permalink / raw) To: Thierry Reding, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Russell King, Ray Jui, Scott Branden, Jon Mason Cc: linux-pwm, devicetree, linux-kernel, Yendapally Reddy Dhananjaya Reddy, bcm-kernel-feedback-list, linux-arm-kernel Update the kona driver to support Broadcom iproc pwm controller Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com> --- drivers/pwm/Kconfig | 6 +- drivers/pwm/pwm-bcm-kona.c | 183 +++++++++++++++++++++++++++++++++++++++------ 2 files changed, 163 insertions(+), 26 deletions(-) diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index c182efc..e45ea33 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -76,9 +76,11 @@ config PWM_ATMEL_TCB config PWM_BCM_KONA tristate "Kona PWM support" - depends on ARCH_BCM_MOBILE + depends on ARCH_BCM_MOBILE || ARCH_BCM_IPROC + default ARCH_BCM_IPROC help - Generic PWM framework driver for Broadcom Kona PWM block. + Generic PWM framework driver for Broadcom Kona PWM block. The + same block is also used in Broadcom iProc SoC's. To compile this driver as a module, choose M here: the module will be called pwm-bcm-kona. diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c index c634183..ef152e3a 100644 --- a/drivers/pwm/pwm-bcm-kona.c +++ b/drivers/pwm/pwm-bcm-kona.c @@ -19,6 +19,7 @@ #include <linux/math64.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/pwm.h> #include <linux/slab.h> @@ -47,30 +48,90 @@ #define PWM_CONTROL_OFFSET (0x00000000) #define PWM_CONTROL_SMOOTH_SHIFT(chan) (24 + (chan)) -#define PWM_CONTROL_TYPE_SHIFT(chan) (16 + (chan)) +#define PWM_CONTROL_TYPE_SHIFT(shift, chan) (shift + chan) #define PWM_CONTROL_POLARITY_SHIFT(chan) (8 + (chan)) #define PWM_CONTROL_TRIGGER_SHIFT(chan) (chan) #define PRESCALE_OFFSET (0x00000004) -#define PRESCALE_SHIFT(chan) ((chan) << 2) -#define PRESCALE_MASK(chan) (0x7 << PRESCALE_SHIFT(chan)) +#define PRESCALE_SHIFT (0x00000004) +#define PRESCALE_MASK (0x00000007) #define PRESCALE_MIN (0x00000000) #define PRESCALE_MAX (0x00000007) -#define PERIOD_COUNT_OFFSET(chan) (0x00000008 + ((chan) << 3)) +#define PERIOD_COUNT_OFFSET(offset, chan) (offset + (chan << 3)) #define PERIOD_COUNT_MIN (0x00000002) #define PERIOD_COUNT_MAX (0x00ffffff) +#define KONA_PERIOD_COUNT_OFFSET (0x00000008) -#define DUTY_CYCLE_HIGH_OFFSET(chan) (0x0000000c + ((chan) << 3)) +#define DUTY_CYCLE_HIGH_OFFSET(offset, chan) (offset + (chan << 3)) #define DUTY_CYCLE_HIGH_MIN (0x00000000) #define DUTY_CYCLE_HIGH_MAX (0x00ffffff) +#define KONA_DUTY_CYCLE_HIGH_OFFSET (0x0000000c) + +#define PWM_CHANNEL_CNT (0x00000006) +#define SIGNAL_PUSH_PULL (0x00000001) +#define PWMOUT_TYPE_SHIFT (0x00000010) + +#define IPROC_PRESCALE_OFFSET (0x00000024) +#define IPROC_PRESCALE_SHIFT (0x00000006) +#define IPROC_PRESCALE_MAX (0x0000003f) + +#define IPROC_PERIOD_COUNT_OFFSET (0x00000004) +#define IPROC_PERIOD_COUNT_MIN (0x00000002) +#define IPROC_PERIOD_COUNT_MAX (0x0000ffff) + +#define IPROC_DUTY_CYCLE_HIGH_OFFSET (0x00000008) +#define IPROC_DUTY_CYCLE_HIGH_MIN (0x00000000) +#define IPROC_DUTY_CYCLE_HIGH_MAX (0x0000ffff) + +#define IPROC_PWM_CHANNEL_CNT (0x00000004) +#define IPROC_SIGNAL_PUSH_PULL (0x00000000) +#define IPROC_PWMOUT_TYPE_SHIFT (0x0000000f) + +/* + * pwm controller reg structure + * + * @prescale_offset: prescale register offset + * @period_offset: period register offset + * @duty_offset: duty register offset + * @no_of_channels: number of channels + * @out_type_shift: out type shift in the register + * @signal_type: push-pull or open drain + * @prescale_max: prescale max + * @prescale_shift: prescale shift in register + * @prescale_ch_ascending: prescale ch order in prescale register + * @duty_cycle_max: value of max duty cycle + * @duty_cycle_min: value of min duty cycle + * @period_count_max: max period count val + * @period_count_min: min period count val + * @smooth_output_support: pwm smooth output support + */ +struct kona_pwmc_reg { + u32 prescale_offset; + u32 period_offset; + u32 duty_offset; + u32 no_of_channels; + u32 out_type_shift; + u32 signal_type; + u32 prescale_max; + u32 prescale_shift; + bool prescale_ch_ascending; + u32 duty_cycle_max; + u32 duty_cycle_min; + u32 period_count_max; + u32 period_count_min; + bool smooth_output_support; +}; struct kona_pwmc { struct pwm_chip chip; void __iomem *base; struct clk *clk; + const struct kona_pwmc_reg *reg; }; +static const struct of_device_id bcm_kona_pwmc_dt[]; + static inline struct kona_pwmc *to_kona_pwmc(struct pwm_chip *_chip) { return container_of(_chip, struct kona_pwmc, chip); @@ -84,7 +145,9 @@ static void kona_pwmc_prepare_for_settings(struct kona_pwmc *kp, { unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET); - value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan); + if (kp->reg->smooth_output_support) + value |= 1 << PWM_CONTROL_SMOOTH_SHIFT(chan); + value &= ~(1 << PWM_CONTROL_TRIGGER_SHIFT(chan)); writel(value, kp->base + PWM_CONTROL_OFFSET); @@ -100,7 +163,9 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan) unsigned int value = readl(kp->base + PWM_CONTROL_OFFSET); /* Set trigger bit and clear smooth bit to apply new settings */ - value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan)); + if (kp->reg->smooth_output_support) + value &= ~(1 << PWM_CONTROL_SMOOTH_SHIFT(chan)); + value |= 1 << PWM_CONTROL_TRIGGER_SHIFT(chan); writel(value, kp->base + PWM_CONTROL_OFFSET); @@ -138,15 +203,17 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, dc = div64_u64(val, div); /* If duty_ns or period_ns are not achievable then return */ - if (pc < PERIOD_COUNT_MIN || dc < DUTY_CYCLE_HIGH_MIN) + if (pc < kp->reg->period_count_min || + dc < kp->reg->duty_cycle_min) return -EINVAL; /* If pc and dc are in bounds, the calculation is done */ - if (pc <= PERIOD_COUNT_MAX && dc <= DUTY_CYCLE_HIGH_MAX) + if (pc <= kp->reg->period_count_max && + dc <= kp->reg->duty_cycle_max) break; /* Otherwise, increase prescale and recalculate pc and dc */ - if (++prescale > PRESCALE_MAX) + if (++prescale > kp->reg->prescale_max) return -EINVAL; } @@ -156,16 +223,30 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, * validated immediately instead of on enable. */ if (pwm_is_enabled(pwm)) { + u32 ch_pre_shift = kp->reg->prescale_shift; + kona_pwmc_prepare_for_settings(kp, chan); - value = readl(kp->base + PRESCALE_OFFSET); - value &= ~PRESCALE_MASK(chan); - value |= prescale << PRESCALE_SHIFT(chan); - writel(value, kp->base + PRESCALE_OFFSET); + if (kp->reg->prescale_ch_ascending) + /* + * The prescale bits mask is in ascending + * order in the register + * "ch(n-2)bits..ch(n-1)bits..ch(n)bits". + */ + ch_pre_shift *= ((kp->reg->no_of_channels - 1) - chan); + else + ch_pre_shift *= chan; - writel(pc, kp->base + PERIOD_COUNT_OFFSET(chan)); + value = readl(kp->base + kp->reg->prescale_offset); + value &= ~(kp->reg->prescale_max << ch_pre_shift); + value |= prescale << ch_pre_shift; + writel(value, kp->base + kp->reg->prescale_offset); - writel(dc, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan)); + writel(pc, kp->base + + PERIOD_COUNT_OFFSET(kp->reg->period_offset, chan)); + + writel(dc, kp->base + + DUTY_CYCLE_HIGH_OFFSET(kp->reg->duty_offset, chan)); kona_pwmc_apply_settings(kp, chan); } @@ -231,17 +312,29 @@ static void kona_pwmc_disable(struct pwm_chip *chip, struct pwm_device *pwm) struct kona_pwmc *kp = to_kona_pwmc(chip); unsigned int chan = pwm->hwpwm; unsigned int value; + u32 ch_pre_shift = kp->reg->prescale_shift; + + if (kp->reg->prescale_ch_ascending) + /* + * The prescale bits mask is in ascending order + * in the register "ch(n-2)bits..ch(n-1)bits..ch(n)bits". + */ + ch_pre_shift *= ((kp->reg->no_of_channels - 1) - chan); + else + ch_pre_shift *= chan; kona_pwmc_prepare_for_settings(kp, chan); /* Simulate a disable by configuring for zero duty */ - writel(0, kp->base + DUTY_CYCLE_HIGH_OFFSET(chan)); - writel(0, kp->base + PERIOD_COUNT_OFFSET(chan)); + writel(0, kp->base + + DUTY_CYCLE_HIGH_OFFSET(kp->reg->duty_offset, chan)); + writel(0, kp->base + + PERIOD_COUNT_OFFSET(kp->reg->period_offset, chan)); /* Set prescale to 0 for this channel */ - value = readl(kp->base + PRESCALE_OFFSET); - value &= ~PRESCALE_MASK(chan); - writel(value, kp->base + PRESCALE_OFFSET); + value = readl(kp->base + kp->reg->prescale_offset); + value &= ~(kp->reg->prescale_max << ch_pre_shift); + writel(value, kp->base + kp->reg->prescale_offset); kona_pwmc_apply_settings(kp, chan); @@ -263,17 +356,23 @@ static int kona_pwmc_probe(struct platform_device *pdev) unsigned int chan; unsigned int value = 0; int ret = 0; + const struct of_device_id *match; kp = devm_kzalloc(&pdev->dev, sizeof(*kp), GFP_KERNEL); if (kp == NULL) return -ENOMEM; + match = of_match_device(bcm_kona_pwmc_dt, &pdev->dev); + if (!match) + return -EINVAL; + + kp->reg = (struct kona_pwmc_reg *)match->data; platform_set_drvdata(pdev, kp); kp->chip.dev = &pdev->dev; kp->chip.ops = &kona_pwm_ops; kp->chip.base = -1; - kp->chip.npwm = 6; + kp->chip.npwm = kp->reg->no_of_channels; kp->chip.of_xlate = of_pwm_xlate_with_flags; kp->chip.of_pwm_n_cells = 3; kp->chip.can_sleep = true; @@ -298,7 +397,8 @@ static int kona_pwmc_probe(struct platform_device *pdev) /* Set push/pull for all channels */ for (chan = 0; chan < kp->chip.npwm; chan++) - value |= (1 << PWM_CONTROL_TYPE_SHIFT(chan)); + value |= (kp->reg->signal_type << + PWM_CONTROL_TYPE_SHIFT(kp->reg->out_type_shift, chan)); writel(value, kp->base + PWM_CONTROL_OFFSET); @@ -323,8 +423,43 @@ static int kona_pwmc_remove(struct platform_device *pdev) return pwmchip_remove(&kp->chip); } +static const struct kona_pwmc_reg kona_pwmc_reg_data = { + .prescale_offset = PRESCALE_OFFSET, + .period_offset = KONA_PERIOD_COUNT_OFFSET, + .duty_offset = KONA_DUTY_CYCLE_HIGH_OFFSET, + .no_of_channels = PWM_CHANNEL_CNT, + .out_type_shift = PWMOUT_TYPE_SHIFT, + .signal_type = SIGNAL_PUSH_PULL, + .prescale_max = PRESCALE_MAX, + .prescale_shift = PRESCALE_SHIFT, + .prescale_ch_ascending = false, + .duty_cycle_max = DUTY_CYCLE_HIGH_MAX, + .duty_cycle_min = DUTY_CYCLE_HIGH_MIN, + .period_count_max = PERIOD_COUNT_MAX, + .period_count_min = PERIOD_COUNT_MIN, + .smooth_output_support = true, +}; + +static const struct kona_pwmc_reg iproc_pwmc_reg_data = { + .prescale_offset = IPROC_PRESCALE_OFFSET, + .period_offset = IPROC_PERIOD_COUNT_OFFSET, + .duty_offset = IPROC_DUTY_CYCLE_HIGH_OFFSET, + .no_of_channels = IPROC_PWM_CHANNEL_CNT, + .out_type_shift = IPROC_PWMOUT_TYPE_SHIFT, + .signal_type = IPROC_SIGNAL_PUSH_PULL, + .prescale_max = IPROC_PRESCALE_MAX, + .prescale_shift = IPROC_PRESCALE_SHIFT, + .prescale_ch_ascending = true, + .duty_cycle_max = IPROC_DUTY_CYCLE_HIGH_MAX, + .duty_cycle_min = IPROC_DUTY_CYCLE_HIGH_MIN, + .period_count_max = IPROC_PERIOD_COUNT_MAX, + .period_count_min = IPROC_PERIOD_COUNT_MIN, + .smooth_output_support = false, +}; + static const struct of_device_id bcm_kona_pwmc_dt[] = { - { .compatible = "brcm,kona-pwm" }, + { .compatible = "brcm,kona-pwm", .data = &kona_pwmc_reg_data}, + { .compatible = "brcm,iproc-pwm", .data = &iproc_pwmc_reg_data}, { }, }; MODULE_DEVICE_TABLE(of, bcm_kona_pwmc_dt); -- 2.1.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller 2016-03-29 14:22 ` [PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller Yendapally Reddy Dhananjaya Reddy @ 2016-05-10 14:40 ` Thierry Reding 2016-06-23 20:32 ` Boris Brezillon 0 siblings, 1 reply; 10+ messages in thread From: Thierry Reding @ 2016-05-10 14:40 UTC (permalink / raw) To: Yendapally Reddy Dhananjaya Reddy Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Russell King, Ray Jui, Scott Branden, Jon Mason, linux-pwm, devicetree, linux-kernel, linux-arm-kernel, bcm-kernel-feedback-list [-- Attachment #1: Type: text/plain, Size: 6822 bytes --] On Tue, Mar 29, 2016 at 10:22:29AM -0400, Yendapally Reddy Dhananjaya Reddy wrote: > Update the kona driver to support Broadcom iproc pwm controller > > Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com> > --- > drivers/pwm/Kconfig | 6 +- > drivers/pwm/pwm-bcm-kona.c | 183 +++++++++++++++++++++++++++++++++++++++------ > 2 files changed, 163 insertions(+), 26 deletions(-) > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index c182efc..e45ea33 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -76,9 +76,11 @@ config PWM_ATMEL_TCB > > config PWM_BCM_KONA > tristate "Kona PWM support" > - depends on ARCH_BCM_MOBILE > + depends on ARCH_BCM_MOBILE || ARCH_BCM_IPROC > + default ARCH_BCM_IPROC Why the default? Typically you'd enable this in one or more of the default configurations. default ARCH_* is really only useful if the driver is essential. PWM doesn't usually fall into this category. > diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c > index c634183..ef152e3a 100644 > --- a/drivers/pwm/pwm-bcm-kona.c > +++ b/drivers/pwm/pwm-bcm-kona.c > @@ -19,6 +19,7 @@ > #include <linux/math64.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/pwm.h> > #include <linux/slab.h> > @@ -47,30 +48,90 @@ > > #define PWM_CONTROL_OFFSET (0x00000000) > #define PWM_CONTROL_SMOOTH_SHIFT(chan) (24 + (chan)) > -#define PWM_CONTROL_TYPE_SHIFT(chan) (16 + (chan)) > +#define PWM_CONTROL_TYPE_SHIFT(shift, chan) (shift + chan) You need to put the parameters into parentheses to avoid expansion from potentially messing up the expression. > #define PWM_CONTROL_POLARITY_SHIFT(chan) (8 + (chan)) > #define PWM_CONTROL_TRIGGER_SHIFT(chan) (chan) > > #define PRESCALE_OFFSET (0x00000004) > -#define PRESCALE_SHIFT(chan) ((chan) << 2) > -#define PRESCALE_MASK(chan) (0x7 << PRESCALE_SHIFT(chan)) > +#define PRESCALE_SHIFT (0x00000004) > +#define PRESCALE_MASK (0x00000007) Hmm... this looks odd. Why are you dropping the chan parameter here? > #define PRESCALE_MIN (0x00000000) > #define PRESCALE_MAX (0x00000007) > > -#define PERIOD_COUNT_OFFSET(chan) (0x00000008 + ((chan) << 3)) > +#define PERIOD_COUNT_OFFSET(offset, chan) (offset + (chan << 3)) Need parentheses here as well. > #define PERIOD_COUNT_MIN (0x00000002) > #define PERIOD_COUNT_MAX (0x00ffffff) > +#define KONA_PERIOD_COUNT_OFFSET (0x00000008) > > -#define DUTY_CYCLE_HIGH_OFFSET(chan) (0x0000000c + ((chan) << 3)) > +#define DUTY_CYCLE_HIGH_OFFSET(offset, chan) (offset + (chan << 3)) > #define DUTY_CYCLE_HIGH_MIN (0x00000000) > #define DUTY_CYCLE_HIGH_MAX (0x00ffffff) > +#define KONA_DUTY_CYCLE_HIGH_OFFSET (0x0000000c) > + > +#define PWM_CHANNEL_CNT (0x00000006) > +#define SIGNAL_PUSH_PULL (0x00000001) > +#define PWMOUT_TYPE_SHIFT (0x00000010) > + > +#define IPROC_PRESCALE_OFFSET (0x00000024) > +#define IPROC_PRESCALE_SHIFT (0x00000006) > +#define IPROC_PRESCALE_MAX (0x0000003f) > + > +#define IPROC_PERIOD_COUNT_OFFSET (0x00000004) > +#define IPROC_PERIOD_COUNT_MIN (0x00000002) > +#define IPROC_PERIOD_COUNT_MAX (0x0000ffff) > + > +#define IPROC_DUTY_CYCLE_HIGH_OFFSET (0x00000008) > +#define IPROC_DUTY_CYCLE_HIGH_MIN (0x00000000) > +#define IPROC_DUTY_CYCLE_HIGH_MAX (0x0000ffff) > + > +#define IPROC_PWM_CHANNEL_CNT (0x00000004) > +#define IPROC_SIGNAL_PUSH_PULL (0x00000000) > +#define IPROC_PWMOUT_TYPE_SHIFT (0x0000000f) > + > +/* > + * pwm controller reg structure > + * > + * @prescale_offset: prescale register offset > + * @period_offset: period register offset > + * @duty_offset: duty register offset > + * @no_of_channels: number of channels > + * @out_type_shift: out type shift in the register > + * @signal_type: push-pull or open drain > + * @prescale_max: prescale max > + * @prescale_shift: prescale shift in register > + * @prescale_ch_ascending: prescale ch order in prescale register > + * @duty_cycle_max: value of max duty cycle > + * @duty_cycle_min: value of min duty cycle > + * @period_count_max: max period count val > + * @period_count_min: min period count val > + * @smooth_output_support: pwm smooth output support > + */ > +struct kona_pwmc_reg { > + u32 prescale_offset; > + u32 period_offset; > + u32 duty_offset; > + u32 no_of_channels; > + u32 out_type_shift; > + u32 signal_type; > + u32 prescale_max; > + u32 prescale_shift; > + bool prescale_ch_ascending; > + u32 duty_cycle_max; > + u32 duty_cycle_min; > + u32 period_count_max; > + u32 period_count_min; > + bool smooth_output_support; > +}; This is rather tedious. It looks to me like this isn't very similar to the existing driver. Register offsets move around, bitfield positions change, feature set is different. Might be better off turning this into a separate driver after all. > +static const struct kona_pwmc_reg kona_pwmc_reg_data = { > + .prescale_offset = PRESCALE_OFFSET, > + .period_offset = KONA_PERIOD_COUNT_OFFSET, > + .duty_offset = KONA_DUTY_CYCLE_HIGH_OFFSET, > + .no_of_channels = PWM_CHANNEL_CNT, > + .out_type_shift = PWMOUT_TYPE_SHIFT, > + .signal_type = SIGNAL_PUSH_PULL, > + .prescale_max = PRESCALE_MAX, > + .prescale_shift = PRESCALE_SHIFT, > + .prescale_ch_ascending = false, > + .duty_cycle_max = DUTY_CYCLE_HIGH_MAX, > + .duty_cycle_min = DUTY_CYCLE_HIGH_MIN, > + .period_count_max = PERIOD_COUNT_MAX, > + .period_count_min = PERIOD_COUNT_MIN, > + .smooth_output_support = true, > +}; > + > +static const struct kona_pwmc_reg iproc_pwmc_reg_data = { > + .prescale_offset = IPROC_PRESCALE_OFFSET, > + .period_offset = IPROC_PERIOD_COUNT_OFFSET, > + .duty_offset = IPROC_DUTY_CYCLE_HIGH_OFFSET, > + .no_of_channels = IPROC_PWM_CHANNEL_CNT, > + .out_type_shift = IPROC_PWMOUT_TYPE_SHIFT, > + .signal_type = IPROC_SIGNAL_PUSH_PULL, > + .prescale_max = IPROC_PRESCALE_MAX, > + .prescale_shift = IPROC_PRESCALE_SHIFT, > + .prescale_ch_ascending = true, > + .duty_cycle_max = IPROC_DUTY_CYCLE_HIGH_MAX, > + .duty_cycle_min = IPROC_DUTY_CYCLE_HIGH_MIN, > + .period_count_max = IPROC_PERIOD_COUNT_MAX, > + .period_count_min = IPROC_PERIOD_COUNT_MIN, > + .smooth_output_support = false, > +}; This looks like you could possible support a lot more hardware with this driver because it's now almost completely parameterized. I don't see much sense in keeping this in the same driver and I think it'd be better to write a new one from scratch, even if that means slight duplication. Or you'll have to make a very compelling argument as to why this is the better option. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller 2016-05-10 14:40 ` Thierry Reding @ 2016-06-23 20:32 ` Boris Brezillon 2016-06-28 12:52 ` Thierry Reding 0 siblings, 1 reply; 10+ messages in thread From: Boris Brezillon @ 2016-06-23 20:32 UTC (permalink / raw) To: Thierry Reding Cc: Yendapally Reddy Dhananjaya Reddy, Mark Rutland, linux-pwm, Russell King, Scott Branden, Pawel Moll, Ian Campbell, Jon Mason, Ray Jui, linux-kernel, devicetree, Rob Herring, bcm-kernel-feedback-list, Kumar Gala, linux-arm-kernel Hi, On Tue, 10 May 2016 16:40:24 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, Mar 29, 2016 at 10:22:29AM -0400, Yendapally Reddy Dhananjaya Reddy wrote: > > Update the kona driver to support Broadcom iproc pwm controller > > > > Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com> > > --- > > drivers/pwm/Kconfig | 6 +- > > drivers/pwm/pwm-bcm-kona.c | 183 +++++++++++++++++++++++++++++++++++++++------ > > 2 files changed, 163 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index c182efc..e45ea33 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -76,9 +76,11 @@ config PWM_ATMEL_TCB > > > > config PWM_BCM_KONA > > tristate "Kona PWM support" > > - depends on ARCH_BCM_MOBILE > > + depends on ARCH_BCM_MOBILE || ARCH_BCM_IPROC > > + default ARCH_BCM_IPROC > > Why the default? Typically you'd enable this in one or more of the > default configurations. default ARCH_* is really only useful if the > driver is essential. PWM doesn't usually fall into this category. > > > diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c > > index c634183..ef152e3a 100644 > > --- a/drivers/pwm/pwm-bcm-kona.c > > +++ b/drivers/pwm/pwm-bcm-kona.c > > @@ -19,6 +19,7 @@ > > #include <linux/math64.h> > > #include <linux/module.h> > > #include <linux/of.h> > > +#include <linux/of_device.h> > > #include <linux/platform_device.h> > > #include <linux/pwm.h> > > #include <linux/slab.h> > > @@ -47,30 +48,90 @@ > > > > #define PWM_CONTROL_OFFSET (0x00000000) > > #define PWM_CONTROL_SMOOTH_SHIFT(chan) (24 + (chan)) > > -#define PWM_CONTROL_TYPE_SHIFT(chan) (16 + (chan)) > > +#define PWM_CONTROL_TYPE_SHIFT(shift, chan) (shift + chan) > > You need to put the parameters into parentheses to avoid expansion from > potentially messing up the expression. > > > #define PWM_CONTROL_POLARITY_SHIFT(chan) (8 + (chan)) > > #define PWM_CONTROL_TRIGGER_SHIFT(chan) (chan) > > > > #define PRESCALE_OFFSET (0x00000004) > > -#define PRESCALE_SHIFT(chan) ((chan) << 2) > > -#define PRESCALE_MASK(chan) (0x7 << PRESCALE_SHIFT(chan)) > > +#define PRESCALE_SHIFT (0x00000004) > > +#define PRESCALE_MASK (0x00000007) > > Hmm... this looks odd. Why are you dropping the chan parameter here? > > > #define PRESCALE_MIN (0x00000000) > > #define PRESCALE_MAX (0x00000007) > > > > -#define PERIOD_COUNT_OFFSET(chan) (0x00000008 + ((chan) << 3)) > > +#define PERIOD_COUNT_OFFSET(offset, chan) (offset + (chan << 3)) > > Need parentheses here as well. > > > #define PERIOD_COUNT_MIN (0x00000002) > > #define PERIOD_COUNT_MAX (0x00ffffff) > > +#define KONA_PERIOD_COUNT_OFFSET (0x00000008) > > > > -#define DUTY_CYCLE_HIGH_OFFSET(chan) (0x0000000c + ((chan) << 3)) > > +#define DUTY_CYCLE_HIGH_OFFSET(offset, chan) (offset + (chan << 3)) > > #define DUTY_CYCLE_HIGH_MIN (0x00000000) > > #define DUTY_CYCLE_HIGH_MAX (0x00ffffff) > > +#define KONA_DUTY_CYCLE_HIGH_OFFSET (0x0000000c) > > + > > +#define PWM_CHANNEL_CNT (0x00000006) > > +#define SIGNAL_PUSH_PULL (0x00000001) > > +#define PWMOUT_TYPE_SHIFT (0x00000010) > > + > > +#define IPROC_PRESCALE_OFFSET (0x00000024) > > +#define IPROC_PRESCALE_SHIFT (0x00000006) > > +#define IPROC_PRESCALE_MAX (0x0000003f) > > + > > +#define IPROC_PERIOD_COUNT_OFFSET (0x00000004) > > +#define IPROC_PERIOD_COUNT_MIN (0x00000002) > > +#define IPROC_PERIOD_COUNT_MAX (0x0000ffff) > > + > > +#define IPROC_DUTY_CYCLE_HIGH_OFFSET (0x00000008) > > +#define IPROC_DUTY_CYCLE_HIGH_MIN (0x00000000) > > +#define IPROC_DUTY_CYCLE_HIGH_MAX (0x0000ffff) > > + > > +#define IPROC_PWM_CHANNEL_CNT (0x00000004) > > +#define IPROC_SIGNAL_PUSH_PULL (0x00000000) > > +#define IPROC_PWMOUT_TYPE_SHIFT (0x0000000f) > > + > > +/* > > + * pwm controller reg structure > > + * > > + * @prescale_offset: prescale register offset > > + * @period_offset: period register offset > > + * @duty_offset: duty register offset > > + * @no_of_channels: number of channels > > + * @out_type_shift: out type shift in the register > > + * @signal_type: push-pull or open drain > > + * @prescale_max: prescale max > > + * @prescale_shift: prescale shift in register > > + * @prescale_ch_ascending: prescale ch order in prescale register > > + * @duty_cycle_max: value of max duty cycle > > + * @duty_cycle_min: value of min duty cycle > > + * @period_count_max: max period count val > > + * @period_count_min: min period count val > > + * @smooth_output_support: pwm smooth output support > > + */ > > +struct kona_pwmc_reg { > > + u32 prescale_offset; > > + u32 period_offset; > > + u32 duty_offset; > > + u32 no_of_channels; > > + u32 out_type_shift; > > + u32 signal_type; > > + u32 prescale_max; > > + u32 prescale_shift; > > + bool prescale_ch_ascending; > > + u32 duty_cycle_max; > > + u32 duty_cycle_min; > > + u32 period_count_max; > > + u32 period_count_min; > > + bool smooth_output_support; > > +}; > > This is rather tedious. It looks to me like this isn't very similar to > the existing driver. Register offsets move around, bitfield positions > change, feature set is different. Might be better off turning this into > a separate driver after all. > > > +static const struct kona_pwmc_reg kona_pwmc_reg_data = { > > + .prescale_offset = PRESCALE_OFFSET, > > + .period_offset = KONA_PERIOD_COUNT_OFFSET, > > + .duty_offset = KONA_DUTY_CYCLE_HIGH_OFFSET, > > + .no_of_channels = PWM_CHANNEL_CNT, > > + .out_type_shift = PWMOUT_TYPE_SHIFT, > > + .signal_type = SIGNAL_PUSH_PULL, > > + .prescale_max = PRESCALE_MAX, > > + .prescale_shift = PRESCALE_SHIFT, > > + .prescale_ch_ascending = false, > > + .duty_cycle_max = DUTY_CYCLE_HIGH_MAX, > > + .duty_cycle_min = DUTY_CYCLE_HIGH_MIN, > > + .period_count_max = PERIOD_COUNT_MAX, > > + .period_count_min = PERIOD_COUNT_MIN, > > + .smooth_output_support = true, > > +}; > > + > > +static const struct kona_pwmc_reg iproc_pwmc_reg_data = { > > + .prescale_offset = IPROC_PRESCALE_OFFSET, > > + .period_offset = IPROC_PERIOD_COUNT_OFFSET, > > + .duty_offset = IPROC_DUTY_CYCLE_HIGH_OFFSET, > > + .no_of_channels = IPROC_PWM_CHANNEL_CNT, > > + .out_type_shift = IPROC_PWMOUT_TYPE_SHIFT, > > + .signal_type = IPROC_SIGNAL_PUSH_PULL, > > + .prescale_max = IPROC_PRESCALE_MAX, > > + .prescale_shift = IPROC_PRESCALE_SHIFT, > > + .prescale_ch_ascending = true, > > + .duty_cycle_max = IPROC_DUTY_CYCLE_HIGH_MAX, > > + .duty_cycle_min = IPROC_DUTY_CYCLE_HIGH_MIN, > > + .period_count_max = IPROC_PERIOD_COUNT_MAX, > > + .period_count_min = IPROC_PERIOD_COUNT_MIN, > > + .smooth_output_support = false, > > +}; > > This looks like you could possible support a lot more hardware with this > driver because it's now almost completely parameterized. > > I don't see much sense in keeping this in the same driver and I think > it'd be better to write a new one from scratch, even if that means > slight duplication. > > Or you'll have to make a very compelling argument as to why this is the > better option. Sorry to enter the discussion just now (Brian CCed me in an answer he made to v4 of this series). Honestly, I'd have the exact opposite argument: if you have a look at newer versions of this patch, you'll see that more than 90% of the code is duplicated, and re-using the same logic with different field positions is quite common in other drivers. Actually the counter argument to your suggestion are bug fixes: when you find a bug, you'll have to remember fixing it for all implementations. Having a common code base has IMO more pros than cons. BTW, if you switch to regmap, you have this field-position customization for free (see reg_field). Regards, Boris ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller 2016-06-23 20:32 ` Boris Brezillon @ 2016-06-28 12:52 ` Thierry Reding 2016-07-04 13:37 ` Boris Brezillon 0 siblings, 1 reply; 10+ messages in thread From: Thierry Reding @ 2016-06-28 12:52 UTC (permalink / raw) To: Boris Brezillon Cc: Yendapally Reddy Dhananjaya Reddy, Mark Rutland, linux-pwm-u79uwXL29TY76Z2rM5mHXA, Russell King, Scott Branden, Pawel Moll, Ian Campbell, Jon Mason, Ray Jui, linux-kernel-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w, Kumar Gala, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r [-- Attachment #1: Type: text/plain, Size: 9222 bytes --] On Thu, Jun 23, 2016 at 10:32:52PM +0200, Boris Brezillon wrote: > Hi, > > On Tue, 10 May 2016 16:40:24 +0200 > Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > > > On Tue, Mar 29, 2016 at 10:22:29AM -0400, Yendapally Reddy Dhananjaya Reddy wrote: > > > Update the kona driver to support Broadcom iproc pwm controller > > > > > > Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com> > > > --- > > > drivers/pwm/Kconfig | 6 +- > > > drivers/pwm/pwm-bcm-kona.c | 183 +++++++++++++++++++++++++++++++++++++++------ > > > 2 files changed, 163 insertions(+), 26 deletions(-) > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > index c182efc..e45ea33 100644 > > > --- a/drivers/pwm/Kconfig > > > +++ b/drivers/pwm/Kconfig > > > @@ -76,9 +76,11 @@ config PWM_ATMEL_TCB > > > > > > config PWM_BCM_KONA > > > tristate "Kona PWM support" > > > - depends on ARCH_BCM_MOBILE > > > + depends on ARCH_BCM_MOBILE || ARCH_BCM_IPROC > > > + default ARCH_BCM_IPROC > > > > Why the default? Typically you'd enable this in one or more of the > > default configurations. default ARCH_* is really only useful if the > > driver is essential. PWM doesn't usually fall into this category. > > > > > diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c > > > index c634183..ef152e3a 100644 > > > --- a/drivers/pwm/pwm-bcm-kona.c > > > +++ b/drivers/pwm/pwm-bcm-kona.c > > > @@ -19,6 +19,7 @@ > > > #include <linux/math64.h> > > > #include <linux/module.h> > > > #include <linux/of.h> > > > +#include <linux/of_device.h> > > > #include <linux/platform_device.h> > > > #include <linux/pwm.h> > > > #include <linux/slab.h> > > > @@ -47,30 +48,90 @@ > > > > > > #define PWM_CONTROL_OFFSET (0x00000000) > > > #define PWM_CONTROL_SMOOTH_SHIFT(chan) (24 + (chan)) > > > -#define PWM_CONTROL_TYPE_SHIFT(chan) (16 + (chan)) > > > +#define PWM_CONTROL_TYPE_SHIFT(shift, chan) (shift + chan) > > > > You need to put the parameters into parentheses to avoid expansion from > > potentially messing up the expression. > > > > > #define PWM_CONTROL_POLARITY_SHIFT(chan) (8 + (chan)) > > > #define PWM_CONTROL_TRIGGER_SHIFT(chan) (chan) > > > > > > #define PRESCALE_OFFSET (0x00000004) > > > -#define PRESCALE_SHIFT(chan) ((chan) << 2) > > > -#define PRESCALE_MASK(chan) (0x7 << PRESCALE_SHIFT(chan)) > > > +#define PRESCALE_SHIFT (0x00000004) > > > +#define PRESCALE_MASK (0x00000007) > > > > Hmm... this looks odd. Why are you dropping the chan parameter here? > > > > > #define PRESCALE_MIN (0x00000000) > > > #define PRESCALE_MAX (0x00000007) > > > > > > -#define PERIOD_COUNT_OFFSET(chan) (0x00000008 + ((chan) << 3)) > > > +#define PERIOD_COUNT_OFFSET(offset, chan) (offset + (chan << 3)) > > > > Need parentheses here as well. > > > > > #define PERIOD_COUNT_MIN (0x00000002) > > > #define PERIOD_COUNT_MAX (0x00ffffff) > > > +#define KONA_PERIOD_COUNT_OFFSET (0x00000008) > > > > > > -#define DUTY_CYCLE_HIGH_OFFSET(chan) (0x0000000c + ((chan) << 3)) > > > +#define DUTY_CYCLE_HIGH_OFFSET(offset, chan) (offset + (chan << 3)) > > > #define DUTY_CYCLE_HIGH_MIN (0x00000000) > > > #define DUTY_CYCLE_HIGH_MAX (0x00ffffff) > > > +#define KONA_DUTY_CYCLE_HIGH_OFFSET (0x0000000c) > > > + > > > +#define PWM_CHANNEL_CNT (0x00000006) > > > +#define SIGNAL_PUSH_PULL (0x00000001) > > > +#define PWMOUT_TYPE_SHIFT (0x00000010) > > > + > > > +#define IPROC_PRESCALE_OFFSET (0x00000024) > > > +#define IPROC_PRESCALE_SHIFT (0x00000006) > > > +#define IPROC_PRESCALE_MAX (0x0000003f) > > > + > > > +#define IPROC_PERIOD_COUNT_OFFSET (0x00000004) > > > +#define IPROC_PERIOD_COUNT_MIN (0x00000002) > > > +#define IPROC_PERIOD_COUNT_MAX (0x0000ffff) > > > + > > > +#define IPROC_DUTY_CYCLE_HIGH_OFFSET (0x00000008) > > > +#define IPROC_DUTY_CYCLE_HIGH_MIN (0x00000000) > > > +#define IPROC_DUTY_CYCLE_HIGH_MAX (0x0000ffff) > > > + > > > +#define IPROC_PWM_CHANNEL_CNT (0x00000004) > > > +#define IPROC_SIGNAL_PUSH_PULL (0x00000000) > > > +#define IPROC_PWMOUT_TYPE_SHIFT (0x0000000f) > > > + > > > +/* > > > + * pwm controller reg structure > > > + * > > > + * @prescale_offset: prescale register offset > > > + * @period_offset: period register offset > > > + * @duty_offset: duty register offset > > > + * @no_of_channels: number of channels > > > + * @out_type_shift: out type shift in the register > > > + * @signal_type: push-pull or open drain > > > + * @prescale_max: prescale max > > > + * @prescale_shift: prescale shift in register > > > + * @prescale_ch_ascending: prescale ch order in prescale register > > > + * @duty_cycle_max: value of max duty cycle > > > + * @duty_cycle_min: value of min duty cycle > > > + * @period_count_max: max period count val > > > + * @period_count_min: min period count val > > > + * @smooth_output_support: pwm smooth output support > > > + */ > > > +struct kona_pwmc_reg { > > > + u32 prescale_offset; > > > + u32 period_offset; > > > + u32 duty_offset; > > > + u32 no_of_channels; > > > + u32 out_type_shift; > > > + u32 signal_type; > > > + u32 prescale_max; > > > + u32 prescale_shift; > > > + bool prescale_ch_ascending; > > > + u32 duty_cycle_max; > > > + u32 duty_cycle_min; > > > + u32 period_count_max; > > > + u32 period_count_min; > > > + bool smooth_output_support; > > > +}; > > > > This is rather tedious. It looks to me like this isn't very similar to > > the existing driver. Register offsets move around, bitfield positions > > change, feature set is different. Might be better off turning this into > > a separate driver after all. > > > > > +static const struct kona_pwmc_reg kona_pwmc_reg_data = { > > > + .prescale_offset = PRESCALE_OFFSET, > > > + .period_offset = KONA_PERIOD_COUNT_OFFSET, > > > + .duty_offset = KONA_DUTY_CYCLE_HIGH_OFFSET, > > > + .no_of_channels = PWM_CHANNEL_CNT, > > > + .out_type_shift = PWMOUT_TYPE_SHIFT, > > > + .signal_type = SIGNAL_PUSH_PULL, > > > + .prescale_max = PRESCALE_MAX, > > > + .prescale_shift = PRESCALE_SHIFT, > > > + .prescale_ch_ascending = false, > > > + .duty_cycle_max = DUTY_CYCLE_HIGH_MAX, > > > + .duty_cycle_min = DUTY_CYCLE_HIGH_MIN, > > > + .period_count_max = PERIOD_COUNT_MAX, > > > + .period_count_min = PERIOD_COUNT_MIN, > > > + .smooth_output_support = true, > > > +}; > > > + > > > +static const struct kona_pwmc_reg iproc_pwmc_reg_data = { > > > + .prescale_offset = IPROC_PRESCALE_OFFSET, > > > + .period_offset = IPROC_PERIOD_COUNT_OFFSET, > > > + .duty_offset = IPROC_DUTY_CYCLE_HIGH_OFFSET, > > > + .no_of_channels = IPROC_PWM_CHANNEL_CNT, > > > + .out_type_shift = IPROC_PWMOUT_TYPE_SHIFT, > > > + .signal_type = IPROC_SIGNAL_PUSH_PULL, > > > + .prescale_max = IPROC_PRESCALE_MAX, > > > + .prescale_shift = IPROC_PRESCALE_SHIFT, > > > + .prescale_ch_ascending = true, > > > + .duty_cycle_max = IPROC_DUTY_CYCLE_HIGH_MAX, > > > + .duty_cycle_min = IPROC_DUTY_CYCLE_HIGH_MIN, > > > + .period_count_max = IPROC_PERIOD_COUNT_MAX, > > > + .period_count_min = IPROC_PERIOD_COUNT_MIN, > > > + .smooth_output_support = false, > > > +}; > > > > This looks like you could possible support a lot more hardware with this > > driver because it's now almost completely parameterized. > > > > I don't see much sense in keeping this in the same driver and I think > > it'd be better to write a new one from scratch, even if that means > > slight duplication. > > > > Or you'll have to make a very compelling argument as to why this is the > > better option. > > Sorry to enter the discussion just now (Brian CCed me in an answer he > made to v4 of this series). > > Honestly, I'd have the exact opposite argument: if you have a look at > newer versions of this patch, you'll see that more than 90% of the code > is duplicated, and re-using the same logic with different field > positions is quite common in other drivers. Actually the counter > argument to your suggestion are bug fixes: when you find a bug, you'll > have to remember fixing it for all implementations. Having a common > code base has IMO more pros than cons. > > BTW, if you switch to regmap, you have this field-position > customization for free (see reg_field). My point was that with the above structures you have an almost completely parameterized driver. There's probably a number of other PWM controllers that you could support using those structure, or by adding to them. The question then becomes: where do we draw the line? When is a new driver warranted, and when is it not. Your argument regarding bug fixes has some weight, but these drivers are really trivial, so bug fixes will be equally trivial to port between them. There's also the issue about errata and quirks that may apply to one but not all supported versions. The outcome of supporting all those combinations can be completely unmaintainable, so I am willing to take a bit of duplication if it means that in the end it will make things easier for me. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller 2016-06-28 12:52 ` Thierry Reding @ 2016-07-04 13:37 ` Boris Brezillon 0 siblings, 0 replies; 10+ messages in thread From: Boris Brezillon @ 2016-07-04 13:37 UTC (permalink / raw) To: Thierry Reding Cc: Mark Rutland, linux-pwm, Russell King, Scott Branden, Pawel Moll, Ian Campbell, Jon Mason, Ray Jui, linux-kernel, Yendapally Reddy Dhananjaya Reddy, Rob Herring, bcm-kernel-feedback-list, linux-arm-kernel, Kumar Gala, devicetree Hi Thierry, This is my last answer to this thread, after that I'll stop bothering you: after all, you're the PWM subsystem maintainer, and I have nothing to say about how you want to maintain your subsytem ;-). On Tue, 28 Jun 2016 14:52:23 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, Jun 23, 2016 at 10:32:52PM +0200, Boris Brezillon wrote: > > Hi, > > > > On Tue, 10 May 2016 16:40:24 +0200 > > Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > On Tue, Mar 29, 2016 at 10:22:29AM -0400, Yendapally Reddy Dhananjaya Reddy wrote: > > > > Update the kona driver to support Broadcom iproc pwm controller > > > > > > > > Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com> > > > > --- > > > > drivers/pwm/Kconfig | 6 +- > > > > drivers/pwm/pwm-bcm-kona.c | 183 +++++++++++++++++++++++++++++++++++++++------ > > > > 2 files changed, 163 insertions(+), 26 deletions(-) > > > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > > index c182efc..e45ea33 100644 > > > > --- a/drivers/pwm/Kconfig > > > > +++ b/drivers/pwm/Kconfig > > > > @@ -76,9 +76,11 @@ config PWM_ATMEL_TCB > > > > > > > > config PWM_BCM_KONA > > > > tristate "Kona PWM support" > > > > - depends on ARCH_BCM_MOBILE > > > > + depends on ARCH_BCM_MOBILE || ARCH_BCM_IPROC > > > > + default ARCH_BCM_IPROC > > > > > > Why the default? Typically you'd enable this in one or more of the > > > default configurations. default ARCH_* is really only useful if the > > > driver is essential. PWM doesn't usually fall into this category. > > > > > > > diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c > > > > index c634183..ef152e3a 100644 > > > > --- a/drivers/pwm/pwm-bcm-kona.c > > > > +++ b/drivers/pwm/pwm-bcm-kona.c > > > > @@ -19,6 +19,7 @@ > > > > #include <linux/math64.h> > > > > #include <linux/module.h> > > > > #include <linux/of.h> > > > > +#include <linux/of_device.h> > > > > #include <linux/platform_device.h> > > > > #include <linux/pwm.h> > > > > #include <linux/slab.h> > > > > @@ -47,30 +48,90 @@ > > > > > > > > #define PWM_CONTROL_OFFSET (0x00000000) > > > > #define PWM_CONTROL_SMOOTH_SHIFT(chan) (24 + (chan)) > > > > -#define PWM_CONTROL_TYPE_SHIFT(chan) (16 + (chan)) > > > > +#define PWM_CONTROL_TYPE_SHIFT(shift, chan) (shift + chan) > > > > > > You need to put the parameters into parentheses to avoid expansion from > > > potentially messing up the expression. > > > > > > > #define PWM_CONTROL_POLARITY_SHIFT(chan) (8 + (chan)) > > > > #define PWM_CONTROL_TRIGGER_SHIFT(chan) (chan) > > > > > > > > #define PRESCALE_OFFSET (0x00000004) > > > > -#define PRESCALE_SHIFT(chan) ((chan) << 2) > > > > -#define PRESCALE_MASK(chan) (0x7 << PRESCALE_SHIFT(chan)) > > > > +#define PRESCALE_SHIFT (0x00000004) > > > > +#define PRESCALE_MASK (0x00000007) > > > > > > Hmm... this looks odd. Why are you dropping the chan parameter here? > > > > > > > #define PRESCALE_MIN (0x00000000) > > > > #define PRESCALE_MAX (0x00000007) > > > > > > > > -#define PERIOD_COUNT_OFFSET(chan) (0x00000008 + ((chan) << 3)) > > > > +#define PERIOD_COUNT_OFFSET(offset, chan) (offset + (chan << 3)) > > > > > > Need parentheses here as well. > > > > > > > #define PERIOD_COUNT_MIN (0x00000002) > > > > #define PERIOD_COUNT_MAX (0x00ffffff) > > > > +#define KONA_PERIOD_COUNT_OFFSET (0x00000008) > > > > > > > > -#define DUTY_CYCLE_HIGH_OFFSET(chan) (0x0000000c + ((chan) << 3)) > > > > +#define DUTY_CYCLE_HIGH_OFFSET(offset, chan) (offset + (chan << 3)) > > > > #define DUTY_CYCLE_HIGH_MIN (0x00000000) > > > > #define DUTY_CYCLE_HIGH_MAX (0x00ffffff) > > > > +#define KONA_DUTY_CYCLE_HIGH_OFFSET (0x0000000c) > > > > + > > > > +#define PWM_CHANNEL_CNT (0x00000006) > > > > +#define SIGNAL_PUSH_PULL (0x00000001) > > > > +#define PWMOUT_TYPE_SHIFT (0x00000010) > > > > + > > > > +#define IPROC_PRESCALE_OFFSET (0x00000024) > > > > +#define IPROC_PRESCALE_SHIFT (0x00000006) > > > > +#define IPROC_PRESCALE_MAX (0x0000003f) > > > > + > > > > +#define IPROC_PERIOD_COUNT_OFFSET (0x00000004) > > > > +#define IPROC_PERIOD_COUNT_MIN (0x00000002) > > > > +#define IPROC_PERIOD_COUNT_MAX (0x0000ffff) > > > > + > > > > +#define IPROC_DUTY_CYCLE_HIGH_OFFSET (0x00000008) > > > > +#define IPROC_DUTY_CYCLE_HIGH_MIN (0x00000000) > > > > +#define IPROC_DUTY_CYCLE_HIGH_MAX (0x0000ffff) > > > > + > > > > +#define IPROC_PWM_CHANNEL_CNT (0x00000004) > > > > +#define IPROC_SIGNAL_PUSH_PULL (0x00000000) > > > > +#define IPROC_PWMOUT_TYPE_SHIFT (0x0000000f) > > > > + > > > > +/* > > > > + * pwm controller reg structure > > > > + * > > > > + * @prescale_offset: prescale register offset > > > > + * @period_offset: period register offset > > > > + * @duty_offset: duty register offset > > > > + * @no_of_channels: number of channels > > > > + * @out_type_shift: out type shift in the register > > > > + * @signal_type: push-pull or open drain > > > > + * @prescale_max: prescale max > > > > + * @prescale_shift: prescale shift in register > > > > + * @prescale_ch_ascending: prescale ch order in prescale register > > > > + * @duty_cycle_max: value of max duty cycle > > > > + * @duty_cycle_min: value of min duty cycle > > > > + * @period_count_max: max period count val > > > > + * @period_count_min: min period count val > > > > + * @smooth_output_support: pwm smooth output support > > > > + */ > > > > +struct kona_pwmc_reg { > > > > + u32 prescale_offset; > > > > + u32 period_offset; > > > > + u32 duty_offset; > > > > + u32 no_of_channels; > > > > + u32 out_type_shift; > > > > + u32 signal_type; > > > > + u32 prescale_max; > > > > + u32 prescale_shift; > > > > + bool prescale_ch_ascending; > > > > + u32 duty_cycle_max; > > > > + u32 duty_cycle_min; > > > > + u32 period_count_max; > > > > + u32 period_count_min; > > > > + bool smooth_output_support; > > > > +}; > > > > > > This is rather tedious. It looks to me like this isn't very similar to > > > the existing driver. Register offsets move around, bitfield positions > > > change, feature set is different. Might be better off turning this into > > > a separate driver after all. > > > > > > > +static const struct kona_pwmc_reg kona_pwmc_reg_data = { > > > > + .prescale_offset = PRESCALE_OFFSET, > > > > + .period_offset = KONA_PERIOD_COUNT_OFFSET, > > > > + .duty_offset = KONA_DUTY_CYCLE_HIGH_OFFSET, > > > > + .no_of_channels = PWM_CHANNEL_CNT, > > > > + .out_type_shift = PWMOUT_TYPE_SHIFT, > > > > + .signal_type = SIGNAL_PUSH_PULL, > > > > + .prescale_max = PRESCALE_MAX, > > > > + .prescale_shift = PRESCALE_SHIFT, > > > > + .prescale_ch_ascending = false, > > > > + .duty_cycle_max = DUTY_CYCLE_HIGH_MAX, > > > > + .duty_cycle_min = DUTY_CYCLE_HIGH_MIN, > > > > + .period_count_max = PERIOD_COUNT_MAX, > > > > + .period_count_min = PERIOD_COUNT_MIN, > > > > + .smooth_output_support = true, > > > > +}; > > > > + > > > > +static const struct kona_pwmc_reg iproc_pwmc_reg_data = { > > > > + .prescale_offset = IPROC_PRESCALE_OFFSET, > > > > + .period_offset = IPROC_PERIOD_COUNT_OFFSET, > > > > + .duty_offset = IPROC_DUTY_CYCLE_HIGH_OFFSET, > > > > + .no_of_channels = IPROC_PWM_CHANNEL_CNT, > > > > + .out_type_shift = IPROC_PWMOUT_TYPE_SHIFT, > > > > + .signal_type = IPROC_SIGNAL_PUSH_PULL, > > > > + .prescale_max = IPROC_PRESCALE_MAX, > > > > + .prescale_shift = IPROC_PRESCALE_SHIFT, > > > > + .prescale_ch_ascending = true, > > > > + .duty_cycle_max = IPROC_DUTY_CYCLE_HIGH_MAX, > > > > + .duty_cycle_min = IPROC_DUTY_CYCLE_HIGH_MIN, > > > > + .period_count_max = IPROC_PERIOD_COUNT_MAX, > > > > + .period_count_min = IPROC_PERIOD_COUNT_MIN, > > > > + .smooth_output_support = false, > > > > +}; > > > > > > This looks like you could possible support a lot more hardware with this > > > driver because it's now almost completely parameterized. > > > > > > I don't see much sense in keeping this in the same driver and I think > > > it'd be better to write a new one from scratch, even if that means > > > slight duplication. > > > > > > Or you'll have to make a very compelling argument as to why this is the > > > better option. > > > > Sorry to enter the discussion just now (Brian CCed me in an answer he > > made to v4 of this series). > > > > Honestly, I'd have the exact opposite argument: if you have a look at > > newer versions of this patch, you'll see that more than 90% of the code > > is duplicated, and re-using the same logic with different field > > positions is quite common in other drivers. Actually the counter > > argument to your suggestion are bug fixes: when you find a bug, you'll > > have to remember fixing it for all implementations. Having a common > > code base has IMO more pros than cons. > > > > BTW, if you switch to regmap, you have this field-position > > customization for free (see reg_field). > > My point was that with the above structures you have an almost > completely parameterized driver. There's probably a number of other PWM > controllers that you could support using those structure, or by adding > to them. The question then becomes: where do we draw the line? When is > a new driver warranted, and when is it not. Well, there's a difference between trying to provide a completely generic PWM driver and trying to factorize code for 2 almost identical IPs (only the field position and width changes here). The generic PWM driver approach is clearly an utopia because all PWM IPs have their own requirements in term of clock source selection, PWM configuration and sequencing constraints. I agree that the decision to factorize common code is subjective, and that the maintainer (you in this case) will always have the final word. > > Your argument regarding bug fixes has some weight, but these drivers are > really trivial, so bug fixes will be equally trivial to port between > them. Well, it's not only about bug fixes, but driver maintenance in general. Let's take the recent atomic PWM rework: we're now recommending to implement ->apply() instead of ->enable()/disable()/config(), and Brian asked Yendapally to rework his implementation to follow this recommendation. If we had chosen for the kona driver enhancement instead of duplicating the code in a new driver we would have the atomic update for the kona IP for free... > > There's also the issue about errata and quirks that may apply to one but > not all supported versions. The outcome of supporting all those > combinations can be completely unmaintainable, so I am willing to take a > bit of duplication if it means that in the end it will make things > easier for me. With only 2 revisions/versions supported, I don't expect to see a lot of different errata, but that's true in general. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH RESEND 3/3] ARM: dts: NSP: Add PWM Support to DT 2016-03-29 14:22 [PATCH RESEND 0/3] pwm support for Broadcom iProc SoC's Yendapally Reddy Dhananjaya Reddy 2016-03-29 14:22 ` [PATCH RESEND 1/3] Documentation: dt: Add Broadcom iproc pwm controller binding Yendapally Reddy Dhananjaya Reddy 2016-03-29 14:22 ` [PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller Yendapally Reddy Dhananjaya Reddy @ 2016-03-29 14:22 ` Yendapally Reddy Dhananjaya Reddy 2016-05-10 13:55 ` [PATCH RESEND 0/3] pwm support for Broadcom iProc SoC's Yendapally Reddy Dhananjaya Reddy 3 siblings, 0 replies; 10+ messages in thread From: Yendapally Reddy Dhananjaya Reddy @ 2016-03-29 14:22 UTC (permalink / raw) To: Thierry Reding, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Russell King, Ray Jui, Scott Branden, Jon Mason Cc: linux-pwm, devicetree, linux-kernel, linux-arm-kernel, bcm-kernel-feedback-list, Yendapally Reddy Dhananjaya Reddy Add PWM support to the device tree for the Broadcom Northstar Plus SoC. Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com> --- arch/arm/boot/dts/bcm-nsp.dtsi | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/arch/arm/boot/dts/bcm-nsp.dtsi b/arch/arm/boot/dts/bcm-nsp.dtsi index def9e78..f9c8341 100644 --- a/arch/arm/boot/dts/bcm-nsp.dtsi +++ b/arch/arm/boot/dts/bcm-nsp.dtsi @@ -206,6 +206,14 @@ brcm,nand-has-wp; }; + pwm: pwm@31000 { + compatible = "brcm,iproc-pwm"; + reg = <0x31000 0x28>; + clocks = <&osc>; + #pwm-cells = <3>; + status = "disabled"; + }; + ccbtimer0: timer@34000 { compatible = "arm,sp804"; reg = <0x34000 0x1000>; -- 2.1.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH RESEND 0/3] pwm support for Broadcom iProc SoC's 2016-03-29 14:22 [PATCH RESEND 0/3] pwm support for Broadcom iProc SoC's Yendapally Reddy Dhananjaya Reddy ` (2 preceding siblings ...) 2016-03-29 14:22 ` [PATCH RESEND 3/3] ARM: dts: NSP: Add PWM Support to DT Yendapally Reddy Dhananjaya Reddy @ 2016-05-10 13:55 ` Yendapally Reddy Dhananjaya Reddy 3 siblings, 0 replies; 10+ messages in thread From: Yendapally Reddy Dhananjaya Reddy @ 2016-05-10 13:55 UTC (permalink / raw) To: Thierry Reding, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Russell King, Ray Jui, Scott Branden, Jon Mason Cc: linux-pwm, devicetree, linux-kernel, linux-arm-kernel, BCM Kernel Feedback, Yendapally Reddy Dhananjaya Reddy Hi Thierry, Can you please provide review comments for this patch set. Thanks Dhananjay On Tue, Mar 29, 2016 at 7:52 PM, Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com> wrote: > This patchset contains the pwm support for the Broadcom's iProc SoC's. > > The first patch provides the documentation details and the > second patch contains the controller support details. The third patch > contains the enable method for Northstar Plus SoC. > > This patch series has been tested on NSP bcm958625HR board with oscilloscope. > The pwm is not enabled for this board as the pins are open and not used. > > This patch series is based on v4.6.0-rc1 and is available from github > repo: https://github.com/Broadcom/cygnus-linux.git > branch: iproc-pwm-v1 > > This patch series was originally sent on 03/01/16. > See https://lkml.org/lkml/2016/3/1/487 > > Yendapally Reddy Dhananjaya Reddy (3): > Documentation: dt: Add Broadcom iproc pwm controller binding > pwm: kona: Add support for Broadcom iproc pwm controller > ARM: dts: NSP: Add PWM Support to DT > > .../devicetree/bindings/pwm/brcm,kona-pwm.txt | 2 +- > arch/arm/boot/dts/bcm-nsp.dtsi | 8 + > drivers/pwm/Kconfig | 6 +- > drivers/pwm/pwm-bcm-kona.c | 183 ++++++++++++++++++--- > 4 files changed, 172 insertions(+), 27 deletions(-) > > -- > 2.1.0 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller @ 2016-05-10 15:11 Yendapally Reddy Dhananjaya Reddy 0 siblings, 0 replies; 10+ messages in thread From: Yendapally Reddy Dhananjaya Reddy @ 2016-05-10 15:11 UTC (permalink / raw) To: Thierry Reding Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Russell King, Ray Jui, Scott Branden, Jon Mason, linux-pwm, devicetree, linux-kernel, linux-arm-kernel, BCM Kernel Feedback Hi Thierry, On Tue, May 10, 2016 at 8:10 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, Mar 29, 2016 at 10:22:29AM -0400, Yendapally Reddy Dhananjaya Reddy wrote: >> Update the kona driver to support Broadcom iproc pwm controller >> >> Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com> >> --- >> drivers/pwm/Kconfig | 6 +- >> drivers/pwm/pwm-bcm-kona.c | 183 +++++++++++++++++++++++++++++++++++++++------ >> 2 files changed, 163 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig >> index c182efc..e45ea33 100644 >> --- a/drivers/pwm/Kconfig >> +++ b/drivers/pwm/Kconfig >> @@ -76,9 +76,11 @@ config PWM_ATMEL_TCB >> >> config PWM_BCM_KONA >> tristate "Kona PWM support" >> - depends on ARCH_BCM_MOBILE >> + depends on ARCH_BCM_MOBILE || ARCH_BCM_IPROC >> + default ARCH_BCM_IPROC > > Why the default? Typically you'd enable this in one or more of the > default configurations. default ARCH_* is really only useful if the > driver is essential. PWM doesn't usually fall into this category. > >> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c >> index c634183..ef152e3a 100644 >> --- a/drivers/pwm/pwm-bcm-kona.c >> +++ b/drivers/pwm/pwm-bcm-kona.c >> @@ -19,6 +19,7 @@ >> #include <linux/math64.h> >> #include <linux/module.h> >> #include <linux/of.h> >> +#include <linux/of_device.h> >> #include <linux/platform_device.h> >> #include <linux/pwm.h> >> #include <linux/slab.h> >> @@ -47,30 +48,90 @@ >> >> #define PWM_CONTROL_OFFSET (0x00000000) >> #define PWM_CONTROL_SMOOTH_SHIFT(chan) (24 + (chan)) >> -#define PWM_CONTROL_TYPE_SHIFT(chan) (16 + (chan)) >> +#define PWM_CONTROL_TYPE_SHIFT(shift, chan) (shift + chan) > > You need to put the parameters into parentheses to avoid expansion from > potentially messing up the expression. > >> #define PWM_CONTROL_POLARITY_SHIFT(chan) (8 + (chan)) >> #define PWM_CONTROL_TRIGGER_SHIFT(chan) (chan) >> >> #define PRESCALE_OFFSET (0x00000004) >> -#define PRESCALE_SHIFT(chan) ((chan) << 2) >> -#define PRESCALE_MASK(chan) (0x7 << PRESCALE_SHIFT(chan)) >> +#define PRESCALE_SHIFT (0x00000004) >> +#define PRESCALE_MASK (0x00000007) > > Hmm... this looks odd. Why are you dropping the chan parameter here? > >> #define PRESCALE_MIN (0x00000000) >> #define PRESCALE_MAX (0x00000007) >> >> -#define PERIOD_COUNT_OFFSET(chan) (0x00000008 + ((chan) << 3)) >> +#define PERIOD_COUNT_OFFSET(offset, chan) (offset + (chan << 3)) > > Need parentheses here as well. > >> #define PERIOD_COUNT_MIN (0x00000002) >> #define PERIOD_COUNT_MAX (0x00ffffff) >> +#define KONA_PERIOD_COUNT_OFFSET (0x00000008) >> >> -#define DUTY_CYCLE_HIGH_OFFSET(chan) (0x0000000c + ((chan) << 3)) >> +#define DUTY_CYCLE_HIGH_OFFSET(offset, chan) (offset + (chan << 3)) >> #define DUTY_CYCLE_HIGH_MIN (0x00000000) >> #define DUTY_CYCLE_HIGH_MAX (0x00ffffff) >> +#define KONA_DUTY_CYCLE_HIGH_OFFSET (0x0000000c) >> + >> +#define PWM_CHANNEL_CNT (0x00000006) >> +#define SIGNAL_PUSH_PULL (0x00000001) >> +#define PWMOUT_TYPE_SHIFT (0x00000010) >> + >> +#define IPROC_PRESCALE_OFFSET (0x00000024) >> +#define IPROC_PRESCALE_SHIFT (0x00000006) >> +#define IPROC_PRESCALE_MAX (0x0000003f) >> + >> +#define IPROC_PERIOD_COUNT_OFFSET (0x00000004) >> +#define IPROC_PERIOD_COUNT_MIN (0x00000002) >> +#define IPROC_PERIOD_COUNT_MAX (0x0000ffff) >> + >> +#define IPROC_DUTY_CYCLE_HIGH_OFFSET (0x00000008) >> +#define IPROC_DUTY_CYCLE_HIGH_MIN (0x00000000) >> +#define IPROC_DUTY_CYCLE_HIGH_MAX (0x0000ffff) >> + >> +#define IPROC_PWM_CHANNEL_CNT (0x00000004) >> +#define IPROC_SIGNAL_PUSH_PULL (0x00000000) >> +#define IPROC_PWMOUT_TYPE_SHIFT (0x0000000f) >> + >> +/* >> + * pwm controller reg structure >> + * >> + * @prescale_offset: prescale register offset >> + * @period_offset: period register offset >> + * @duty_offset: duty register offset >> + * @no_of_channels: number of channels >> + * @out_type_shift: out type shift in the register >> + * @signal_type: push-pull or open drain >> + * @prescale_max: prescale max >> + * @prescale_shift: prescale shift in register >> + * @prescale_ch_ascending: prescale ch order in prescale register >> + * @duty_cycle_max: value of max duty cycle >> + * @duty_cycle_min: value of min duty cycle >> + * @period_count_max: max period count val >> + * @period_count_min: min period count val >> + * @smooth_output_support: pwm smooth output support >> + */ >> +struct kona_pwmc_reg { >> + u32 prescale_offset; >> + u32 period_offset; >> + u32 duty_offset; >> + u32 no_of_channels; >> + u32 out_type_shift; >> + u32 signal_type; >> + u32 prescale_max; >> + u32 prescale_shift; >> + bool prescale_ch_ascending; >> + u32 duty_cycle_max; >> + u32 duty_cycle_min; >> + u32 period_count_max; >> + u32 period_count_min; >> + bool smooth_output_support; >> +}; > > This is rather tedious. It looks to me like this isn't very similar to > the existing driver. Register offsets move around, bitfield positions > change, feature set is different. Might be better off turning this into > a separate driver after all. > >> +static const struct kona_pwmc_reg kona_pwmc_reg_data = { >> + .prescale_offset = PRESCALE_OFFSET, >> + .period_offset = KONA_PERIOD_COUNT_OFFSET, >> + .duty_offset = KONA_DUTY_CYCLE_HIGH_OFFSET, >> + .no_of_channels = PWM_CHANNEL_CNT, >> + .out_type_shift = PWMOUT_TYPE_SHIFT, >> + .signal_type = SIGNAL_PUSH_PULL, >> + .prescale_max = PRESCALE_MAX, >> + .prescale_shift = PRESCALE_SHIFT, >> + .prescale_ch_ascending = false, >> + .duty_cycle_max = DUTY_CYCLE_HIGH_MAX, >> + .duty_cycle_min = DUTY_CYCLE_HIGH_MIN, >> + .period_count_max = PERIOD_COUNT_MAX, >> + .period_count_min = PERIOD_COUNT_MIN, >> + .smooth_output_support = true, >> +}; >> + >> +static const struct kona_pwmc_reg iproc_pwmc_reg_data = { >> + .prescale_offset = IPROC_PRESCALE_OFFSET, >> + .period_offset = IPROC_PERIOD_COUNT_OFFSET, >> + .duty_offset = IPROC_DUTY_CYCLE_HIGH_OFFSET, >> + .no_of_channels = IPROC_PWM_CHANNEL_CNT, >> + .out_type_shift = IPROC_PWMOUT_TYPE_SHIFT, >> + .signal_type = IPROC_SIGNAL_PUSH_PULL, >> + .prescale_max = IPROC_PRESCALE_MAX, >> + .prescale_shift = IPROC_PRESCALE_SHIFT, >> + .prescale_ch_ascending = true, >> + .duty_cycle_max = IPROC_DUTY_CYCLE_HIGH_MAX, >> + .duty_cycle_min = IPROC_DUTY_CYCLE_HIGH_MIN, >> + .period_count_max = IPROC_PERIOD_COUNT_MAX, >> + .period_count_min = IPROC_PERIOD_COUNT_MIN, >> + .smooth_output_support = false, >> +}; > > This looks like you could possible support a lot more hardware with this > driver because it's now almost completely parameterized. > > I don't see much sense in keeping this in the same driver and I think > it'd be better to write a new one from scratch, even if that means > slight duplication. > These two variants have the same block with different reg offsets and bit fields. I thought of having a same driver, will address the fixes for both if any. I will generate a new one as suggested and submit a v2 version. > Or you'll have to make a very compelling argument as to why this is the > better option. > > Thierry Thanks Dhananjay ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-07-04 13:37 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-29 14:22 [PATCH RESEND 0/3] pwm support for Broadcom iProc SoC's Yendapally Reddy Dhananjaya Reddy 2016-03-29 14:22 ` [PATCH RESEND 1/3] Documentation: dt: Add Broadcom iproc pwm controller binding Yendapally Reddy Dhananjaya Reddy 2016-03-29 14:22 ` [PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller Yendapally Reddy Dhananjaya Reddy 2016-05-10 14:40 ` Thierry Reding 2016-06-23 20:32 ` Boris Brezillon 2016-06-28 12:52 ` Thierry Reding 2016-07-04 13:37 ` Boris Brezillon 2016-03-29 14:22 ` [PATCH RESEND 3/3] ARM: dts: NSP: Add PWM Support to DT Yendapally Reddy Dhananjaya Reddy 2016-05-10 13:55 ` [PATCH RESEND 0/3] pwm support for Broadcom iProc SoC's Yendapally Reddy Dhananjaya Reddy 2016-05-10 15:11 [PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller Yendapally Reddy Dhananjaya Reddy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).