Re: [PATCH 2/3] pwm: add a driver for the STMPE PWM

[Lee Jones <lee.jones@linaro.org>]

On Wed, 10 Feb 2016, Linus Walleij wrote:

> This adds a driver for the STMPE 24xx series of multi-purpose
> I2C expanders. (I think STMPE means ST Microelectronics
> Multi-Purpose Expander.) This PWM was designed in accordance with
> Nokia specifications and is kind of weird and usually just
> switched between max and zero dutycycle. However it is indeed
> a PWM so it needs to live in the PWM subsystem.
> 
> This PWM is mostly used for white LED backlight.
> 
> Cc: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Lee could you ACK the few lines added to drivers/mfd/stmpe.c
> so that the PWM maintainers can take this through their tree?

Hells no! ;)

> Thanks.
> ---
>  drivers/mfd/stmpe.c     |  37 ++++++
>  drivers/pwm/Kconfig     |   7 ++
>  drivers/pwm/Makefile    |   1 +
>  drivers/pwm/pwm-stmpe.c | 297 ++++++++++++++++++++++++++++++++++++++++++++++++

Any reason why you're bundling up changes in two subsystems?

>  4 files changed, 342 insertions(+)
>  create mode 100644 drivers/pwm/pwm-stmpe.c
> 
> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
> index 8222e374e4b1..4eb08e0471f8 100644
> --- a/drivers/mfd/stmpe.c
> +++ b/drivers/mfd/stmpe.c
> @@ -333,6 +333,33 @@ static const struct mfd_cell stmpe_keypad_cell = {
>  	.num_resources	= ARRAY_SIZE(stmpe_keypad_resources),
>  };
>  
> +

Superflous '\n'.

> +/*
> + * PWM (1601, 2401, 2403)
> + */
> +static struct resource stmpe_pwm_resources[] = {
> +	{
> +		.name	= "PWM0",
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +	{
> +		.name	= "PWM1",
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +	{
> +		.name	= "PWM2",
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +};

I prefer the DEFINE_RES_* stuff these days.

> +static const struct mfd_cell stmpe_pwm_cell = {
> +	.name		= "stmpe-pwm",
> +	.of_compatible  = "st,stmpe-pwm",
> +	.resources	= stmpe_pwm_resources,
> +	.num_resources	= ARRAY_SIZE(stmpe_pwm_resources),
> +};
> +
> +

What's with the '\n\n'?

>  /*
>   * STMPE801
>   */
> @@ -537,6 +564,11 @@ static struct stmpe_variant_block stmpe1601_blocks[] = {
>  		.irq	= STMPE1601_IRQ_KEYPAD,
>  		.block	= STMPE_BLOCK_KEYPAD,
>  	},
> +	{
> +		.cell	= &stmpe_pwm_cell,
> +		.irq	= STMPE1601_IRQ_PWM0,
> +		.block	= STMPE_BLOCK_PWM,
> +	},
>  };
>  
>  /* supported autosleep timeout delay (in msecs) */
> @@ -771,6 +803,11 @@ static struct stmpe_variant_block stmpe24xx_blocks[] = {
>  		.irq	= STMPE24XX_IRQ_KEYPAD,
>  		.block	= STMPE_BLOCK_KEYPAD,
>  	},
> +	{
> +		.cell	= &stmpe_pwm_cell,
> +		.irq	= STMPE24XX_IRQ_PWM0,
> +		.block	= STMPE_BLOCK_PWM,
> +	},
>  };

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog