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

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

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

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?

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?

Thierry

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