Re: [PATCH RESEND 2/3] pwm: kona: Add support for Broadcom iproc pwm controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Thierry,

On Tue, May 10, 2016 at 8:10 PM, Thierry Reding
<thierry.reding@xxxxxxxxx> 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@xxxxxxxxxxxx>
>> ---
>>  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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux