diff mbox

[6/8] pwm: crc: Add Crystalcove (CRC) PWM driver

Message ID 1430818716-27287-1-git-send-email-shobhit.kumar@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kumar, Shobhit May 5, 2015, 9:38 a.m. UTC
The Crystalcove PMIC controls PWM signals and this driver exports that
capability as a PWM chip driver. This is platform device implementtaion
of the drivers/mfd cell device for CRC PMIC

v2: Use the existing config callback with duty_ns and period_ns(Thierry)

v3: Correct the subject line (Lee jones)

CC: Samuel Ortiz <sameo@linux.intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/pwm/Kconfig   |   7 +++
 drivers/pwm/Makefile  |   1 +
 drivers/pwm/pwm-crc.c | 171 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 179 insertions(+)
 create mode 100644 drivers/pwm/pwm-crc.c

Comments

Paul Bolle May 6, 2015, 7:40 a.m. UTC | #1
On Tue, 2015-05-05 at 15:08 +0530, Shobhit Kumar wrote:
> The Crystalcove PMIC controls PWM signals and this driver exports that
> capability as a PWM chip driver. This is platform device implementtaion
> of the drivers/mfd cell device for CRC PMIC
> 
> v2: Use the existing config callback with duty_ns and period_ns(Thierry)
> 
> v3: Correct the subject line (Lee jones)
> 
> CC: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>

The same comments can be made as for v2, see
http://lkml.kernel.org/r/1430428322.2187.24.camel@x220 . Maybe you
didn't receive that message.

It could also be that you think my comments were invalid, or too vague,
or whatever. Please say so, because then I don't have to bother you
again when you send out v4.

Thanks,


Paul Bolle
Thierry Reding May 6, 2015, 12:14 p.m. UTC | #2
On Tue, May 05, 2015 at 03:08:36PM +0530, Shobhit Kumar wrote:
> The Crystalcove PMIC controls PWM signals and this driver exports that

You say signal_s_ here, but you only expose a single PWM device. Does
the PMIC really control more than one? If it isn't, this should probably
become: "controls a PWM output and this driver...".

> capability as a PWM chip driver. This is platform device implementtaion

"implementation"

> of the drivers/mfd cell device for CRC PMIC

Sentences should end with a full stop.

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b1541f4..954da3e 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -183,6 +183,13 @@ config PWM_LPC32XX
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-lpc32xx.
>  
> +config PWM_CRC
> +	bool "Intel Crystalcove (CRC) PWM support"
> +	depends on X86 && INTEL_SOC_PMIC
> +	help
> +	  Generic PWM framework driver for Crystalcove (CRC) PMIC based PWM
> +	  control.
> +

This is badly sorted. Please keep the list sorted alphabetically.

>  config PWM_LPSS
>  	tristate "Intel LPSS PWM support"
>  	depends on X86
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index ec50eb5..3d38fed 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -35,3 +35,4 @@ obj-$(CONFIG_PWM_TIPWMSS)	+= pwm-tipwmss.o
>  obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
>  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
>  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
> +obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o

This too.

> diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c
> new file mode 100644
> index 0000000..987f3b4
> --- /dev/null
> +++ b/drivers/pwm/pwm-crc.c
> @@ -0,0 +1,171 @@
> +/*
> + * pwm-crc.c - Intel Crystal Cove PWM Driver

I think you can safely remove this line. You already know what file it
is when you open it in your editor, and the description is in the
MODULE_DESCRIPTION string already.

> + *
> + * Copyright (C) 2015 Intel Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * Author: Shobhit Kumar <shobhit.kumar@intel.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +#include <linux/pwm.h>
> +
> +#define PWM0_CLK_DIV		0x4B
> +#define  PWM_OUTPUT_ENABLE	(1<<7)

Should have spaces around <<.

> +#define  PWM_DIV_CLK_0		0x00 /* DIVIDECLK = BASECLK */
> +#define  PWM_DIV_CLK_100	0x63 /* DIVIDECLK = BASECLK/100 */
> +#define  PWM_DIV_CLK_128	0x7F /* DIVIDECLK = BASECLK/128 */
> +
> +#define PWM0_DUTY_CYCLE		0x4E
> +#define BACKLIGHT_EN		0x51
> +
> +#define PWM_MAX_LEVEL		0xFF
> +
> +#define PWM_BASE_CLK		6000	/* 6 MHz */

This number is actually 6 KHz. I think it'd be better if you stuck with
one unit here. Or perhaps there's some other reason why you can't use
6000000 here instead?

> +#define PWM_MAX_PERIOD_NS	21333 /* 46.875KHz */
> +
> +/**
> + * struct crystalcove_pwm - Crystal Cove PWM controller
> + * @chip: the abstract pwm_chip structure.
> + * @regmap: the regmap from the parent device.
> + */
> +struct crystalcove_pwm {
> +	struct pwm_chip chip;
> +	struct platform_device *pdev;

I think I had at some point requested that you get rid of this and use
the chip.dev member instead. There's no kerneldoc for it and it isn't
(well, almost, see below) used anywhere else, so perhaps you forgot to
remove it here?

> +	struct regmap *regmap;
> +};
> +
> +static inline struct crystalcove_pwm *to_crc_pwm(struct pwm_chip *pc)
> +{
> +	return container_of(pc, struct crystalcove_pwm, chip);
> +}
> +
> +static int crc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm)
> +{
> +	struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
> +
> +	regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 1);
> +
> +	return 0;
> +}
> +
> +static void crc_pwm_disable(struct pwm_chip *c, struct pwm_device *pwm)
> +{
> +	struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
> +
> +	regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 0);
> +}
> +
> +static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm,
> +				  int duty_ns, int period_ns)

Please align arguments on subsequent lines with the first argument of
the first line.

> +{
> +	struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
> +	struct device *dev = &crc_pwm->pdev->dev;

Did you test reconfiguring the PWM? I don't see crc_pwm->pdev getting
initialized anywhere, so this should crash trying to dereference a NULL
pointer.

Of course if you get rid of the pdev field as I suggested you can simply
get the struct device * from c->dev.

> +	int level, percent;
> +
> +	if (period_ns > PWM_MAX_PERIOD_NS) {
> +		dev_err(dev, "un-supported period_ns\n");
> +		return -1;

You should return -EINVAL here. Besides being a literal and therefore a
bad idea, -1 == -EPERM and doesn't match the error condition.

> +	}
> +
> +	if (pwm->period != period_ns) {
> +		int clk_div;
> +
> +		/* changing the clk divisor, need to disable fisrt */
> +		crc_pwm_disable(c, pwm);
> +		clk_div = PWM_BASE_CLK * period_ns / 1000000;

Similar to the above, this is confusing because you're mixing up
different scales here. period_ns is in nanoseconds, so it'd be natural
to divide by 1000000000 (though you should really be using NSEC_PER_SEC
instead). If you counterweight that by expressing PWM_BASE_CLK in Hz
(6000000) you get much nicer symmetry and make the code a lot easier to
understand.

> +
> +		regmap_write(crc_pwm->regmap, PWM0_CLK_DIV,
> +					clk_div | PWM_OUTPUT_ENABLE);
> +
> +		/* enable back */
> +		crc_pwm_enable(c, pwm);
> +	}
> +
> +	if (duty_ns > period_ns) {
> +		dev_err(dev, "duty cycle cannot be greater than cycle period\n");
> +		return -1;
> +	}

The PWM core already performs this check, so you'll never get here in
case this condition is true.

> +
> +	/* change the pwm duty cycle */
> +	percent = duty_ns * 100 / period_ns;
> +	level = percent * PWM_MAX_LEVEL / 100;

Why do you need to apply the rule of three twice here? Doesn't

	level = duty_ns * PWM_MAX_LEVEL / period_ns;

give you what you want?

> +	regmap_write(crc_pwm->regmap, PWM0_DUTY_CYCLE, level);
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops crc_pwm_ops = {
> +	.config = crc_pwm_config,
> +	.enable = crc_pwm_enable,
> +	.disable = crc_pwm_disable,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int crystalcove_pwm_probe(struct platform_device *pdev)
> +{
> +	struct crystalcove_pwm *pwm;
> +	int retval;
> +	struct device *dev = pdev->dev.parent;
> +	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
> +
> +	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	pwm->chip.dev = &pdev->dev;
> +	pwm->chip.ops = &crc_pwm_ops;
> +	pwm->chip.base = -1;
> +	pwm->chip.npwm = 1;
> +
> +	/* get the PMIC regmap */
> +	pwm->regmap = pmic->regmap;
> +
> +	retval = pwmchip_add(&pwm->chip);
> +	if (retval < 0)
> +		return retval;
> +
> +	dev_dbg(&pdev->dev, "crc-pwm probe successful\n");

Do you really want this? The driver core will complain in any of the
above failures, so what use is there to be chatty when probing
succeeds?

> +static struct platform_driver crystalcove_pwm_driver = {
> +	.probe = crystalcove_pwm_probe,
> +	.remove = crystalcove_pwm_remove,
> +	.driver = {
> +		.name = "crystal_cove_pwm",

I'd prefer this to be "crystal-cove-pwm" for consistency with other
drivers, but since the MFD part already uses underscores in names it'd
introduce an inconsistency there. So I'm fine with this one as-is.

Thierry
Shobhit Kumar May 7, 2015, 7:13 a.m. UTC | #3
On Wed, May 6, 2015 at 1:10 PM, Paul Bolle <pebolle@tiscali.nl> wrote:
> On Tue, 2015-05-05 at 15:08 +0530, Shobhit Kumar wrote:
>> The Crystalcove PMIC controls PWM signals and this driver exports that
>> capability as a PWM chip driver. This is platform device implementtaion
>> of the drivers/mfd cell device for CRC PMIC
>>
>> v2: Use the existing config callback with duty_ns and period_ns(Thierry)
>>
>> v3: Correct the subject line (Lee jones)
>>
>> CC: Samuel Ortiz <sameo@linux.intel.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Alexandre Courbot <gnurou@gmail.com>
>> Cc: Thierry Reding <thierry.reding@gmail.com>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>
> The same comments can be made as for v2, see
> http://lkml.kernel.org/r/1430428322.2187.24.camel@x220 . Maybe you
> didn't receive that message.
>
> It could also be that you think my comments were invalid, or too vague,
> or whatever. Please say so, because then I don't have to bother you
> again when you send out v4.
>

Not at all, I just missed your comments and realise my mistake later
after sending next update. Somehow the mailing list filters that I
have setup are not working correctly. I will look into your comments.

Regards
Shobhit
Shobhit Kumar May 7, 2015, 7:19 a.m. UTC | #4
On Wed, May 6, 2015 at 5:44 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, May 05, 2015 at 03:08:36PM +0530, Shobhit Kumar wrote:
>> The Crystalcove PMIC controls PWM signals and this driver exports that
>
> You say signal_s_ here, but you only expose a single PWM device. Does
> the PMIC really control more than one? If it isn't, this should probably
> become: "controls a PWM output and this driver...".

Actually it does support 3 of them but on the platform only one is
being used and I exported only that as of now. Probably I should
expand a little in the commit message indicating this. will re-post
after fixing based on your other comments.

Regards
Shobhit

>
>> capability as a PWM chip driver. This is platform device implementtaion
>
> "implementation"
>
>> of the drivers/mfd cell device for CRC PMIC
>
> Sentences should end with a full stop.
>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index b1541f4..954da3e 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -183,6 +183,13 @@ config PWM_LPC32XX
>>         To compile this driver as a module, choose M here: the module
>>         will be called pwm-lpc32xx.
>>
>> +config PWM_CRC
>> +     bool "Intel Crystalcove (CRC) PWM support"
>> +     depends on X86 && INTEL_SOC_PMIC
>> +     help
>> +       Generic PWM framework driver for Crystalcove (CRC) PMIC based PWM
>> +       control.
>> +
>
> This is badly sorted. Please keep the list sorted alphabetically.
>
>>  config PWM_LPSS
>>       tristate "Intel LPSS PWM support"
>>       depends on X86
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index ec50eb5..3d38fed 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -35,3 +35,4 @@ obj-$(CONFIG_PWM_TIPWMSS)   += pwm-tipwmss.o
>>  obj-$(CONFIG_PWM_TWL)                += pwm-twl.o
>>  obj-$(CONFIG_PWM_TWL_LED)    += pwm-twl-led.o
>>  obj-$(CONFIG_PWM_VT8500)     += pwm-vt8500.o
>> +obj-$(CONFIG_PWM_CRC)                += pwm-crc.o
>
> This too.
>
>> diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c
>> new file mode 100644
>> index 0000000..987f3b4
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-crc.c
>> @@ -0,0 +1,171 @@
>> +/*
>> + * pwm-crc.c - Intel Crystal Cove PWM Driver
>
> I think you can safely remove this line. You already know what file it
> is when you open it in your editor, and the description is in the
> MODULE_DESCRIPTION string already.
>
>> + *
>> + * Copyright (C) 2015 Intel Corporation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License version
>> + * 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * Author: Shobhit Kumar <shobhit.kumar@intel.com>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/intel_soc_pmic.h>
>> +#include <linux/pwm.h>
>> +
>> +#define PWM0_CLK_DIV         0x4B
>> +#define  PWM_OUTPUT_ENABLE   (1<<7)
>
> Should have spaces around <<.
>
>> +#define  PWM_DIV_CLK_0               0x00 /* DIVIDECLK = BASECLK */
>> +#define  PWM_DIV_CLK_100     0x63 /* DIVIDECLK = BASECLK/100 */
>> +#define  PWM_DIV_CLK_128     0x7F /* DIVIDECLK = BASECLK/128 */
>> +
>> +#define PWM0_DUTY_CYCLE              0x4E
>> +#define BACKLIGHT_EN         0x51
>> +
>> +#define PWM_MAX_LEVEL                0xFF
>> +
>> +#define PWM_BASE_CLK         6000    /* 6 MHz */
>
> This number is actually 6 KHz. I think it'd be better if you stuck with
> one unit here. Or perhaps there's some other reason why you can't use
> 6000000 here instead?
>
>> +#define PWM_MAX_PERIOD_NS    21333 /* 46.875KHz */
>> +
>> +/**
>> + * struct crystalcove_pwm - Crystal Cove PWM controller
>> + * @chip: the abstract pwm_chip structure.
>> + * @regmap: the regmap from the parent device.
>> + */
>> +struct crystalcove_pwm {
>> +     struct pwm_chip chip;
>> +     struct platform_device *pdev;
>
> I think I had at some point requested that you get rid of this and use
> the chip.dev member instead. There's no kerneldoc for it and it isn't
> (well, almost, see below) used anywhere else, so perhaps you forgot to
> remove it here?
>
>> +     struct regmap *regmap;
>> +};
>> +
>> +static inline struct crystalcove_pwm *to_crc_pwm(struct pwm_chip *pc)
>> +{
>> +     return container_of(pc, struct crystalcove_pwm, chip);
>> +}
>> +
>> +static int crc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm)
>> +{
>> +     struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
>> +
>> +     regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 1);
>> +
>> +     return 0;
>> +}
>> +
>> +static void crc_pwm_disable(struct pwm_chip *c, struct pwm_device *pwm)
>> +{
>> +     struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
>> +
>> +     regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 0);
>> +}
>> +
>> +static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm,
>> +                               int duty_ns, int period_ns)
>
> Please align arguments on subsequent lines with the first argument of
> the first line.
>
>> +{
>> +     struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
>> +     struct device *dev = &crc_pwm->pdev->dev;
>
> Did you test reconfiguring the PWM? I don't see crc_pwm->pdev getting
> initialized anywhere, so this should crash trying to dereference a NULL
> pointer.
>
> Of course if you get rid of the pdev field as I suggested you can simply
> get the struct device * from c->dev.
>
>> +     int level, percent;
>> +
>> +     if (period_ns > PWM_MAX_PERIOD_NS) {
>> +             dev_err(dev, "un-supported period_ns\n");
>> +             return -1;
>
> You should return -EINVAL here. Besides being a literal and therefore a
> bad idea, -1 == -EPERM and doesn't match the error condition.
>
>> +     }
>> +
>> +     if (pwm->period != period_ns) {
>> +             int clk_div;
>> +
>> +             /* changing the clk divisor, need to disable fisrt */
>> +             crc_pwm_disable(c, pwm);
>> +             clk_div = PWM_BASE_CLK * period_ns / 1000000;
>
> Similar to the above, this is confusing because you're mixing up
> different scales here. period_ns is in nanoseconds, so it'd be natural
> to divide by 1000000000 (though you should really be using NSEC_PER_SEC
> instead). If you counterweight that by expressing PWM_BASE_CLK in Hz
> (6000000) you get much nicer symmetry and make the code a lot easier to
> understand.
>
>> +
>> +             regmap_write(crc_pwm->regmap, PWM0_CLK_DIV,
>> +                                     clk_div | PWM_OUTPUT_ENABLE);
>> +
>> +             /* enable back */
>> +             crc_pwm_enable(c, pwm);
>> +     }
>> +
>> +     if (duty_ns > period_ns) {
>> +             dev_err(dev, "duty cycle cannot be greater than cycle period\n");
>> +             return -1;
>> +     }
>
> The PWM core already performs this check, so you'll never get here in
> case this condition is true.
>
>> +
>> +     /* change the pwm duty cycle */
>> +     percent = duty_ns * 100 / period_ns;
>> +     level = percent * PWM_MAX_LEVEL / 100;
>
> Why do you need to apply the rule of three twice here? Doesn't
>
>         level = duty_ns * PWM_MAX_LEVEL / period_ns;
>
> give you what you want?
>
>> +     regmap_write(crc_pwm->regmap, PWM0_DUTY_CYCLE, level);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct pwm_ops crc_pwm_ops = {
>> +     .config = crc_pwm_config,
>> +     .enable = crc_pwm_enable,
>> +     .disable = crc_pwm_disable,
>> +     .owner = THIS_MODULE,
>> +};
>> +
>> +static int crystalcove_pwm_probe(struct platform_device *pdev)
>> +{
>> +     struct crystalcove_pwm *pwm;
>> +     int retval;
>> +     struct device *dev = pdev->dev.parent;
>> +     struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
>> +
>> +     pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
>> +     if (!pwm)
>> +             return -ENOMEM;
>> +
>> +     pwm->chip.dev = &pdev->dev;
>> +     pwm->chip.ops = &crc_pwm_ops;
>> +     pwm->chip.base = -1;
>> +     pwm->chip.npwm = 1;
>> +
>> +     /* get the PMIC regmap */
>> +     pwm->regmap = pmic->regmap;
>> +
>> +     retval = pwmchip_add(&pwm->chip);
>> +     if (retval < 0)
>> +             return retval;
>> +
>> +     dev_dbg(&pdev->dev, "crc-pwm probe successful\n");
>
> Do you really want this? The driver core will complain in any of the
> above failures, so what use is there to be chatty when probing
> succeeds?
>
>> +static struct platform_driver crystalcove_pwm_driver = {
>> +     .probe = crystalcove_pwm_probe,
>> +     .remove = crystalcove_pwm_remove,
>> +     .driver = {
>> +             .name = "crystal_cove_pwm",
>
> I'd prefer this to be "crystal-cove-pwm" for consistency with other
> drivers, but since the MFD part already uses underscores in names it'd
> introduce an inconsistency there. So I'm fine with this one as-is.
>
> Thierry
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Shobhit Kumar May 20, 2015, 3:09 p.m. UTC | #5
On Thu, May 7, 2015 at 12:49 PM, Shobhit Kumar <kumar@shobhit.info> wrote:
> On Wed, May 6, 2015 at 5:44 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
>> On Tue, May 05, 2015 at 03:08:36PM +0530, Shobhit Kumar wrote:
>>> The Crystalcove PMIC controls PWM signals and this driver exports that
>>
>> You say signal_s_ here, but you only expose a single PWM device. Does
>> the PMIC really control more than one? If it isn't, this should probably
>> become: "controls a PWM output and this driver...".
>
> Actually it does support 3 of them but on the platform only one is
> being used and I exported only that as of now. Probably I should
> expand a little in the commit message indicating this. will re-post
> after fixing based on your other comments.

Updates pending due to personal leave. Can be expected next week.

Regards
Shobhit

>
> Regards
> Shobhit
>
>>
>>> capability as a PWM chip driver. This is platform device implementtaion
>>
>> "implementation"
>>
>>> of the drivers/mfd cell device for CRC PMIC
>>
>> Sentences should end with a full stop.
>>
>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>> index b1541f4..954da3e 100644
>>> --- a/drivers/pwm/Kconfig
>>> +++ b/drivers/pwm/Kconfig
>>> @@ -183,6 +183,13 @@ config PWM_LPC32XX
>>>         To compile this driver as a module, choose M here: the module
>>>         will be called pwm-lpc32xx.
>>>
>>> +config PWM_CRC
>>> +     bool "Intel Crystalcove (CRC) PWM support"
>>> +     depends on X86 && INTEL_SOC_PMIC
>>> +     help
>>> +       Generic PWM framework driver for Crystalcove (CRC) PMIC based PWM
>>> +       control.
>>> +
>>
>> This is badly sorted. Please keep the list sorted alphabetically.
>>
>>>  config PWM_LPSS
>>>       tristate "Intel LPSS PWM support"
>>>       depends on X86
>>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>>> index ec50eb5..3d38fed 100644
>>> --- a/drivers/pwm/Makefile
>>> +++ b/drivers/pwm/Makefile
>>> @@ -35,3 +35,4 @@ obj-$(CONFIG_PWM_TIPWMSS)   += pwm-tipwmss.o
>>>  obj-$(CONFIG_PWM_TWL)                += pwm-twl.o
>>>  obj-$(CONFIG_PWM_TWL_LED)    += pwm-twl-led.o
>>>  obj-$(CONFIG_PWM_VT8500)     += pwm-vt8500.o
>>> +obj-$(CONFIG_PWM_CRC)                += pwm-crc.o
>>
>> This too.
>>
>>> diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c
>>> new file mode 100644
>>> index 0000000..987f3b4
>>> --- /dev/null
>>> +++ b/drivers/pwm/pwm-crc.c
>>> @@ -0,0 +1,171 @@
>>> +/*
>>> + * pwm-crc.c - Intel Crystal Cove PWM Driver
>>
>> I think you can safely remove this line. You already know what file it
>> is when you open it in your editor, and the description is in the
>> MODULE_DESCRIPTION string already.
>>
>>> + *
>>> + * Copyright (C) 2015 Intel Corporation. All rights reserved.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License version
>>> + * 2 as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * Author: Shobhit Kumar <shobhit.kumar@intel.com>
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/mfd/intel_soc_pmic.h>
>>> +#include <linux/pwm.h>
>>> +
>>> +#define PWM0_CLK_DIV         0x4B
>>> +#define  PWM_OUTPUT_ENABLE   (1<<7)
>>
>> Should have spaces around <<.
>>
>>> +#define  PWM_DIV_CLK_0               0x00 /* DIVIDECLK = BASECLK */
>>> +#define  PWM_DIV_CLK_100     0x63 /* DIVIDECLK = BASECLK/100 */
>>> +#define  PWM_DIV_CLK_128     0x7F /* DIVIDECLK = BASECLK/128 */
>>> +
>>> +#define PWM0_DUTY_CYCLE              0x4E
>>> +#define BACKLIGHT_EN         0x51
>>> +
>>> +#define PWM_MAX_LEVEL                0xFF
>>> +
>>> +#define PWM_BASE_CLK         6000    /* 6 MHz */
>>
>> This number is actually 6 KHz. I think it'd be better if you stuck with
>> one unit here. Or perhaps there's some other reason why you can't use
>> 6000000 here instead?
>>
>>> +#define PWM_MAX_PERIOD_NS    21333 /* 46.875KHz */
>>> +
>>> +/**
>>> + * struct crystalcove_pwm - Crystal Cove PWM controller
>>> + * @chip: the abstract pwm_chip structure.
>>> + * @regmap: the regmap from the parent device.
>>> + */
>>> +struct crystalcove_pwm {
>>> +     struct pwm_chip chip;
>>> +     struct platform_device *pdev;
>>
>> I think I had at some point requested that you get rid of this and use
>> the chip.dev member instead. There's no kerneldoc for it and it isn't
>> (well, almost, see below) used anywhere else, so perhaps you forgot to
>> remove it here?
>>
>>> +     struct regmap *regmap;
>>> +};
>>> +
>>> +static inline struct crystalcove_pwm *to_crc_pwm(struct pwm_chip *pc)
>>> +{
>>> +     return container_of(pc, struct crystalcove_pwm, chip);
>>> +}
>>> +
>>> +static int crc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm)
>>> +{
>>> +     struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
>>> +
>>> +     regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 1);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static void crc_pwm_disable(struct pwm_chip *c, struct pwm_device *pwm)
>>> +{
>>> +     struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
>>> +
>>> +     regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 0);
>>> +}
>>> +
>>> +static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm,
>>> +                               int duty_ns, int period_ns)
>>
>> Please align arguments on subsequent lines with the first argument of
>> the first line.
>>
>>> +{
>>> +     struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
>>> +     struct device *dev = &crc_pwm->pdev->dev;
>>
>> Did you test reconfiguring the PWM? I don't see crc_pwm->pdev getting
>> initialized anywhere, so this should crash trying to dereference a NULL
>> pointer.
>>
>> Of course if you get rid of the pdev field as I suggested you can simply
>> get the struct device * from c->dev.
>>
>>> +     int level, percent;
>>> +
>>> +     if (period_ns > PWM_MAX_PERIOD_NS) {
>>> +             dev_err(dev, "un-supported period_ns\n");
>>> +             return -1;
>>
>> You should return -EINVAL here. Besides being a literal and therefore a
>> bad idea, -1 == -EPERM and doesn't match the error condition.
>>
>>> +     }
>>> +
>>> +     if (pwm->period != period_ns) {
>>> +             int clk_div;
>>> +
>>> +             /* changing the clk divisor, need to disable fisrt */
>>> +             crc_pwm_disable(c, pwm);
>>> +             clk_div = PWM_BASE_CLK * period_ns / 1000000;
>>
>> Similar to the above, this is confusing because you're mixing up
>> different scales here. period_ns is in nanoseconds, so it'd be natural
>> to divide by 1000000000 (though you should really be using NSEC_PER_SEC
>> instead). If you counterweight that by expressing PWM_BASE_CLK in Hz
>> (6000000) you get much nicer symmetry and make the code a lot easier to
>> understand.
>>
>>> +
>>> +             regmap_write(crc_pwm->regmap, PWM0_CLK_DIV,
>>> +                                     clk_div | PWM_OUTPUT_ENABLE);
>>> +
>>> +             /* enable back */
>>> +             crc_pwm_enable(c, pwm);
>>> +     }
>>> +
>>> +     if (duty_ns > period_ns) {
>>> +             dev_err(dev, "duty cycle cannot be greater than cycle period\n");
>>> +             return -1;
>>> +     }
>>
>> The PWM core already performs this check, so you'll never get here in
>> case this condition is true.
>>
>>> +
>>> +     /* change the pwm duty cycle */
>>> +     percent = duty_ns * 100 / period_ns;
>>> +     level = percent * PWM_MAX_LEVEL / 100;
>>
>> Why do you need to apply the rule of three twice here? Doesn't
>>
>>         level = duty_ns * PWM_MAX_LEVEL / period_ns;
>>
>> give you what you want?
>>
>>> +     regmap_write(crc_pwm->regmap, PWM0_DUTY_CYCLE, level);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct pwm_ops crc_pwm_ops = {
>>> +     .config = crc_pwm_config,
>>> +     .enable = crc_pwm_enable,
>>> +     .disable = crc_pwm_disable,
>>> +     .owner = THIS_MODULE,
>>> +};
>>> +
>>> +static int crystalcove_pwm_probe(struct platform_device *pdev)
>>> +{
>>> +     struct crystalcove_pwm *pwm;
>>> +     int retval;
>>> +     struct device *dev = pdev->dev.parent;
>>> +     struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
>>> +
>>> +     pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
>>> +     if (!pwm)
>>> +             return -ENOMEM;
>>> +
>>> +     pwm->chip.dev = &pdev->dev;
>>> +     pwm->chip.ops = &crc_pwm_ops;
>>> +     pwm->chip.base = -1;
>>> +     pwm->chip.npwm = 1;
>>> +
>>> +     /* get the PMIC regmap */
>>> +     pwm->regmap = pmic->regmap;
>>> +
>>> +     retval = pwmchip_add(&pwm->chip);
>>> +     if (retval < 0)
>>> +             return retval;
>>> +
>>> +     dev_dbg(&pdev->dev, "crc-pwm probe successful\n");
>>
>> Do you really want this? The driver core will complain in any of the
>> above failures, so what use is there to be chatty when probing
>> succeeds?
>>
>>> +static struct platform_driver crystalcove_pwm_driver = {
>>> +     .probe = crystalcove_pwm_probe,
>>> +     .remove = crystalcove_pwm_remove,
>>> +     .driver = {
>>> +             .name = "crystal_cove_pwm",
>>
>> I'd prefer this to be "crystal-cove-pwm" for consistency with other
>> drivers, but since the MFD part already uses underscores in names it'd
>> introduce an inconsistency there. So I'm fine with this one as-is.
>>
>> Thierry
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
Shobhit Kumar June 17, 2015, 2:43 a.m. UTC | #6
On Wed, May 20, 2015 at 8:39 PM, Shobhit Kumar <kumar@shobhit.info> wrote:
> On Thu, May 7, 2015 at 12:49 PM, Shobhit Kumar <kumar@shobhit.info> wrote:
>> On Wed, May 6, 2015 at 5:44 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
>>> On Tue, May 05, 2015 at 03:08:36PM +0530, Shobhit Kumar wrote:
>>>> The Crystalcove PMIC controls PWM signals and this driver exports that
>>>
>>> You say signal_s_ here, but you only expose a single PWM device. Does
>>> the PMIC really control more than one? If it isn't, this should probably
>>> become: "controls a PWM output and this driver...".
>>
>> Actually it does support 3 of them but on the platform only one is
>> being used and I exported only that as of now. Probably I should
>> expand a little in the commit message indicating this. will re-post
>> after fixing based on your other comments.
>
> Updates pending due to personal leave. Can be expected next week.

Folks, really sorry, been busy with lot of unexpected and unavoidable
stuff. Working on getting the patches right. Expect them this week.

>>>
>>>> capability as a PWM chip driver. This is platform device implementtaion
>>>
>>> "implementation"
>>>
>>>> of the drivers/mfd cell device for CRC PMIC
>>>
>>> Sentences should end with a full stop.
>>>
>>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>>> index b1541f4..954da3e 100644
>>>> --- a/drivers/pwm/Kconfig
>>>> +++ b/drivers/pwm/Kconfig
>>>> @@ -183,6 +183,13 @@ config PWM_LPC32XX
>>>>         To compile this driver as a module, choose M here: the module
>>>>         will be called pwm-lpc32xx.
>>>>
>>>> +config PWM_CRC
>>>> +     bool "Intel Crystalcove (CRC) PWM support"
>>>> +     depends on X86 && INTEL_SOC_PMIC
>>>> +     help
>>>> +       Generic PWM framework driver for Crystalcove (CRC) PMIC based PWM
>>>> +       control.
>>>> +
>>>
>>> This is badly sorted. Please keep the list sorted alphabetically.
>>>
>>>>  config PWM_LPSS
>>>>       tristate "Intel LPSS PWM support"
>>>>       depends on X86
>>>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>>>> index ec50eb5..3d38fed 100644
>>>> --- a/drivers/pwm/Makefile
>>>> +++ b/drivers/pwm/Makefile
>>>> @@ -35,3 +35,4 @@ obj-$(CONFIG_PWM_TIPWMSS)   += pwm-tipwmss.o
>>>>  obj-$(CONFIG_PWM_TWL)                += pwm-twl.o
>>>>  obj-$(CONFIG_PWM_TWL_LED)    += pwm-twl-led.o
>>>>  obj-$(CONFIG_PWM_VT8500)     += pwm-vt8500.o
>>>> +obj-$(CONFIG_PWM_CRC)                += pwm-crc.o
>>>
>>> This too.
>>>
>>>> diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c
>>>> new file mode 100644
>>>> index 0000000..987f3b4
>>>> --- /dev/null
>>>> +++ b/drivers/pwm/pwm-crc.c
>>>> @@ -0,0 +1,171 @@
>>>> +/*
>>>> + * pwm-crc.c - Intel Crystal Cove PWM Driver
>>>
>>> I think you can safely remove this line. You already know what file it
>>> is when you open it in your editor, and the description is in the
>>> MODULE_DESCRIPTION string already.
>>>
>>>> + *
>>>> + * Copyright (C) 2015 Intel Corporation. All rights reserved.
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU General Public License version
>>>> + * 2 as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + *
>>>> + * Author: Shobhit Kumar <shobhit.kumar@intel.com>
>>>> + */
>>>> +
>>>> +#include <linux/module.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/regmap.h>
>>>> +#include <linux/mfd/intel_soc_pmic.h>
>>>> +#include <linux/pwm.h>
>>>> +
>>>> +#define PWM0_CLK_DIV         0x4B
>>>> +#define  PWM_OUTPUT_ENABLE   (1<<7)
>>>
>>> Should have spaces around <<.
>>>
>>>> +#define  PWM_DIV_CLK_0               0x00 /* DIVIDECLK = BASECLK */
>>>> +#define  PWM_DIV_CLK_100     0x63 /* DIVIDECLK = BASECLK/100 */
>>>> +#define  PWM_DIV_CLK_128     0x7F /* DIVIDECLK = BASECLK/128 */
>>>> +
>>>> +#define PWM0_DUTY_CYCLE              0x4E
>>>> +#define BACKLIGHT_EN         0x51
>>>> +
>>>> +#define PWM_MAX_LEVEL                0xFF
>>>> +
>>>> +#define PWM_BASE_CLK         6000    /* 6 MHz */
>>>
>>> This number is actually 6 KHz. I think it'd be better if you stuck with
>>> one unit here. Or perhaps there's some other reason why you can't use
>>> 6000000 here instead?
>>>
>>>> +#define PWM_MAX_PERIOD_NS    21333 /* 46.875KHz */
>>>> +
>>>> +/**
>>>> + * struct crystalcove_pwm - Crystal Cove PWM controller
>>>> + * @chip: the abstract pwm_chip structure.
>>>> + * @regmap: the regmap from the parent device.
>>>> + */
>>>> +struct crystalcove_pwm {
>>>> +     struct pwm_chip chip;
>>>> +     struct platform_device *pdev;
>>>
>>> I think I had at some point requested that you get rid of this and use
>>> the chip.dev member instead. There's no kerneldoc for it and it isn't
>>> (well, almost, see below) used anywhere else, so perhaps you forgot to
>>> remove it here?
>>>
>>>> +     struct regmap *regmap;
>>>> +};
>>>> +
>>>> +static inline struct crystalcove_pwm *to_crc_pwm(struct pwm_chip *pc)
>>>> +{
>>>> +     return container_of(pc, struct crystalcove_pwm, chip);
>>>> +}
>>>> +
>>>> +static int crc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm)
>>>> +{
>>>> +     struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
>>>> +
>>>> +     regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 1);
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static void crc_pwm_disable(struct pwm_chip *c, struct pwm_device *pwm)
>>>> +{
>>>> +     struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
>>>> +
>>>> +     regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 0);
>>>> +}
>>>> +
>>>> +static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm,
>>>> +                               int duty_ns, int period_ns)
>>>
>>> Please align arguments on subsequent lines with the first argument of
>>> the first line.
>>>
>>>> +{
>>>> +     struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
>>>> +     struct device *dev = &crc_pwm->pdev->dev;
>>>
>>> Did you test reconfiguring the PWM? I don't see crc_pwm->pdev getting
>>> initialized anywhere, so this should crash trying to dereference a NULL
>>> pointer.
>>>
>>> Of course if you get rid of the pdev field as I suggested you can simply
>>> get the struct device * from c->dev.
>>>
>>>> +     int level, percent;
>>>> +
>>>> +     if (period_ns > PWM_MAX_PERIOD_NS) {
>>>> +             dev_err(dev, "un-supported period_ns\n");
>>>> +             return -1;
>>>
>>> You should return -EINVAL here. Besides being a literal and therefore a
>>> bad idea, -1 == -EPERM and doesn't match the error condition.
>>>
>>>> +     }
>>>> +
>>>> +     if (pwm->period != period_ns) {
>>>> +             int clk_div;
>>>> +
>>>> +             /* changing the clk divisor, need to disable fisrt */
>>>> +             crc_pwm_disable(c, pwm);
>>>> +             clk_div = PWM_BASE_CLK * period_ns / 1000000;
>>>
>>> Similar to the above, this is confusing because you're mixing up
>>> different scales here. period_ns is in nanoseconds, so it'd be natural
>>> to divide by 1000000000 (though you should really be using NSEC_PER_SEC
>>> instead). If you counterweight that by expressing PWM_BASE_CLK in Hz
>>> (6000000) you get much nicer symmetry and make the code a lot easier to
>>> understand.
>>>
>>>> +
>>>> +             regmap_write(crc_pwm->regmap, PWM0_CLK_DIV,
>>>> +                                     clk_div | PWM_OUTPUT_ENABLE);
>>>> +
>>>> +             /* enable back */
>>>> +             crc_pwm_enable(c, pwm);
>>>> +     }
>>>> +
>>>> +     if (duty_ns > period_ns) {
>>>> +             dev_err(dev, "duty cycle cannot be greater than cycle period\n");
>>>> +             return -1;
>>>> +     }
>>>
>>> The PWM core already performs this check, so you'll never get here in
>>> case this condition is true.
>>>
>>>> +
>>>> +     /* change the pwm duty cycle */
>>>> +     percent = duty_ns * 100 / period_ns;
>>>> +     level = percent * PWM_MAX_LEVEL / 100;
>>>
>>> Why do you need to apply the rule of three twice here? Doesn't
>>>
>>>         level = duty_ns * PWM_MAX_LEVEL / period_ns;
>>>
>>> give you what you want?
>>>
>>>> +     regmap_write(crc_pwm->regmap, PWM0_DUTY_CYCLE, level);
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static const struct pwm_ops crc_pwm_ops = {
>>>> +     .config = crc_pwm_config,
>>>> +     .enable = crc_pwm_enable,
>>>> +     .disable = crc_pwm_disable,
>>>> +     .owner = THIS_MODULE,
>>>> +};
>>>> +
>>>> +static int crystalcove_pwm_probe(struct platform_device *pdev)
>>>> +{
>>>> +     struct crystalcove_pwm *pwm;
>>>> +     int retval;
>>>> +     struct device *dev = pdev->dev.parent;
>>>> +     struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
>>>> +
>>>> +     pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
>>>> +     if (!pwm)
>>>> +             return -ENOMEM;
>>>> +
>>>> +     pwm->chip.dev = &pdev->dev;
>>>> +     pwm->chip.ops = &crc_pwm_ops;
>>>> +     pwm->chip.base = -1;
>>>> +     pwm->chip.npwm = 1;
>>>> +
>>>> +     /* get the PMIC regmap */
>>>> +     pwm->regmap = pmic->regmap;
>>>> +
>>>> +     retval = pwmchip_add(&pwm->chip);
>>>> +     if (retval < 0)
>>>> +             return retval;
>>>> +
>>>> +     dev_dbg(&pdev->dev, "crc-pwm probe successful\n");
>>>
>>> Do you really want this? The driver core will complain in any of the
>>> above failures, so what use is there to be chatty when probing
>>> succeeds?
>>>
>>>> +static struct platform_driver crystalcove_pwm_driver = {
>>>> +     .probe = crystalcove_pwm_probe,
>>>> +     .remove = crystalcove_pwm_remove,
>>>> +     .driver = {
>>>> +             .name = "crystal_cove_pwm",
>>>
>>> I'd prefer this to be "crystal-cove-pwm" for consistency with other
>>> drivers, but since the MFD part already uses underscores in names it'd
>>> introduce an inconsistency there. So I'm fine with this one as-is.
>>>
>>> Thierry
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
diff mbox

Patch

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index b1541f4..954da3e 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -183,6 +183,13 @@  config PWM_LPC32XX
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-lpc32xx.
 
+config PWM_CRC
+	bool "Intel Crystalcove (CRC) PWM support"
+	depends on X86 && INTEL_SOC_PMIC
+	help
+	  Generic PWM framework driver for Crystalcove (CRC) PMIC based PWM
+	  control.
+
 config PWM_LPSS
 	tristate "Intel LPSS PWM support"
 	depends on X86
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index ec50eb5..3d38fed 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -35,3 +35,4 @@  obj-$(CONFIG_PWM_TIPWMSS)	+= pwm-tipwmss.o
 obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
 obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
 obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
+obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c
new file mode 100644
index 0000000..987f3b4
--- /dev/null
+++ b/drivers/pwm/pwm-crc.c
@@ -0,0 +1,171 @@ 
+/*
+ * pwm-crc.c - Intel Crystal Cove PWM Driver
+ *
+ * Copyright (C) 2015 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Author: Shobhit Kumar <shobhit.kumar@intel.com>
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/pwm.h>
+
+#define PWM0_CLK_DIV		0x4B
+#define  PWM_OUTPUT_ENABLE	(1<<7)
+#define  PWM_DIV_CLK_0		0x00 /* DIVIDECLK = BASECLK */
+#define  PWM_DIV_CLK_100	0x63 /* DIVIDECLK = BASECLK/100 */
+#define  PWM_DIV_CLK_128	0x7F /* DIVIDECLK = BASECLK/128 */
+
+#define PWM0_DUTY_CYCLE		0x4E
+#define BACKLIGHT_EN		0x51
+
+#define PWM_MAX_LEVEL		0xFF
+
+#define PWM_BASE_CLK		6000	/* 6 MHz */
+#define PWM_MAX_PERIOD_NS	21333 /* 46.875KHz */
+
+/**
+ * struct crystalcove_pwm - Crystal Cove PWM controller
+ * @chip: the abstract pwm_chip structure.
+ * @regmap: the regmap from the parent device.
+ */
+struct crystalcove_pwm {
+	struct pwm_chip chip;
+	struct platform_device *pdev;
+	struct regmap *regmap;
+};
+
+static inline struct crystalcove_pwm *to_crc_pwm(struct pwm_chip *pc)
+{
+	return container_of(pc, struct crystalcove_pwm, chip);
+}
+
+static int crc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm)
+{
+	struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
+
+	regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 1);
+
+	return 0;
+}
+
+static void crc_pwm_disable(struct pwm_chip *c, struct pwm_device *pwm)
+{
+	struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
+
+	regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 0);
+}
+
+static int crc_pwm_config(struct pwm_chip *c, struct pwm_device *pwm,
+				  int duty_ns, int period_ns)
+{
+	struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
+	struct device *dev = &crc_pwm->pdev->dev;
+	int level, percent;
+
+	if (period_ns > PWM_MAX_PERIOD_NS) {
+		dev_err(dev, "un-supported period_ns\n");
+		return -1;
+	}
+
+	if (pwm->period != period_ns) {
+		int clk_div;
+
+		/* changing the clk divisor, need to disable fisrt */
+		crc_pwm_disable(c, pwm);
+		clk_div = PWM_BASE_CLK * period_ns / 1000000;
+
+		regmap_write(crc_pwm->regmap, PWM0_CLK_DIV,
+					clk_div | PWM_OUTPUT_ENABLE);
+
+		/* enable back */
+		crc_pwm_enable(c, pwm);
+	}
+
+	if (duty_ns > period_ns) {
+		dev_err(dev, "duty cycle cannot be greater than cycle period\n");
+		return -1;
+	}
+
+	/* change the pwm duty cycle */
+	percent = duty_ns * 100 / period_ns;
+	level = percent * PWM_MAX_LEVEL / 100;
+	regmap_write(crc_pwm->regmap, PWM0_DUTY_CYCLE, level);
+
+	return 0;
+}
+
+static const struct pwm_ops crc_pwm_ops = {
+	.config = crc_pwm_config,
+	.enable = crc_pwm_enable,
+	.disable = crc_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static int crystalcove_pwm_probe(struct platform_device *pdev)
+{
+	struct crystalcove_pwm *pwm;
+	int retval;
+	struct device *dev = pdev->dev.parent;
+	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
+
+	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	pwm->chip.dev = &pdev->dev;
+	pwm->chip.ops = &crc_pwm_ops;
+	pwm->chip.base = -1;
+	pwm->chip.npwm = 1;
+
+	/* get the PMIC regmap */
+	pwm->regmap = pmic->regmap;
+
+	retval = pwmchip_add(&pwm->chip);
+	if (retval < 0)
+		return retval;
+
+	dev_dbg(&pdev->dev, "crc-pwm probe successful\n");
+	platform_set_drvdata(pdev, pwm);
+
+	return 0;
+}
+
+static int crystalcove_pwm_remove(struct platform_device *pdev)
+{
+	struct crystalcove_pwm *pwm = platform_get_drvdata(pdev);
+	int retval;
+
+	retval = pwmchip_remove(&pwm->chip);
+	if (retval < 0)
+		return retval;
+
+	dev_dbg(&pdev->dev, "crc-pwm driver removed\n");
+
+	return 0;
+}
+
+static struct platform_driver crystalcove_pwm_driver = {
+	.probe = crystalcove_pwm_probe,
+	.remove = crystalcove_pwm_remove,
+	.driver = {
+		.name = "crystal_cove_pwm",
+	},
+};
+
+module_platform_driver(crystalcove_pwm_driver);
+
+MODULE_AUTHOR("Shobhit Kumar <shobhit.kumar@intel.com>");
+MODULE_DESCRIPTION("Intel Crystal Cove PWM Driver");
+MODULE_LICENSE("GPL v2");