Re: [PATCH] pwm:lpss: update pwm setting for Broxton

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3035 bytes --]

On Thu, Nov 12, 2015 at 04:50:25PM +0200, Mika Westerberg wrote:
> On Thu, Nov 12, 2015 at 01:52:36PM +0100, Thierry Reding wrote:
> > On Fri, Nov 13, 2015 at 12:28:04AM +0800, Qipeng Zha wrote:
> > > For Broxton PWM controller, base unit is defined as 8bit integer
> > > and 14bit fraction, so need to update base unit setting to output
> > > wave with right frequency.
> > > a) add scaler for each board setting;
> > > b) remove validity check of base unit for special board, let pwm
> > > user to handle this;
> > > 
> > > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> > > ---
> > >  drivers/pwm/pwm-lpss.c | 20 +++++++++++---------
> > >  drivers/pwm/pwm-lpss.h |  1 +
> > >  2 files changed, 12 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> > > index 2504410..5a907db 100644
> > > --- a/drivers/pwm/pwm-lpss.c
> > > +++ b/drivers/pwm/pwm-lpss.c
> > > @@ -14,6 +14,7 @@
> > >   */
> > >  
> > >  #include <linux/io.h>
> > > +#include <linux/time.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/module.h>
> > >  #include <linux/pm_runtime.h>
> > > @@ -24,11 +25,9 @@
> > >  #define PWM_ENABLE			BIT(31)
> > >  #define PWM_SW_UPDATE			BIT(30)
> > >  #define PWM_BASE_UNIT_SHIFT		8
> > > -#define PWM_BASE_UNIT_MASK		0x00ffff00
> > > +#define PWM_BASE_UNIT_MASK		0x3fffff00
> > 
> > Isn't this going to potentially write reserved bits on non-Broxton
> > platforms? Previously the upper 8 bits were masked out, but now only the
> > upper 2 bits are masked out. What about the other 6?
> > 
> > Perhaps it'd be better to parameterize the mask in a way similar to the
> > scaler value?
> 
> Or call the field "base_unit_bits" which holds 16 for BSW/BYT and 22 for
> BXT, and calculate both scaler and mask from that in pwm_lpss_config()?

Yeah, that should work as well.

> Actually I think we should make the driver private structure look like
> this:
> 
> struct pwm_lpss_chip {
>         struct pwm_chip chip;
>         void __iomem *regs;
>         const struct pwm_lpss_boardinfo *info;
> };
> 
> and then set ->info to point to the platform data in probe().
> 
> pwm_lpss_config() can then refer lpwm->info->base_unit_bits and
> lpwm->info->clk_rate.

Yeah, that thought had crossed my mind as well. I would've probably
waited until a third field was copied from info to chip before
requesting that change, but since you already brought it up, feel free
to send a patch.

> > Also it's unclear to me how critical this is. Initial patches for
> > Broxton support were merged into Linus' tree yesterday. Presumably they
> > had received some testing before, but nobody can't have noticed or this
> > bug wouldn't exist in the patches that were merged. Does this break any
> > existing setups and hence should go into v4.4 along with the initial
> > Broxton support?
> 
> Nobody outside Intel should have Broxtons yet so it should not break any
> existing users.

Okay.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]