Re: [Intel-gfx] [PATCH 6/8] pwm: crc: Add Crystalcove (CRC) PWM driver

2015-06-16 Thread Shobhit Kumar
On Wed, May 20, 2015 at 8:39 PM, Shobhit Kumar ku...@shobhit.info wrote:
 On Thu, May 7, 2015 at 12:49 PM, Shobhit Kumar ku...@shobhit.info wrote:
 On Wed, May 6, 2015 at 5:44 PM, Thierry Reding thierry.red...@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 000..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.ku...@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   (17)

 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_LEVEL0xFF
 +
 +#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
 600 here instead?

 +#define PWM_MAX_PERIOD_NS21333 /* 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);
 +
 + 

Re: [Intel-gfx] [PATCH 6/8] pwm: crc: Add Crystalcove (CRC) PWM driver

2015-05-20 Thread Shobhit Kumar
On Thu, May 7, 2015 at 12:49 PM, Shobhit Kumar ku...@shobhit.info wrote:
 On Wed, May 6, 2015 at 5:44 PM, Thierry Reding thierry.red...@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 000..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.ku...@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   (17)

 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_LEVEL0xFF
 +
 +#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
 600 here instead?

 +#define PWM_MAX_PERIOD_NS21333 /* 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);
 +
 + 

Re: [Intel-gfx] [PATCH 6/8] pwm: crc: Add Crystalcove (CRC) PWM driver

2015-05-07 Thread Shobhit Kumar
On Wed, May 6, 2015 at 5:44 PM, Thierry Reding thierry.red...@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 000..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.ku...@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   (17)

 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_LEVEL0xFF
 +
 +#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
 600 here instead?

 +#define PWM_MAX_PERIOD_NS21333 /* 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)

Re: [Intel-gfx] [PATCH 6/8] pwm: crc: Add Crystalcove (CRC) PWM driver

2015-05-07 Thread Shobhit Kumar
On Wed, May 6, 2015 at 1:10 PM, Paul Bolle pebo...@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 sa...@linux.intel.com
 Cc: Linus Walleij linus.wall...@linaro.org
 Cc: Alexandre Courbot gnu...@gmail.com
 Cc: Thierry Reding thierry.red...@gmail.com
 Signed-off-by: Shobhit Kumar shobhit.ku...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 6/8] pwm: crc: Add Crystalcove (CRC) PWM driver

2015-05-06 Thread Paul Bolle
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 sa...@linux.intel.com
 Cc: Linus Walleij linus.wall...@linaro.org
 Cc: Alexandre Courbot gnu...@gmail.com
 Cc: Thierry Reding thierry.red...@gmail.com
 Signed-off-by: Shobhit Kumar shobhit.ku...@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

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 6/8] pwm: crc: Add Crystalcove (CRC) PWM driver

2015-05-06 Thread Thierry Reding
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 000..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.ku...@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   (17)

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_LEVEL0xFF
 +
 +#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
600 here instead?

 +#define PWM_MAX_PERIOD_NS21333 /* 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