Hi, On Tue, 10 May 2016 16:40:24 +0200 Thierry Reding <[email protected]> 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 > > <[email protected]> > > --- > > 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
