Hi All, On some of the BYT devices, for DSI panels, the panel enable/disable signals and backlight control are done using the Crystalcove PMIC. This series provides support for the same and has been reviewed earlier on - https://lkml.org/lkml/2015/4/29/301
This series addresses the review comments with one patch from last set already merged as - https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=...
Basically following are implemented -
1. GPIO control for panel enable/disable with GFX device as consumer 2. New PWM chip driver added for CRC PMIC based backlight control 3. i915 is modified to use the CRC gpio chip and the CRC PWM chip to do backlight control. This is now added in the generic panel backlight control infrastructure
All these patches have been tested on AsusT100 and working fine using /sys/class/backlight/intel_backlight interface.
Patches were also verified on android-x86 tree for AsusT100.
Regards Shobhit
Shobhit Kumar (7): gpiolib: Add support for removing registered consumer lookup table mfd: intel_soc_pmic_core: Add lookup table for Panel Control as GPIO signal mfd: intel_soc_pmic_crc: Add PWM cell device for Crystalcove PMIC mfd: intel_soc_pmic_core: ADD PWM lookup table for CRC PMIC based PWM pwm: crc: Add Crystalcove (CRC) PWM driver drm/i915: Use the CRC gpio for panel enable/disable drm/i915: Backlight control using CRC PMIC based PWM driver
drivers/gpio/gpiolib.c | 13 ++++ drivers/gpu/drm/i915/intel_bios.h | 7 ++ drivers/gpu/drm/i915/intel_drv.h | 4 + drivers/gpu/drm/i915/intel_dsi.c | 38 ++++++++- drivers/gpu/drm/i915/intel_dsi.h | 3 + drivers/gpu/drm/i915/intel_panel.c | 95 +++++++++++++++++++++-- drivers/mfd/intel_soc_pmic_core.c | 29 +++++++ drivers/mfd/intel_soc_pmic_crc.c | 3 + drivers/pwm/Kconfig | 7 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-crc.c | 155 +++++++++++++++++++++++++++++++++++++ include/linux/gpio/machine.h | 1 + 12 files changed, 349 insertions(+), 7 deletions(-) create mode 100644 drivers/pwm/pwm-crc.c
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)
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 Reviewed-by: Linus Walleij linus.walleij@linaro.org 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 957ede5..9d3ea4e 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1675,6 +1675,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 Mon, Jun 22, 2015 at 12:54 PM, Shobhit Kumar shobhit.kumar@intel.com wrote:
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)
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 Reviewed-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com
Do you plan to take this upstream for the next merge window or should I just merge this patch?
Yours, Linus Walleij
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)
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 Acked-by: Lee Jones lee.jones@linaro.org Acked-by: Linus Walleij linus.walleij@linaro.org 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 7436075..4a74948 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 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)
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 Acked-by: Lee Jones lee.jones@linaro.org Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com --- drivers/mfd/intel_soc_pmic_core.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
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;
Hi Shobhit Kumar,
On 06/22/2015 04:24 PM, 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)
v3: Correct the subject line (Lee jones)
This part should only describe what this is about..
Don't put this patch change history over here. Include this change history after ... Signed-off-by: Author <email> ---
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
Here you add this change history so that after applying this will not be the part of your commit description.
This comment is applicable for all of your patches.
On Mon, Jun 22, 2015 at 04:33:22PM +0530, Varka Bhadram wrote:
Hi Shobhit Kumar,
On 06/22/2015 04:24 PM, 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)
v3: Correct the subject line (Lee jones)
This part should only describe what this is about..
Don't put this patch change history over here. Include this change history after ... Signed-off-by: Author <email>
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
Here you add this change history so that after applying this will not be the part of your commit description.
This comment is applicable for all of your patches.
It's honestly a per-maintainer thing and hard to tell who wants what ... Personally I do want to include the patch changelog in the commit message. -Daniel
-- Best regards, Varka Bhadram.
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 22 Jun 2015, Daniel Vetter wrote:
On Mon, Jun 22, 2015 at 04:33:22PM +0530, Varka Bhadram wrote:
Hi Shobhit Kumar,
On 06/22/2015 04:24 PM, 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)
v3: Correct the subject line (Lee jones)
This part should only describe what this is about..
Don't put this patch change history over here. Include this change history after ... Signed-off-by: Author <email>
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
Here you add this change history so that after applying this will not be the part of your commit description.
This comment is applicable for all of your patches.
It's honestly a per-maintainer thing and hard to tell who wants what ... Personally I do want to include the patch changelog in the commit message.
The patch change-log should go below the '---'. There are very few (weird ;) ) Maintainers who like to see them in the commit log.
On Tue, Jun 23, 2015 at 12:49 PM, Lee Jones lee.jones@linaro.org wrote:
On Mon, 22 Jun 2015, Daniel Vetter wrote:
On Mon, Jun 22, 2015 at 04:33:22PM +0530, Varka Bhadram wrote:
Hi Shobhit Kumar,
On 06/22/2015 04:24 PM, 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)
v3: Correct the subject line (Lee jones)
This part should only describe what this is about..
Don't put this patch change history over here. Include this change history after ... Signed-off-by: Author <email>
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
Here you add this change history so that after applying this will not be the part of your commit description.
This comment is applicable for all of your patches.
It's honestly a per-maintainer thing and hard to tell who wants what ... Personally I do want to include the patch changelog in the commit message.
The patch change-log should go below the '---'. There are very few (weird ;) ) Maintainers who like to see them in the commit log.
To satisfy everybody, for the last two patches for intel-gfx, I will keep the version history as is, for others, I will move it down as suggested. Will push all again as new series after addressing all comments.
Regards Shobhit
-- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
The Crystalcove PMIC provides three PWM signals and this driver exports one of them on the BYT platform which is used to control backlight for DSI panel. This is platform device implementation 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)
v4: Address comment by Thierry & Paul - Commit message update and fixes for few syntax errors - Add PWM_CRC in Kconfig and Makefile sorted alphabetically - Use the PWM_BASE_CLK as 6000000 for better code readability - Remove the redundant rule of three while calculating pwm level - Use the platform_device in pwm_chip - Use builin_platform_driver
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 Cc: Paul Bolle pebolle@tiscali.nl Cc: Paul Gortmaker paul.gortmaker@windriver.com Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com --- drivers/pwm/Kconfig | 7 +++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-crc.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 163 insertions(+) create mode 100644 drivers/pwm/pwm-crc.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index b1541f4..948d9ab 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -111,6 +111,13 @@ config PWM_CLPS711X To compile this driver as a module, choose M here: the module will be called pwm-clps711x.
+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_EP93XX tristate "Cirrus Logic EP93xx PWM support" depends on ARCH_EP93XX diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index ec50eb5..d186f35 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_PWM_BCM_KONA) += pwm-bcm-kona.o obj-$(CONFIG_PWM_BCM2835) += pwm-bcm2835.o obj-$(CONFIG_PWM_BFIN) += pwm-bfin.o obj-$(CONFIG_PWM_CLPS711X) += pwm-clps711x.o +obj-$(CONFIG_PWM_CRC) += pwm-crc.o obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o obj-$(CONFIG_PWM_IMG) += pwm-img.o diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c new file mode 100644 index 0000000..dcd9782 --- /dev/null +++ b/drivers/pwm/pwm-crc.c @@ -0,0 +1,155 @@ +/* + * 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/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 6000000 /* 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 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->chip.dev; + int level; + + if (period_ns > PWM_MAX_PERIOD_NS) { + dev_err(dev, "un-supported period_ns\n"); + return -EINVAL; + } + + 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 / NSEC_PER_SEC; + + regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, + clk_div | PWM_OUTPUT_ENABLE); + + /* enable back */ + crc_pwm_enable(c, pwm); + } + + /* change the pwm duty cycle */ + level = duty_ns * PWM_MAX_LEVEL / period_ns; + 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, +}; + +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; + + 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", + }, +}; + +builtin_platform_driver(crystalcove_pwm_driver);
Hi Shobhit Kumar,
On 06/22/2015 04:24 PM, Shobhit Kumar wrote:
The Crystalcove PMIC provides three PWM signals and this driver exports one of them on the BYT platform which is used to control backlight for DSI panel. This is platform device implementation 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)
v4: Address comment by Thierry & Paul - Commit message update and fixes for few syntax errors - Add PWM_CRC in Kconfig and Makefile sorted alphabetically - Use the PWM_BASE_CLK as 6000000 for better code readability - Remove the redundant rule of three while calculating pwm level - Use the platform_device in pwm_chip - Use builin_platform_driver
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 Cc: Paul Bolle pebolle@tiscali.nl Cc: Paul Gortmaker paul.gortmaker@windriver.com Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com
(...)
+#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)
Can't be BIT() macro ?
+#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
(...)
+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;- platform_set_drvdata(pdev, pwm);
If you can change this oder we can simply do something like this: platform_set_drvdata(pdev, pwm); return pwmchip_add(&pwm->chip);
- 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");
This debug message may not be required :-)
you can directly do:
return pwmchip_remove(&pwm->chip);
- return 0;
+}
+static struct platform_driver crystalcove_pwm_driver = {
- .probe = crystalcove_pwm_probe,
- .remove = crystalcove_pwm_remove,
- .driver = {
.name = "crystal_cove_pwm",- },
+};
+builtin_platform_driver(crystalcove_pwm_driver);
On Mon, Jun 22, 2015 at 4:46 PM, Varka Bhadram varkabhadram@gmail.com wrote:
Hi Shobhit Kumar,
On 06/22/2015 04:24 PM, Shobhit Kumar wrote:
The Crystalcove PMIC provides three PWM signals and this driver exports one of them on the BYT platform which is used to control backlight for DSI panel. This is platform device implementation 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)
v4: Address comment by Thierry & Paul - Commit message update and fixes for few syntax errors - Add PWM_CRC in Kconfig and Makefile sorted alphabetically - Use the PWM_BASE_CLK as 6000000 for better code readability - Remove the redundant rule of three while calculating pwm level - Use the platform_device in pwm_chip - Use builin_platform_driver
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 Cc: Paul Bolle pebolle@tiscali.nl Cc: Paul Gortmaker paul.gortmaker@windriver.com Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com
(...)
+#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)
Can't be BIT() macro ?
Can be done.
+#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
(...)
+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;platform_set_drvdata(pdev, pwm);If you can change this oder we can simply do something like this:
platform_set_drvdata(pdev, pwm); return pwmchip_add(&pwm->chip);
Okay. seems better.
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");This debug message may not be required :-)
you can directly do:
return pwmchip_remove(&pwm->chip);
Yeah, will update.
Regards Shobhit
return 0;+}
+static struct platform_driver crystalcove_pwm_driver = {
.probe = crystalcove_pwm_probe,.remove = crystalcove_pwm_remove,.driver = {.name = "crystal_cove_pwm",},+};
+builtin_platform_driver(crystalcove_pwm_driver);
-- Best regards, Varka Bhadram.
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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) v4: Moved PWM vs SoC backlight #defines to intel_bios.h (Jani)
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: Linus Walleij linus.walleij@linaro.org Reviewed-by: Jani Nikula jani.nikula@intel.com Signed-off-by: Shobhit Kumar shobhit.kumar@intel.com --- drivers/gpu/drm/i915/intel_bios.h | 7 +++++++ drivers/gpu/drm/i915/intel_dsi.c | 32 ++++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_dsi.h | 3 +++ 3 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h index af0b476..f7ad6a5 100644 --- a/drivers/gpu/drm/i915/intel_bios.h +++ b/drivers/gpu/drm/i915/intel_bios.h @@ -778,6 +778,13 @@ int intel_parse_bios(struct drm_device *dev); #define MIPI_DSI_UNDEFINED_PANEL_ID 0 #define MIPI_DSI_GENERIC_PANEL_ID 1
+/* + * PMIC vs SoC Backlight support specified in pwm_blc + * field in mipi_config block below. +*/ +#define PPS_BLC_PMIC 0 +#define PPS_BLC_SOC 1 + struct mipi_config { u16 panel_id;
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index b5a5558..c4db74a 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..42a6859 100644 --- a/drivers/gpu/drm/i915/intel_dsi.h +++ b/drivers/gpu/drm/i915/intel_dsi.h @@ -42,6 +42,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 */
Use the CRC PWM device in intel_panel.c and add new MIPI backlight specififc callbacks
v2: Modify to use pwm_config callback v3: Addressed Jani's comments - Renamed all function as pwm_* instead of vlv_* - Call intel_panel_actually_set_backlight in enable function - Return -ENODEV in case pwm_get fails - in case pwm_config error return error cdoe from pwm_config - Cleanup pwm in intel_panel_destroy_backlight
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 | 4 ++ drivers/gpu/drm/i915/intel_dsi.c | 6 +++ drivers/gpu/drm/i915/intel_panel.c | 95 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 100 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 2afb31a..561c17f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -182,6 +182,10 @@ 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; } backlight;
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index c4db74a..be8722c 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 7d83527..2aa30db 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) @@ -544,6 +548,15 @@ static u32 bxt_get_backlight(struct intel_connector *connector) return I915_READ(BXT_BLC_PWM_DUTY1); }
+static u32 pwm_get_backlight(struct intel_connector *connector) +{ + 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); +} + static u32 intel_panel_get_backlight(struct intel_connector *connector) { struct drm_device *dev = connector->base.dev; @@ -632,6 +645,14 @@ static void bxt_set_backlight(struct intel_connector *connector, u32 level) I915_WRITE(BXT_BLC_PWM_DUTY1, level); }
+static void pwm_set_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) { @@ -769,6 +790,16 @@ static void bxt_disable_backlight(struct intel_connector *connector) I915_WRITE(BXT_BLC_PWM_CTL1, tmp & ~BXT_BLC_PWM_ENABLE); }
+static void pwm_disable_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; @@ -1002,6 +1033,14 @@ static void bxt_enable_backlight(struct intel_connector *connector) I915_WRITE(BXT_BLC_PWM_CTL1, pwm_ctl | BXT_BLC_PWM_ENABLE); }
+static void pwm_enable_backlight(struct intel_connector *connector) +{ + struct intel_panel *panel = &connector->panel; + + pwm_enable(panel->backlight.pwm); + intel_panel_actually_set_backlight(connector, panel->backlight.level); +} + void intel_panel_enable_backlight(struct intel_connector *connector) { struct drm_device *dev = connector->base.dev; @@ -1378,6 +1417,40 @@ bxt_setup_backlight(struct intel_connector *connector, enum pipe unused) return 0; }
+static int pwm_setup_backlight(struct intel_connector *connector, + enum pipe pipe) +{ + struct drm_device *dev = connector->base.dev; + struct intel_panel *panel = &connector->panel; + int retval = -1; + + /* Get the PWM chip for backlight control */ + panel->backlight.pwm = pwm_get(dev->dev, "pwm_backlight"); + if (IS_ERR(panel->backlight.pwm)) { + DRM_ERROR("Failed to own the pwm chip\n"); + panel->backlight.pwm = NULL; + return -ENODEV; + } + + retval = pwm_config(panel->backlight.pwm, CRC_PMIC_PWM_PERIOD_NS, + CRC_PMIC_PWM_PERIOD_NS); + if (retval < 0) { + DRM_ERROR("Failed to configure the pwm chip\n"); + pwm_put(panel->backlight.pwm); + panel->backlight.pwm = NULL; + return retval; + } + + 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 != 0; + + return 0; +} + int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe) { struct drm_device *dev = connector->dev; @@ -1421,6 +1494,10 @@ void intel_panel_destroy_backlight(struct drm_connector *connector) struct intel_connector *intel_connector = to_intel_connector(connector); struct intel_panel *panel = &intel_connector->panel;
+ /* dispose of the pwm */ + if (panel->backlight.pwm) + pwm_put(panel->backlight.pwm); + panel->backlight.present = false; }
@@ -1448,11 +1525,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 = pwm_setup_backlight; + dev_priv->display.enable_backlight = pwm_enable_backlight; + dev_priv->display.disable_backlight = pwm_disable_backlight; + dev_priv->display.set_backlight = pwm_set_backlight; + dev_priv->display.get_backlight = pwm_get_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;
On Mon, Jun 22, 2015 at 04:24:25PM +0530, Shobhit Kumar wrote:
Use the CRC PWM device in intel_panel.c and add new MIPI backlight specififc callbacks
v2: Modify to use pwm_config callback v3: Addressed Jani's comments - Renamed all function as pwm_* instead of vlv_* - Call intel_panel_actually_set_backlight in enable function - Return -ENODEV in case pwm_get fails - in case pwm_config error return error cdoe from pwm_config - Cleanup pwm in intel_panel_destroy_backlight
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 | 4 ++ drivers/gpu/drm/i915/intel_dsi.c | 6 +++ drivers/gpu/drm/i915/intel_panel.c | 95 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 100 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 2afb31a..561c17f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -182,6 +182,10 @@ 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; } backlight;
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index c4db74a..be8722c 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);
^
Whoops. But since the PWM backlight doesn't need the initial pipe for anything you can actually just pass INVALID_PIPE here.
return;
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 7d83527..2aa30db 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
This define appears to be unused.
void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode) @@ -544,6 +548,15 @@ static u32 bxt_get_backlight(struct intel_connector *connector) return I915_READ(BXT_BLC_PWM_DUTY1); }
+static u32 pwm_get_backlight(struct intel_connector *connector) +{
- 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);
+}
static u32 intel_panel_get_backlight(struct intel_connector *connector) { struct drm_device *dev = connector->base.dev; @@ -632,6 +645,14 @@ static void bxt_set_backlight(struct intel_connector *connector, u32 level) I915_WRITE(BXT_BLC_PWM_DUTY1, level); }
+static void pwm_set_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) { @@ -769,6 +790,16 @@ static void bxt_disable_backlight(struct intel_connector *connector) I915_WRITE(BXT_BLC_PWM_CTL1, tmp & ~BXT_BLC_PWM_ENABLE); }
+static void pwm_disable_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; @@ -1002,6 +1033,14 @@ static void bxt_enable_backlight(struct intel_connector *connector) I915_WRITE(BXT_BLC_PWM_CTL1, pwm_ctl | BXT_BLC_PWM_ENABLE); }
+static void pwm_enable_backlight(struct intel_connector *connector) +{
- struct intel_panel *panel = &connector->panel;
- pwm_enable(panel->backlight.pwm);
- intel_panel_actually_set_backlight(connector, panel->backlight.level);
+}
void intel_panel_enable_backlight(struct intel_connector *connector) { struct drm_device *dev = connector->base.dev; @@ -1378,6 +1417,40 @@ bxt_setup_backlight(struct intel_connector *connector, enum pipe unused) return 0; }
+static int pwm_setup_backlight(struct intel_connector *connector,
enum pipe pipe)+{
- struct drm_device *dev = connector->base.dev;
- struct intel_panel *panel = &connector->panel;
- int retval = -1;
No need to initialize it.
- /* Get the PWM chip for backlight control */
- panel->backlight.pwm = pwm_get(dev->dev, "pwm_backlight");
- if (IS_ERR(panel->backlight.pwm)) {
DRM_ERROR("Failed to own the pwm chip\n");panel->backlight.pwm = NULL;return -ENODEV;- }
- retval = pwm_config(panel->backlight.pwm, CRC_PMIC_PWM_PERIOD_NS,
CRC_PMIC_PWM_PERIOD_NS);- if (retval < 0) {
DRM_ERROR("Failed to configure the pwm chip\n");pwm_put(panel->backlight.pwm);panel->backlight.pwm = NULL;return retval;- }
- 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 != 0;
- return 0;
+}
int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe) { struct drm_device *dev = connector->dev; @@ -1421,6 +1494,10 @@ void intel_panel_destroy_backlight(struct drm_connector *connector) struct intel_connector *intel_connector = to_intel_connector(connector); struct intel_panel *panel = &intel_connector->panel;
- /* dispose of the pwm */
- if (panel->backlight.pwm)
pwm_put(panel->backlight.pwm);- panel->backlight.present = false;
}
@@ -1448,11 +1525,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) {
Do all VLV DSI desins use the PMIC for backlight, or is there something more specific in VBT we could look at?
And what about CHT?
Othwerwise things seem reasonable, so with the the intel_panel_setup_backlight() pipe thing fixed this patch is Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
I also gave the entire series a go on my FFRD8 and it appears to work just fine, so you can also add Tested-by: Ville Syrjälä ville.syrjala@linux.intel.com to all the patches if you want.
dev_priv->display.setup_backlight = pwm_setup_backlight;dev_priv->display.enable_backlight = pwm_enable_backlight;dev_priv->display.disable_backlight = pwm_disable_backlight;dev_priv->display.set_backlight = pwm_set_backlight;dev_priv->display.get_backlight = pwm_get_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;}-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jun 25, 2015 at 2:18 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Mon, Jun 22, 2015 at 04:24:25PM +0530, Shobhit Kumar wrote:
Use the CRC PWM device in intel_panel.c and add new MIPI backlight specififc callbacks
v2: Modify to use pwm_config callback v3: Addressed Jani's comments - Renamed all function as pwm_* instead of vlv_* - Call intel_panel_actually_set_backlight in enable function - Return -ENODEV in case pwm_get fails - in case pwm_config error return error cdoe from pwm_config - Cleanup pwm in intel_panel_destroy_backlight
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 | 4 ++ drivers/gpu/drm/i915/intel_dsi.c | 6 +++ drivers/gpu/drm/i915/intel_panel.c | 95 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 100 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 2afb31a..561c17f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -182,6 +182,10 @@ 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; } backlight;diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index c4db74a..be8722c 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);^Whoops. But since the PWM backlight doesn't need the initial pipe for anything you can actually just pass INVALID_PIPE here.
You are right, its unused, but I thought passing right value still made sense. Otherwise it makes it look like I am setting up back-light for invalid pipe, when the real meaning is something like DONTCARE_PIPE
return;diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 7d83527..2aa30db 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
This define appears to be unused.
Yeah, missed removing it.
void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode) @@ -544,6 +548,15 @@ static u32 bxt_get_backlight(struct intel_connector *connector) return I915_READ(BXT_BLC_PWM_DUTY1); }
+static u32 pwm_get_backlight(struct intel_connector *connector) +{
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);+}
static u32 intel_panel_get_backlight(struct intel_connector *connector) { struct drm_device *dev = connector->base.dev; @@ -632,6 +645,14 @@ static void bxt_set_backlight(struct intel_connector *connector, u32 level) I915_WRITE(BXT_BLC_PWM_DUTY1, level); }
+static void pwm_set_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) { @@ -769,6 +790,16 @@ static void bxt_disable_backlight(struct intel_connector *connector) I915_WRITE(BXT_BLC_PWM_CTL1, tmp & ~BXT_BLC_PWM_ENABLE); }
+static void pwm_disable_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; @@ -1002,6 +1033,14 @@ static void bxt_enable_backlight(struct intel_connector *connector) I915_WRITE(BXT_BLC_PWM_CTL1, pwm_ctl | BXT_BLC_PWM_ENABLE); }
+static void pwm_enable_backlight(struct intel_connector *connector) +{
struct intel_panel *panel = &connector->panel;pwm_enable(panel->backlight.pwm);intel_panel_actually_set_backlight(connector, panel->backlight.level);+}
void intel_panel_enable_backlight(struct intel_connector *connector) { struct drm_device *dev = connector->base.dev; @@ -1378,6 +1417,40 @@ bxt_setup_backlight(struct intel_connector *connector, enum pipe unused) return 0; }
+static int pwm_setup_backlight(struct intel_connector *connector,
enum pipe pipe)+{
struct drm_device *dev = connector->base.dev;struct intel_panel *panel = &connector->panel;int retval = -1;No need to initialize it.
/* Get the PWM chip for backlight control */panel->backlight.pwm = pwm_get(dev->dev, "pwm_backlight");if (IS_ERR(panel->backlight.pwm)) {DRM_ERROR("Failed to own the pwm chip\n");panel->backlight.pwm = NULL;return -ENODEV;}retval = pwm_config(panel->backlight.pwm, CRC_PMIC_PWM_PERIOD_NS,CRC_PMIC_PWM_PERIOD_NS);if (retval < 0) {DRM_ERROR("Failed to configure the pwm chip\n");pwm_put(panel->backlight.pwm);panel->backlight.pwm = NULL;return retval;}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 != 0;return 0;+}
int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe) { struct drm_device *dev = connector->dev; @@ -1421,6 +1494,10 @@ void intel_panel_destroy_backlight(struct drm_connector *connector) struct intel_connector *intel_connector = to_intel_connector(connector); struct intel_panel *panel = &intel_connector->panel;
/* dispose of the pwm */if (panel->backlight.pwm)pwm_put(panel->backlight.pwm);panel->backlight.present = false;}
@@ -1448,11 +1525,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) {Do all VLV DSI desins use the PMIC for backlight, or is there something more specific in VBT we could look at?
No, VLV designs can actually also use LPSS PWM as well as DISPLAY_PWM. Infact we had a case where even for eDP, customer design used LPSS. So this flag in VBT for mipi_config block "pwm_blc" indicate the same for now. But today this is only PMIC Vs LPSS. There is another update pending where we have full flexibility in VBT to define PMIC or LPSS or DISPLAY_PWM to handle the eDP case that I mentioned above.
And what about CHT?
Its the same as I described above. But then beyond CHT, there is effort to unify this all as only one SoC PWM.
Othwerwise things seem reasonable, so with the the intel_panel_setup_backlight() pipe thing fixed this patch is Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
I also gave the entire series a go on my FFRD8 and it appears to work just fine, so you can also add Tested-by: Ville Syrjälä ville.syrjala@linux.intel.com to all the patches if you want.
Thanks a lot for testing it out.
Regards Shobhit
dev_priv->display.setup_backlight = pwm_setup_backlight;dev_priv->display.enable_backlight = pwm_enable_backlight;dev_priv->display.disable_backlight = pwm_disable_backlight;dev_priv->display.set_backlight = pwm_set_backlight;dev_priv->display.get_backlight = pwm_get_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;-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jun 25, 2015 at 05:38:50PM +0530, Shobhit Kumar wrote:
On Thu, Jun 25, 2015 at 2:18 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Mon, Jun 22, 2015 at 04:24:25PM +0530, Shobhit Kumar wrote:
Use the CRC PWM device in intel_panel.c and add new MIPI backlight specififc callbacks
v2: Modify to use pwm_config callback v3: Addressed Jani's comments - Renamed all function as pwm_* instead of vlv_* - Call intel_panel_actually_set_backlight in enable function - Return -ENODEV in case pwm_get fails - in case pwm_config error return error cdoe from pwm_config - Cleanup pwm in intel_panel_destroy_backlight
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 | 4 ++ drivers/gpu/drm/i915/intel_dsi.c | 6 +++ drivers/gpu/drm/i915/intel_panel.c | 95 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 100 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 2afb31a..561c17f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -182,6 +182,10 @@ 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; } backlight;diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index c4db74a..be8722c 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);^Whoops. But since the PWM backlight doesn't need the initial pipe for anything you can actually just pass INVALID_PIPE here.
You are right, its unused, but I thought passing right value still made sense. Otherwise it makes it look like I am setting up back-light for invalid pipe, when the real meaning is something like DONTCARE_PIPE
Well it's not really about the pipe. It's about which set of BLC registers we're supoosed to use when using the BLC built into the display engine. And that's only done so that we take over the hardware state correctly. So INVALID_PIPE is just fine in this case since the backlight control has nothing to do with the pipe.
On Thu, Jun 25, 2015 at 6:17 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Thu, Jun 25, 2015 at 05:38:50PM +0530, Shobhit Kumar wrote:
On Thu, Jun 25, 2015 at 2:18 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Mon, Jun 22, 2015 at 04:24:25PM +0530, Shobhit Kumar wrote:
Use the CRC PWM device in intel_panel.c and add new MIPI backlight specififc callbacks
v2: Modify to use pwm_config callback v3: Addressed Jani's comments - Renamed all function as pwm_* instead of vlv_* - Call intel_panel_actually_set_backlight in enable function - Return -ENODEV in case pwm_get fails - in case pwm_config error return error cdoe from pwm_config - Cleanup pwm in intel_panel_destroy_backlight
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 | 4 ++ drivers/gpu/drm/i915/intel_dsi.c | 6 +++ drivers/gpu/drm/i915/intel_panel.c | 95 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 100 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 2afb31a..561c17f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -182,6 +182,10 @@ 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; } backlight;diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index c4db74a..be8722c 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);^Whoops. But since the PWM backlight doesn't need the initial pipe for anything you can actually just pass INVALID_PIPE here.
You are right, its unused, but I thought passing right value still made sense. Otherwise it makes it look like I am setting up back-light for invalid pipe, when the real meaning is something like DONTCARE_PIPE
Well it's not really about the pipe. It's about which set of BLC registers we're supoosed to use when using the BLC built into the display engine. And that's only done so that we take over the hardware state correctly. So INVALID_PIPE is just fine in this case since the backlight control has nothing to do with the pipe.
Ok, will update.
Regards Shobhit
On Thu, 25 Jun 2015, Shobhit Kumar kumar@shobhit.info wrote:
On Thu, Jun 25, 2015 at 2:18 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Mon, Jun 22, 2015 at 04:24:25PM +0530, Shobhit Kumar wrote:
Use the CRC PWM device in intel_panel.c and add new MIPI backlight specififc callbacks
v2: Modify to use pwm_config callback v3: Addressed Jani's comments - Renamed all function as pwm_* instead of vlv_* - Call intel_panel_actually_set_backlight in enable function - Return -ENODEV in case pwm_get fails - in case pwm_config error return error cdoe from pwm_config - Cleanup pwm in intel_panel_destroy_backlight
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 | 4 ++ drivers/gpu/drm/i915/intel_dsi.c | 6 +++ drivers/gpu/drm/i915/intel_panel.c | 95 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 100 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 2afb31a..561c17f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -182,6 +182,10 @@ 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; } backlight;diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index c4db74a..be8722c 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);^Whoops. But since the PWM backlight doesn't need the initial pipe for anything you can actually just pass INVALID_PIPE here.
You are right, its unused, but I thought passing right value still made sense. Otherwise it makes it look like I am setting up back-light for invalid pipe, when the real meaning is something like DONTCARE_PIPE
return;diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 7d83527..2aa30db 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
This define appears to be unused.
Yeah, missed removing it.
void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode) @@ -544,6 +548,15 @@ static u32 bxt_get_backlight(struct intel_connector *connector) return I915_READ(BXT_BLC_PWM_DUTY1); }
+static u32 pwm_get_backlight(struct intel_connector *connector) +{
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);+}
static u32 intel_panel_get_backlight(struct intel_connector *connector) { struct drm_device *dev = connector->base.dev; @@ -632,6 +645,14 @@ static void bxt_set_backlight(struct intel_connector *connector, u32 level) I915_WRITE(BXT_BLC_PWM_DUTY1, level); }
+static void pwm_set_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) { @@ -769,6 +790,16 @@ static void bxt_disable_backlight(struct intel_connector *connector) I915_WRITE(BXT_BLC_PWM_CTL1, tmp & ~BXT_BLC_PWM_ENABLE); }
+static void pwm_disable_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; @@ -1002,6 +1033,14 @@ static void bxt_enable_backlight(struct intel_connector *connector) I915_WRITE(BXT_BLC_PWM_CTL1, pwm_ctl | BXT_BLC_PWM_ENABLE); }
+static void pwm_enable_backlight(struct intel_connector *connector) +{
struct intel_panel *panel = &connector->panel;pwm_enable(panel->backlight.pwm);intel_panel_actually_set_backlight(connector, panel->backlight.level);+}
void intel_panel_enable_backlight(struct intel_connector *connector) { struct drm_device *dev = connector->base.dev; @@ -1378,6 +1417,40 @@ bxt_setup_backlight(struct intel_connector *connector, enum pipe unused) return 0; }
+static int pwm_setup_backlight(struct intel_connector *connector,
enum pipe pipe)+{
struct drm_device *dev = connector->base.dev;struct intel_panel *panel = &connector->panel;int retval = -1;No need to initialize it.
/* Get the PWM chip for backlight control */panel->backlight.pwm = pwm_get(dev->dev, "pwm_backlight");if (IS_ERR(panel->backlight.pwm)) {DRM_ERROR("Failed to own the pwm chip\n");panel->backlight.pwm = NULL;return -ENODEV;}retval = pwm_config(panel->backlight.pwm, CRC_PMIC_PWM_PERIOD_NS,CRC_PMIC_PWM_PERIOD_NS);if (retval < 0) {DRM_ERROR("Failed to configure the pwm chip\n");pwm_put(panel->backlight.pwm);panel->backlight.pwm = NULL;return retval;}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 != 0;return 0;+}
int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe) { struct drm_device *dev = connector->dev; @@ -1421,6 +1494,10 @@ void intel_panel_destroy_backlight(struct drm_connector *connector) struct intel_connector *intel_connector = to_intel_connector(connector); struct intel_panel *panel = &intel_connector->panel;
/* dispose of the pwm */if (panel->backlight.pwm)pwm_put(panel->backlight.pwm);panel->backlight.present = false;}
@@ -1448,11 +1525,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) {Do all VLV DSI desins use the PMIC for backlight, or is there something more specific in VBT we could look at?
No, VLV designs can actually also use LPSS PWM as well as DISPLAY_PWM. Infact we had a case where even for eDP, customer design used LPSS. So this flag in VBT for mipi_config block "pwm_blc" indicate the same for now. But today this is only PMIC Vs LPSS. There is another update pending where we have full flexibility in VBT to define PMIC or LPSS or DISPLAY_PWM to handle the eDP case that I mentioned above.
Side note, we may need to move the hooks from dev_priv->display to connector->panel in the future.
BR, Jani.
And what about CHT?
Its the same as I described above. But then beyond CHT, there is effort to unify this all as only one SoC PWM.
Othwerwise things seem reasonable, so with the the intel_panel_setup_backlight() pipe thing fixed this patch is Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
I also gave the entire series a go on my FFRD8 and it appears to work just fine, so you can also add Tested-by: Ville Syrjälä ville.syrjala@linux.intel.com to all the patches if you want.
Thanks a lot for testing it out.
Regards Shobhit
dev_priv->display.setup_backlight = pwm_setup_backlight;dev_priv->display.enable_backlight = pwm_enable_backlight;dev_priv->display.disable_backlight = pwm_disable_backlight;dev_priv->display.set_backlight = pwm_set_backlight;dev_priv->display.get_backlight = pwm_get_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;-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jani Nikula -
Lee Jones -
Linus Walleij -
Shobhit Kumar -
Shobhit Kumar -
Varka Bhadram -
Ville Syrjälä