This is not how we submit subsequent patch-sets.

Please submit them as a whole, seperately from the first submission and with versioning information i.e. [PATCH v2 X/Y] Stuff ...

In case we unload and load a driver module again that is registering a lookup table, without this it will result in multiple entries. Provide an option to remove the lookup table on driver unload

v2: Ccing maintainers v3: Correct the subject line (Lee jones)

Change logs should go underneth the '---' and above the diffstat found below.

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 Reviewed-by: Alexandre Courbot acourbot@nvidia.com Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com

---

drivers/gpio/gpiolib.c | 13 +++++++++++++ include/linux/gpio/machine.h | 1 + 2 files changed, 14 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 59eaa23..2420af9 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1658,6 +1658,19 @@ void gpiod_add_lookup_table(struct gpiod_lookup_table *table) mutex_unlock(&gpio_lookup_lock); }

+/**

• • gpiod_remove_lookup_table() - unregister GPIO device consumers
• • @table: table of consumers to unregister
• */

+void gpiod_remove_lookup_table(struct gpiod_lookup_table *table) +{

• mutex_lock(&gpio_lookup_lock);
• list_del(&table->list);
• mutex_unlock(&gpio_lookup_lock);

+}

static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id, unsigned int idx, enum gpio_lookup_flags *flags) diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h index e270614..c0d712d 100644 --- a/include/linux/gpio/machine.h +++ b/include/linux/gpio/machine.h @@ -57,5 +57,6 @@ struct gpiod_lookup_table { }

void gpiod_add_lookup_table(struct gpiod_lookup_table *table); +void gpiod_remove_lookup_table(struct gpiod_lookup_table *table);

#endif /* __LINUX_GPIO_MACHINE_H */

On Tue, 05 May 2015, Daniel Vetter wrote:

On Tue, May 05, 2015 at 11:45:05AM +0100, Lee Jones wrote:

...

This is not how we submit subsequent patch-sets.

It is unfortunately how we handle patches on dri-devel&intel-gfx to be able to cope with massive mail load. If everyone who submits to intel-gfx would always resend the entire series for minor updates of som patches we'd completely drown in the resulting flood.

For one or two simple fix-ups in the set perhaps, but when submitting the entire set it needs to be threaded as a separate block, rather than seeing current and superseded patches inter-woven. This submission is already a rat's nest and I'm struggling to see which patches are which. I'm really not looking forward to v3 and v4! Attaching one version to another is a good way to keep control if you really are over-whelmed. For your use-case I would expect to see the following, which is achieved using --in-reply-to:

[PATCH 0/2] Here is what I did... [PATCH 1/2] Clean up and tests [PATCH 2/2] Implementation [PATCH v2 0/3] Here is a reroll [PATCH v2 1/3] Clean up [PATCH v2 2/3] New tests [PATCH v2 3/3] Implementation

The version numbers also need to be present and aren't in this re-submission.

...

Please submit them as a whole, seperately from the first submission and with versioning information i.e. [PATCH v2 X/Y] Stuff ...

...

In case we unload and load a driver module again that is registering a lookup table, without this it will result in multiple entries. Provide an option to remove the lookup table on driver unload

v2: Ccing maintainers v3: Correct the subject line (Lee jones)

Change logs should go underneth the '---' and above the diffstat found below.

Again just style differences between subsystems, I generally want to have those above the ---.

For all commits? Then I'm guessing your Git history is all but unreadable. In the kernel, unless the changelog holds valuable historic information which influance key design decisions, we put the patch changelog *below* the '---'.

Please read Documentation/SubmittingPatches:

"14) The canonical patch format

[...]

The "---" marker line serves the essential purpose of marking for patch handling tools where the changelog message ends.

[...]

Other comments relevant only to the moment or the maintainer, not suitable for the permanent changelog, should also go here. A good example of such comments might be *"patch changelogs"* which describe what has changed between the v1 and v2 version of the patch."

...

...

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 Reviewed-by: Alexandre Courbot acourbot@nvidia.com Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com

---

drivers/gpio/gpiolib.c | 13 +++++++++++++ include/linux/gpio/machine.h | 1 + 2 files changed, 14 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 59eaa23..2420af9 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1658,6 +1658,19 @@ void gpiod_add_lookup_table(struct gpiod_lookup_table *table) mutex_unlock(&gpio_lookup_lock); }

+/**

• • gpiod_remove_lookup_table() - unregister GPIO device consumers
• • @table: table of consumers to unregister
• */

+void gpiod_remove_lookup_table(struct gpiod_lookup_table *table) +{

• mutex_lock(&gpio_lookup_lock);
• list_del(&table->list);
• mutex_unlock(&gpio_lookup_lock);

+}

static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id, unsigned int idx, enum gpio_lookup_flags *flags) diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h index e270614..c0d712d 100644 --- a/include/linux/gpio/machine.h +++ b/include/linux/gpio/machine.h @@ -57,5 +57,6 @@ struct gpiod_lookup_table { }

void gpiod_add_lookup_table(struct gpiod_lookup_table *table); +void gpiod_remove_lookup_table(struct gpiod_lookup_table *table);

#endif /* __LINUX_GPIO_MACHINE_H */

On Wed, 29 Apr 2015, Shobhit Kumar wrote:

On some Intel SoC platforms, the panel enable/disable signals are controlled by CRC PMIC. Add those control as a new GPIO in a lookup table for gpio-crystalcove chip during CRC driver load

v2: Make the lookup table static (Thierry) Remove the lookup table during driver remove (Thierry)

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/mfd/intel_soc_pmic_core.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)

I have no idea what this stuff is, but it looks plausible.

For my own reference: Acked-by: Lee Jones lee.jones@linaro.org

diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c index 7b50b6b..f3d918e 100644 --- a/drivers/mfd/intel_soc_pmic_core.c +++ b/drivers/mfd/intel_soc_pmic_core.c @@ -24,8 +24,19 @@ #include <linux/acpi.h> #include <linux/regmap.h> #include <linux/mfd/intel_soc_pmic.h> +#include <linux/gpio/machine.h> #include "intel_soc_pmic_core.h"

+/* Lookup table for the Panel Enable/Disable line as GPIO signals */ +static struct gpiod_lookup_table panel_gpio_table = {

• /* Intel GFX is consumer */
• .dev_id = "0000:00:02.0",
• .table = {

• /* Panel EN/DISABLE */
  

• GPIO_LOOKUP("gpio_crystalcove", 94, "panel", GPIO_ACTIVE_HIGH),
  

• },

+};

static int intel_soc_pmic_find_gpio_irq(struct device *dev) { struct gpio_desc *desc; @@ -85,6 +96,9 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c, if (ret) dev_warn(dev, "Can't enable IRQ as wake source: %d\n", ret);

• /* Add lookup table binding for Panel Control to the GPIO Chip */
• gpiod_add_lookup_table(&panel_gpio_table);
• ret = mfd_add_devices(dev, -1, config->cell_dev, config->n_cell_devs, NULL, 0, regmap_irq_get_domain(pmic->irq_chip_data));

@@ -104,6 +118,9 @@ static int intel_soc_pmic_i2c_remove(struct i2c_client *i2c)

regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);

• /* Remove lookup table for Panel Control from the GPIO Chip */

• gpiod_remove_lookup_table(&panel_gpio_table);

• mfd_remove_devices(&i2c->dev);

  return 0;

By the way, your subject lines are messed up.

They should adhere to the conventions laid down by the Maintainers.

`git log --oneline -- drivers/<subsystem>`

On some Intel SoC platforms, the panel enable/disable signals are controlled by CRC PMIC. Add those control as a new GPIO in a lookup table for gpio-crystalcove chip during CRC driver load

v2: Make the lookup table static (Thierry) Remove the lookup table during driver remove (Thierry)

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/mfd/intel_soc_pmic_core.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)

diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c index 7b50b6b..f3d918e 100644 --- a/drivers/mfd/intel_soc_pmic_core.c +++ b/drivers/mfd/intel_soc_pmic_core.c @@ -24,8 +24,19 @@ #include <linux/acpi.h> #include <linux/regmap.h> #include <linux/mfd/intel_soc_pmic.h> +#include <linux/gpio/machine.h> #include "intel_soc_pmic_core.h"

+/* Lookup table for the Panel Enable/Disable line as GPIO signals */ +static struct gpiod_lookup_table panel_gpio_table = {

• /* Intel GFX is consumer */
• .dev_id = "0000:00:02.0",
• .table = {

• /* Panel EN/DISABLE */
  

• GPIO_LOOKUP("gpio_crystalcove", 94, "panel", GPIO_ACTIVE_HIGH),
  

• },

+};

static int intel_soc_pmic_find_gpio_irq(struct device *dev) { struct gpio_desc *desc; @@ -85,6 +96,9 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c, if (ret) dev_warn(dev, "Can't enable IRQ as wake source: %d\n", ret);

• /* Add lookup table binding for Panel Control to the GPIO Chip */
• gpiod_add_lookup_table(&panel_gpio_table);
• ret = mfd_add_devices(dev, -1, config->cell_dev, config->n_cell_devs, NULL, 0, regmap_irq_get_domain(pmic->irq_chip_data));

@@ -104,6 +118,9 @@ static int intel_soc_pmic_i2c_remove(struct i2c_client *i2c)

regmap_del_irq_chip(pmic->irq, pmic->irq_chip_data);

• /* Remove lookup table for Panel Control from the GPIO Chip */

• gpiod_remove_lookup_table(&panel_gpio_table);

• mfd_remove_devices(&i2c->dev);

  return 0;

Needed for PWM control suuported by the PMIC

v2: 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 Acked-by: Lee Jones lee.jones@linaro.org Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com --- drivers/mfd/intel_soc_pmic_crc.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c index 4cc1b32..8839e25 100644 --- a/drivers/mfd/intel_soc_pmic_crc.c +++ b/drivers/mfd/intel_soc_pmic_crc.c @@ -109,6 +109,9 @@ static struct mfd_cell crystal_cove_dev[] = { { .name = "crystal_cove_pmic", }, + { + .name = "crystal_cove_pwm", + }, };

static const struct regmap_config crystal_cove_regmap_config = {

On Wed, 29 Apr 2015, Shobhit Kumar wrote:

On some BYT PLatform the PWM is controlled using CRC PMIC. Add a lookup entry for the same to be used by the consumer (Intel GFX)

v2: Remove the lookup table on driver unload (Thierry)

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/mfd/intel_soc_pmic_core.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)

How do you expect this set to be managed?

Acked-by: Lee Jones lee.jones@linaro.org

diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c index f3d918e..a00ddd9 100644 --- a/drivers/mfd/intel_soc_pmic_core.c +++ b/drivers/mfd/intel_soc_pmic_core.c @@ -25,6 +25,7 @@ #include <linux/regmap.h> #include <linux/mfd/intel_soc_pmic.h> #include <linux/gpio/machine.h> +#include <linux/pwm.h> #include "intel_soc_pmic_core.h"

/* Lookup table for the Panel Enable/Disable line as GPIO signals */ @@ -37,6 +38,11 @@ static struct gpiod_lookup_table panel_gpio_table = { }, };

+/* PWM consumed by the Intel GFX */ +static struct pwm_lookup crc_pwm_lookup[] = {

• PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_backlight", 0, PWM_POLARITY_NORMAL),

+};

static int intel_soc_pmic_find_gpio_irq(struct device *dev) { struct gpio_desc *desc; @@ -99,6 +105,9 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c, /* Add lookup table binding for Panel Control to the GPIO Chip */ gpiod_add_lookup_table(&panel_gpio_table);

• /* Add lookup table for crc-pwm */
• pwm_add_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
• ret = mfd_add_devices(dev, -1, config->cell_dev, config->n_cell_devs, NULL, 0, regmap_irq_get_domain(pmic->irq_chip_data));

@@ -121,6 +130,9 @@ static int intel_soc_pmic_i2c_remove(struct i2c_client *i2c) /* Remove lookup table for Panel Control from the GPIO Chip */ gpiod_remove_lookup_table(&panel_gpio_table);

• /* remove crc-pwm lookup table */

• pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));

• mfd_remove_devices(&i2c->dev);

  return 0;

On Tue, 05 May 2015, Shobhit Kumar wrote:

On 04/29/2015 07:54 PM, Lee Jones wrote:

...

On Wed, 29 Apr 2015, Shobhit Kumar wrote:

...

On some BYT PLatform the PWM is controlled using CRC PMIC. Add a lookup entry for the same to be used by the consumer (Intel GFX)

v2: Remove the lookup table on driver unload (Thierry)

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/mfd/intel_soc_pmic_core.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)

How do you expect this set to be managed?

There are some dependencies on the look up table remove functionality in earlier patches, so I think 3/8 can go in only after 1/8. Similarly 5/8 can go only after 2/8. Rest all can be independent.

Taking patches 'in order' is tough to coordinate and takes a very long time. The best thing to do is have all of the patches taken in via a single tree at the same time.

...

Acked-by: Lee Jones lee.jones@linaro.org

...

diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c index f3d918e..a00ddd9 100644 --- a/drivers/mfd/intel_soc_pmic_core.c +++ b/drivers/mfd/intel_soc_pmic_core.c @@ -25,6 +25,7 @@ #include <linux/regmap.h> #include <linux/mfd/intel_soc_pmic.h> #include <linux/gpio/machine.h> +#include <linux/pwm.h> #include "intel_soc_pmic_core.h"

/* Lookup table for the Panel Enable/Disable line as GPIO signals */ @@ -37,6 +38,11 @@ static struct gpiod_lookup_table panel_gpio_table = { }, };

+/* PWM consumed by the Intel GFX */ +static struct pwm_lookup crc_pwm_lookup[] = {

• PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_backlight", 0, PWM_POLARITY_NORMAL),

+};

static int intel_soc_pmic_find_gpio_irq(struct device *dev) { struct gpio_desc *desc; @@ -99,6 +105,9 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c, /* Add lookup table binding for Panel Control to the GPIO Chip */ gpiod_add_lookup_table(&panel_gpio_table);

• /* Add lookup table for crc-pwm */
• pwm_add_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
• ret = mfd_add_devices(dev, -1, config->cell_dev, config->n_cell_devs, NULL, 0, regmap_irq_get_domain(pmic->irq_chip_data));

@@ -121,6 +130,9 @@ static int intel_soc_pmic_i2c_remove(struct i2c_client *i2c) /* Remove lookup table for Panel Control from the GPIO Chip */ gpiod_remove_lookup_table(&panel_gpio_table);

• /* remove crc-pwm lookup table */

• pwm_remove_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));

• mfd_remove_devices(&i2c->dev);

  return 0;

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

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");

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

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

On Wed, 29 Apr 2015, Shobhit Kumar shobhit.kumar@intel.com wrote:

The CRC (Crystal Cove) PMIC, controls the panel enable and disable signals for BYT for dsi panels. This is indicated in the VBT fields. Use that to initialize and use GPIO based control for these signals.

v2: Use the newer gpiod interface(Alexandre) v3: Remove the redundant checks and unused code (Ville)

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/gpu/drm/i915/intel_dsi.c | 32 ++++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_dsi.h | 6 ++++++ 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 5196642..be55ffa 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -31,6 +31,7 @@ #include <drm/drm_panel.h> #include <drm/drm_mipi_dsi.h> #include <linux/slab.h> +#include <linux/gpio/consumer.h> #include "i915_drv.h" #include "intel_drv.h" #include "intel_dsi.h" @@ -415,6 +416,12 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)

DRM_DEBUG_KMS("\n");

• /* Panel Enable over CRC PMIC */
• if (intel_dsi->gpio_panel)

• gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
  

• msleep(intel_dsi->panel_on_delay);
• /* Disable DPOunit clock gating, can stall pipe
  • and we need DPLL REFA always enabled */
  tmp = I915_READ(DPLL(pipe));

@@ -432,8 +439,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder) /* put device in ready state */ intel_dsi_device_ready(encoder);

• msleep(intel_dsi->panel_on_delay);

• drm_panel_prepare(intel_dsi->panel);

  for_each_dsi_port(port, intel_dsi->ports)

@@ -576,6 +581,10 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder)

msleep(intel_dsi->panel_off_delay); msleep(intel_dsi->panel_pwr_cycle_delay);

• /* Panel Disable over CRC PMIC */
• if (intel_dsi->gpio_panel)

• gpiod_set_value_cansleep(intel_dsi->gpio_panel, 0);
  

}

static bool intel_dsi_get_hw_state(struct intel_encoder *encoder, @@ -955,6 +964,11 @@ static void intel_dsi_encoder_destroy(struct drm_encoder *encoder) /* XXX: Logically this call belongs in the panel driver. */ drm_panel_remove(intel_dsi->panel); }

• /* dispose of the gpios */
• if (intel_dsi->gpio_panel)

• gpiod_put(intel_dsi->gpio_panel);
  

• intel_encoder_destroy(encoder);

}

@@ -1071,6 +1085,20 @@ void intel_dsi_init(struct drm_device *dev) goto err; }

• /*

• * In case of BYT with CRC PMIC, we need to use GPIO for
  

• * Panel control.
  

• */
  

• if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) {

• intel_dsi->gpio_panel =
  

• 	gpiod_get(dev->dev, "panel", GPIOD_OUT_HIGH);
  

• if (IS_ERR(intel_dsi->gpio_panel)) {
  

• 	DRM_ERROR("Failed to own gpio for panel control\n");
  

• 	intel_dsi->gpio_panel = NULL;
  

• }
  

• }
• intel_encoder->type = INTEL_OUTPUT_DSI; intel_encoder->cloneable = 0; drm_connector_init(dev, connector, &intel_dsi_connector_funcs,

diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h index 2784ac4..bf1bade 100644 --- a/drivers/gpu/drm/i915/intel_dsi.h +++ b/drivers/gpu/drm/i915/intel_dsi.h @@ -29,6 +29,9 @@ #include <drm/drm_mipi_dsi.h> #include "intel_drv.h"

+#define PPS_BLC_PMIC 0 +#define PPS_BLC_SOC 1

Since these values are defined in the VBT, perhaps these should be in intel_bios.h. Up to you, I'm fine either way.

Reviewed-by: Jani Nikula jani.nikula@intel.com

/* Dual Link support */ #define DSI_DUAL_LINK_NONE 0 #define DSI_DUAL_LINK_FRONT_BACK 1 @@ -42,6 +45,9 @@ struct intel_dsi { struct drm_panel *panel; struct intel_dsi_host *dsi_hosts[I915_MAX_PORTS];

• /* GPIO Desc for CRC based Panel control */

• struct gpio_desc *gpio_panel;

• struct intel_connector *attached_connector;

  /* bit mask of ports being driven */

-- 2.1.0

On Wed, 29 Apr 2015, Shobhit Kumar shobhit.kumar@intel.com wrote:

Use the CRC PWM device in intel_panel.c and add new MIPI backlight specififc callbacks

v2: Modify to use pwm_config callback

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/gpu/drm/i915/intel_drv.h | 5 +++ drivers/gpu/drm/i915/intel_dsi.c | 6 +++ drivers/gpu/drm/i915/intel_panel.c | 92 +++++++++++++++++++++++++++++++++++--- 3 files changed, 98 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 897f17d..b4ebe3b 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -182,7 +182,12 @@ struct intel_panel { bool enabled; bool combination_mode; /* gen 2/4 only */ bool active_low_pwm;

• /* PWM chip */
  

• struct pwm_device *pwm;
  

• struct backlight_device *device;

Superfluous whitespace.

} backlight;

void (*backlight_power)(struct intel_connector *, bool enable); diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index be55ffa..83c4540 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -402,6 +402,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder)

intel_dsi_port_enable(encoder);

}

• intel_panel_enable_backlight(intel_dsi->attached_connector);

}

static void intel_dsi_pre_enable(struct intel_encoder *encoder) @@ -466,6 +468,8 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder)

DRM_DEBUG_KMS("\n");

• intel_panel_disable_backlight(intel_dsi->attached_connector);
• if (is_vid_mode(intel_dsi)) { /* Send Shutdown command to the panel in LP mode */ for_each_dsi_port(port, intel_dsi->ports)

@@ -1132,6 +1136,8 @@ void intel_dsi_init(struct drm_device *dev) }

intel_panel_init(&intel_connector->panel, fixed_mode, NULL);

• intel_panel_setup_backlight(connector,

• (intel_encoder->crtc_mask = (1 << PIPE_A)) ? PIPE_A: PIPE_B);
  
  

  return;

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 08532d4..5700f6f 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -32,8 +32,12 @@

#include <linux/kernel.h> #include <linux/moduleparam.h> +#include <linux/pwm.h> #include "intel_drv.h"

+#define CRC_PMIC_PWM_PERIOD_NS 21333 +#define CRC_PMIC_PWM_STEPS 255

void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode) @@ -536,6 +540,15 @@ static u32 vlv_get_backlight(struct intel_connector *connector) return _vlv_get_backlight(dev, pipe); }

+static u32 vlv_get_mipi_backlight(struct intel_connector *connector)

I'd rather call this either pmic_get_backlight or even pwm_get_backlight because there's nothing really vlv specific about the functions themselves. Same for all of them.

+{

• struct intel_panel *panel = &connector->panel;
• int duty_ns;
• duty_ns = pwm_get_duty_cycle(panel->backlight.pwm);
• return DIV_ROUND_UP(duty_ns * 100, CRC_PMIC_PWM_PERIOD_NS);

Basically we have all the code in place to do the scaling from hw to user. See the note in intel_backlight_device_register. We shouldn't need to add additional scaling all around.

However this can be a future cleanup IMO.

+}

static u32 intel_panel_get_backlight(struct intel_connector *connector) { struct drm_device *dev = connector->base.dev; @@ -616,6 +629,14 @@ static void vlv_set_backlight(struct intel_connector *connector, u32 level) I915_WRITE(VLV_BLC_PWM_CTL(pipe), tmp | level); }

+static void vlv_set_mipi_backlight(struct intel_connector *connector, u32 level) +{

• struct intel_panel *panel = &connector->panel;
• int duty_ns = DIV_ROUND_UP(level * CRC_PMIC_PWM_PERIOD_NS, 100);
• pwm_config(panel->backlight.pwm, duty_ns, CRC_PMIC_PWM_PERIOD_NS);

+}

static void intel_panel_actually_set_backlight(struct intel_connector *connector, u32 level) { @@ -741,6 +762,16 @@ static void vlv_disable_backlight(struct intel_connector *connector) I915_WRITE(VLV_BLC_PWM_CTL2(pipe), tmp & ~BLM_PWM_ENABLE); }

+static void vlv_disable_mipi_backlight(struct intel_connector *connector) +{

• struct intel_panel *panel = &connector->panel;
• /* Disable the backlight */
• pwm_config(panel->backlight.pwm, 0, CRC_PMIC_PWM_PERIOD_NS);
• usleep_range(2000, 3000);
• pwm_disable(panel->backlight.pwm);

+}

void intel_panel_disable_backlight(struct intel_connector *connector) { struct drm_device *dev = connector->base.dev; @@ -947,6 +978,16 @@ static void vlv_enable_backlight(struct intel_connector *connector) I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2 | BLM_PWM_ENABLE); }

+static void vlv_enable_mipi_backlight(struct intel_connector *connector) +{

• struct intel_panel *panel = &connector->panel;
• int duty_ns = DIV_ROUND_UP(

• 	panel->backlight.level * CRC_PMIC_PWM_PERIOD_NS, 100);
  

• pwm_enable(panel->backlight.pwm);
• pwm_config(panel->backlight.pwm, duty_ns, CRC_PMIC_PWM_PERIOD_NS);

All other enable functions call

intel_panel_actually_set_backlight(connector, panel->backlight.level);

to do this. It'll give you logging too.

+}

void intel_panel_enable_backlight(struct intel_connector *connector) { struct drm_device *dev = connector->base.dev; @@ -1299,6 +1340,34 @@ static int vlv_setup_backlight(struct intel_connector *connector, enum pipe pipe return 0; }

+static int vlv_setup_mipi_backlight(struct intel_connector *connector, enum pipe pipe) +{

• struct drm_device *dev = connector->base.dev;
• struct intel_panel *panel = &connector->panel;
• /* Get the PWM chip for backlight control */
• panel->backlight.pwm = pwm_get(dev->dev, "pwm_backlight");
• if (IS_ERR(panel->backlight.pwm)) {

• DRM_ERROR("Faild to own the pwm chip\n");
  

• panel->backlight.pwm = NULL;
  

I think you want to return -ENODEV here. And then you can drop the "else" below and make that a normal if.

• } else if (pwm_config(panel->backlight.pwm, CRC_PMIC_PWM_PERIOD_NS,

• 				CRC_PMIC_PWM_PERIOD_NS) < 0) {
  

• DRM_ERROR("Failed to configure the pwm chip\n");
  

• pwm_put(panel->backlight.pwm);
  

• panel->backlight.pwm = NULL;
  

• return -1;
  

Save whatever pwm_config returns and return that instead of -1.

• }
• panel->backlight.min = 0; /* 0% */
• panel->backlight.max = 100; /* 100% */
• panel->backlight.level = DIV_ROUND_UP(

• 		 pwm_get_duty_cycle(panel->backlight.pwm) * 100,
  

• 		 CRC_PMIC_PWM_PERIOD_NS);
  

• panel->backlight.enabled = panel->backlight.level ? true : false;

panel->backlight.level != 0

• return 0;

+}

int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe) { struct drm_device *dev = connector->dev; @@ -1363,11 +1432,19 @@ void intel_panel_init_backlight_funcs(struct drm_device *dev) dev_priv->display.set_backlight = pch_set_backlight; dev_priv->display.get_backlight = pch_get_backlight; } else if (IS_VALLEYVIEW(dev)) {

• dev_priv->display.setup_backlight = vlv_setup_backlight;
  

• dev_priv->display.enable_backlight = vlv_enable_backlight;
  

• dev_priv->display.disable_backlight = vlv_disable_backlight;
  

• dev_priv->display.set_backlight = vlv_set_backlight;
  

• dev_priv->display.get_backlight = vlv_get_backlight;
  

• if (dev_priv->vbt.has_mipi) {
  

• 	dev_priv->display.setup_backlight = vlv_setup_mipi_backlight;
  

• 	dev_priv->display.enable_backlight = vlv_enable_mipi_backlight;
  

• 	dev_priv->display.disable_backlight = vlv_disable_mipi_backlight;
  

• 	dev_priv->display.set_backlight = vlv_set_mipi_backlight;
  

• 	dev_priv->display.get_backlight = vlv_get_mipi_backlight;
  

• } else {
  

• 	dev_priv->display.setup_backlight = vlv_setup_backlight;
  

• 	dev_priv->display.enable_backlight = vlv_enable_backlight;
  

• 	dev_priv->display.disable_backlight = vlv_disable_backlight;
  

• 	dev_priv->display.set_backlight = vlv_set_backlight;
  

• 	dev_priv->display.get_backlight = vlv_get_backlight;
  

• }
  

  } else if (IS_GEN4(dev)) { dev_priv->display.setup_backlight = i965_setup_backlight; dev_priv->display.enable_backlight = i965_enable_backlight;

@@ -1404,6 +1481,11 @@ void intel_panel_fini(struct intel_panel *panel) if (panel->downclock_mode) drm_mode_destroy(intel_connector->base.dev, panel->downclock_mode);

• /* dispose of the pwm */
• if (panel->backlight.pwm)

• pwm_put(panel->backlight.pwm);
  

The cleanup should be in intel_panel_destroy_backlight to match where .pwm is initialized.

BR, Jani.

}

void intel_backlight_register(struct drm_device *dev)

2.1.0