[RFC v5 0/9] Crystalcove (CRC) PMIC based panel and pwm control

[RFC v5 0/9] Crystalcove (CRC) PMIC based panel and pwm control

[Shobhit Kumar]

Hi All,
These patches try to enable panel control as a new GPIO via lookup table
during pmic core driver loading. Code is modifed to use generic gpiod_*
routines which will take care of ownership issues. 

Also a new PWM cell device is added to the PMIC MFD. New PWM driver pwm-crc
is used to do pwm control. Since the pwm is to be configured using clock
divisor and duty percent instead of period_ns and duty_ns new helpers have been
attempted in pwm core framework. Again pwm is added to a lookup table during
PMIC core driver load time.

Finally all is tied in the display driver enabling DSI panel. As of now the pwm
use is not hooked in to the bigger backlight class driver based infra in i915
but will be done as next step.

Sending all patches together to give a complete view of changes and how they gel
together. Please provide your comments.

Regards
Shobhit

Shobhit Kumar (9):
  drivers/mfd: Add lookup table for Panel Control as GPIO signal
  gpio/crystalcove: Add additional GPIO for Panel control
  drm/i915: Use the CRC gpio for panel enable/disable
  drivers/pwm: Add helper to configure pwm using clock divisor and duty
    percent
  drivers/mfd: Add PWM cell device for Crystalcove PMIC
  drivers/pwm: Add Crystalcove (CRC) PWM driver
  drivers/pwm: Remove __init initializer for pwm_add_table
  drivers/mfd: ADD PWM lookup table for CRC PMIC based PWM
  drm/i915: Backlight control using CRC PMIC based PWM driver

 drivers/gpio/gpio-crystalcove.c   |   7 +-
 drivers/gpu/drm/i915/intel_dsi.c  |  59 +++++++++++++-
 drivers/gpu/drm/i915/intel_dsi.h  |  13 +++
 drivers/mfd/intel_soc_pmic_core.c |  23 ++++++
 drivers/mfd/intel_soc_pmic_crc.c  |   3 +
 drivers/pwm/Kconfig               |   7 ++
 drivers/pwm/Makefile              |   1 +
 drivers/pwm/core.c                |  26 +++++-
 drivers/pwm/pwm-crc.c             | 161 ++++++++++++++++++++++++++++++++++++++
 include/linux/pwm.h               |  33 ++++++++
 10 files changed, 329 insertions(+), 4 deletions(-)
 create mode 100644 drivers/pwm/pwm-crc.c

-- 
2.1.0

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

[RFC v5 1/9] drivers/mfd: Add lookup table for Panel Control as GPIO signal

[Shobhit Kumar]

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

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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c
index 80cef04..365d5de 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 */
+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),
+	},
+};
+
 /*
  * On some boards the PMIC interrupt may come from a GPIO line.
  * Try to lookup the ACPI table and see if such connection exists. If not,
@@ -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));
-- 
2.1.0

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

[RFC v5 2/9] gpio/crystalcove: Add additional GPIO for Panel control

[Shobhit Kumar]

Export PANEL_EN/DISABLE (offset 0x52) as additional GPIO. Needed
by display driver to enable the DSI panel on BYT platform where
the Panel EN/Disable control is routed thorugh CRC PMIC

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/gpio/gpio-crystalcove.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio-crystalcove.c
index 3d9e08f..91a7ffe 100644
--- a/drivers/gpio/gpio-crystalcove.c
+++ b/drivers/gpio/gpio-crystalcove.c
@@ -24,7 +24,7 @@
 #include <linux/mfd/intel_soc_pmic.h>
 
 #define CRYSTALCOVE_GPIO_NUM	16
-#define CRYSTALCOVE_VGPIO_NUM	94
+#define CRYSTALCOVE_VGPIO_NUM	95
 
 #define UPDATE_IRQ_TYPE		BIT(0)
 #define UPDATE_IRQ_MASK		BIT(1)
@@ -39,6 +39,7 @@
 #define GPIO0P0CTLI		0x33
 #define GPIO1P0CTLO		0x3b
 #define GPIO1P0CTLI		0x43
+#define GPIOPANELCTL		0x52
 
 #define CTLI_INTCNT_DIS		(0)
 #define CTLI_INTCNT_NE		(1 << 1)
@@ -93,6 +94,10 @@ static inline int to_reg(int gpio, enum ctrl_register reg_type)
 {
 	int reg;
 
+	if (gpio == 94) {
+		return GPIOPANELCTL;
+	}
+
 	if (reg_type == CTRL_IN) {
 		if (gpio < 8)
 			reg = GPIO0P0CTLI;
-- 
2.1.0

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

[RFC v5 3/9] drm/i915: Use the CRC gpio for panel enable/disable

[Shobhit Kumar]

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)

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 | 34 ++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_dsi.h | 10 ++++++++++
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index c8c8b24..219421c 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,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
 
 	DRM_DEBUG_KMS("\n");
 
+	/* Panel Enable over CRC PMIC */
+	if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC &&
+						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 +440,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 +582,11 @@ 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 (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC &&
+						 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 +966,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);
 }
 
@@ -1070,6 +1086,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..8be75a7 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -29,6 +29,13 @@
 #include <drm/drm_mipi_dsi.h>
 #include "intel_drv.h"
 
+/* CRC PMIC GPIO Access */
+#define GPIO_CHIP_NAME		"gpio_crystalcove"
+#define GPIO_PANEL_EN		94
+
+#define PPS_BLC_PMIC   0
+#define PPS_BLC_SOC    1
+
 /* Dual Link support */
 #define DSI_DUAL_LINK_NONE		0
 #define DSI_DUAL_LINK_FRONT_BACK	1
@@ -42,6 +49,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

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

[RFC v5 4/9] drivers/pwm: Add helper to configure pwm using clock divisor and duty percent

[Shobhit Kumar]

Some chips instead of using period_ns and duty_ns can be configured
using the clock divisor and duty percent. Adds an alternative
configuration method for such chips

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/core.c  | 24 ++++++++++++++++++++++++
 include/linux/pwm.h | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 810aef3..d979bc0 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -422,6 +422,30 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
 EXPORT_SYMBOL_GPL(pwm_config);
 
 /**
+ * pwm_config_alternate() - change a PWM device configuration
+ * @pwm: PWM device
+ * @clk_div: Clock divider to configure
+ * @duty_percentage: duty cycle in percentage
+ */
+int pwm_config_alternate(struct pwm_device *pwm, int clk_div, int duty_percent)
+{
+	int err;
+
+	if (!pwm || clk_div < 0 || duty_percent < 0 || duty_percent > 100)
+		return -EINVAL;
+
+	err = pwm->chip->ops->config_alternate(pwm->chip, pwm, clk_div, duty_percent);
+	if (err)
+		return err;
+
+	pwm->clk_div = clk_div;
+	pwm->duty_percent = duty_percent;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pwm_config_alternate);
+
+/**
  * pwm_set_polarity() - configure the polarity of a PWM signal
  * @pwm: PWM device
  * @polarity: new polarity of the PWM signal
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index e90628c..739cb2b 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -24,6 +24,12 @@ void pwm_free(struct pwm_device *pwm);
 int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
 
 /*
+ * pwm_config_alternate - change a PWM device configuration
+ *			  based on clk_dic and duty_percent
+ */
+int pwm_config_alternate(struct pwm_device *pwm, int clk_div, int duty_percent);
+
+/*
  * pwm_enable - start a PWM output toggling
  */
 int pwm_enable(struct pwm_device *pwm);
@@ -89,6 +95,8 @@ struct pwm_device {
 
 	unsigned int		period; 	/* in nanoseconds */
 	unsigned int		duty_cycle;	/* in nanoseconds */
+	unsigned int		clk_div;
+	unsigned int		duty_percent;
 	enum pwm_polarity	polarity;
 };
 
@@ -114,6 +122,28 @@ static inline unsigned int pwm_get_duty_cycle(struct pwm_device *pwm)
 	return pwm ? pwm->duty_cycle : 0;
 }
 
+static inline void pwm_set_clk_div(struct pwm_device *pwm, unsigned int clk_div)
+{
+	if (pwm)
+		pwm->clk_div = clk_div;
+}
+
+static inline unsigned int pwm_get_clk_div(struct pwm_device *pwm)
+{
+	return pwm ? pwm->clk_div : 0;
+}
+
+static inline void pwm_set_duty_percent(struct pwm_device *pwm, unsigned int duty_percent)
+{
+	if (pwm)
+		pwm->duty_percent = duty_percent;
+}
+
+static inline unsigned int pwm_get_duty_percent(struct pwm_device *pwm)
+{
+	return pwm ? pwm->duty_percent : 0;
+}
+
 /*
  * pwm_set_polarity - configure the polarity of a PWM signal
  */
@@ -138,6 +168,9 @@ struct pwm_ops {
 	int			(*config)(struct pwm_chip *chip,
 					  struct pwm_device *pwm,
 					  int duty_ns, int period_ns);
+	int			(*config_alternate)(struct pwm_chip *chip,
+					  struct pwm_device *pwm,
+					  int clk_div, int duty_percent);
 	int			(*set_polarity)(struct pwm_chip *chip,
 					  struct pwm_device *pwm,
 					  enum pwm_polarity polarity);
-- 
2.1.0

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

[RFC v5 5/9] drivers/mfd: Add PWM cell device for Crystalcove PMIC

[Shobhit Kumar]

Needed for PWM control suuported by the PMIC

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_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 = {
-- 
2.1.0

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

[RFC v5 6/9] drivers/pwm: Add Crystalcove (CRC) PWM driver

[Shobhit Kumar]

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

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 | 161 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 169 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..9dc169e
--- /dev/null
+++ b/drivers/pwm/pwm-crc.c
@@ -0,0 +1,161 @@
+/*
+ * 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
+
+/**
+ * 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_config(struct pwm_chip *c, struct pwm_device *pwm,
+				  int clk_div, int duty_percent)
+{
+	struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
+	struct device *dev = &crc_pwm->pdev->dev;
+	int level;
+
+	dev_dbg(dev, "crc-pwm-config\n");
+
+	if (pwm->clk_div != clk_div) {
+		/* configure the clock divider */
+		if (clk_div > PWM_DIV_CLK_128)
+			clk_div = PWM_DIV_CLK_128;
+
+		regmap_write(crc_pwm->regmap, PWM0_CLK_DIV,
+					clk_div | PWM_OUTPUT_ENABLE);
+
+	}
+
+	/* change the pwm duty cycle */
+	level = (duty_percent * PWM_MAX_LEVEL) / 100;
+	regmap_write(crc_pwm->regmap, PWM0_DUTY_CYCLE, level);
+
+	return 0;
+}
+
+static int crc_pwm_enable(struct pwm_chip *c, struct pwm_device *pwm)
+{
+	struct crystalcove_pwm *crc_pwm = to_crc_pwm(c);
+	struct device *dev = &crc_pwm->pdev->dev;
+
+	dev_dbg(dev, "crc-pwm-enable\n");
+
+	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);
+	struct device *dev = &crc_pwm->pdev->dev;
+
+	dev_dbg(dev, "crc-pwm-disable\n");
+
+	regmap_write(crc_pwm->regmap, BACKLIGHT_EN, 0);
+}
+
+static const struct pwm_ops crc_pwm_ops = {
+	.config_alternate = 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");
-- 
2.1.0

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

[RFC v5 7/9] drivers/pwm: Remove __init initializer for pwm_add_table

[Shobhit Kumar]

For platforms where there are DT, some early MFD modules can reguster
lookup tables. Remove __init initializer so that this works. This is
similar to gpio_add_lookup_table which allows later initializations

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/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index d979bc0..485c75e 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -597,7 +597,7 @@ EXPORT_SYMBOL_GPL(of_pwm_get);
  * @table: array of consumers to register
  * @num: number of consumers in table
  */
-void __init pwm_add_table(struct pwm_lookup *table, size_t num)
+void pwm_add_table(struct pwm_lookup *table, size_t num)
 {
 	mutex_lock(&pwm_lookup_lock);
 
-- 
2.1.0

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

[RFC v5 8/9] drivers/mfd: ADD PWM lookup table for CRC PMIC based PWM

[Shobhit Kumar]

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)

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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c
index 365d5de..b140e3c 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 @@ 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),
+};
+
 /*
  * On some boards the PMIC interrupt may come from a GPIO line.
  * Try to lookup the ACPI table and see if such connection exists. If not,
@@ -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));
-- 
2.1.0

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

[RFC v5 9/9] drm/i915: Backlight control using CRC PMIC based PWM driver

[Shobhit Kumar]

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 | 25 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dsi.h |  3 +++
 2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 219421c..511446f 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -32,6 +32,7 @@
 #include <drm/drm_mipi_dsi.h>
 #include <linux/slab.h>
 #include <linux/gpio/consumer.h>
+#include <linux/pwm.h>
 #include "i915_drv.h"
 #include "intel_drv.h"
 #include "intel_dsi.h"
@@ -402,6 +403,10 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
 
 		intel_dsi_port_enable(encoder);
 	}
+
+	/* Enable the backlight with default PWM as programmed by BIOS */
+	pwm_enable(intel_dsi->pwm);
+	pwm_config_alternate(intel_dsi->pwm, 0x7F, 100);
 }
 
 static void intel_dsi_pre_enable(struct intel_encoder *encoder)
@@ -467,6 +472,11 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder)
 
 	DRM_DEBUG_KMS("\n");
 
+	/* Disable the backlight */
+	pwm_config_alternate(intel_dsi->pwm, 0x7F, 0);
+	usleep_range(2000, 3000);
+	pwm_disable(intel_dsi->pwm);
+
 	if (is_vid_mode(intel_dsi)) {
 		/* Send Shutdown command to the panel in LP mode */
 		for_each_dsi_port(port, intel_dsi->ports)
@@ -971,6 +981,10 @@ static void intel_dsi_encoder_destroy(struct drm_encoder *encoder)
 	if (intel_dsi->gpio_panel)
 		gpiod_put(intel_dsi->gpio_panel);
 
+	/* dispose of the pwm */
+	if (intel_dsi->pwm)
+		pwm_put(intel_dsi->pwm);
+
 	intel_encoder_destroy(encoder);
 }
 
@@ -1098,6 +1112,17 @@ void intel_dsi_init(struct drm_device *dev)
 			DRM_ERROR("Failed to own gpio for panel control\n");
 			intel_dsi->gpio_panel = NULL;
 		}
+
+		/* Get the PWM chip for backlight control */
+		intel_dsi->pwm = pwm_get(dev->dev, "pwm_backlight");
+		if (IS_ERR(intel_dsi->pwm)) {
+			DRM_ERROR("Faild to own the pwm chip\n");
+			intel_dsi->pwm = NULL;
+		} else if (pwm_config_alternate(intel_dsi->pwm, 0x7F, 100) < 0) {
+			DRM_ERROR("Failed to configure the pwm chip\n");
+			pwm_put(intel_dsi->pwm);
+			intel_dsi->pwm = NULL;
+		}
 	}
 
 	intel_encoder->type = INTEL_OUTPUT_DSI;
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index 8be75a7..d8ec9c1 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -52,6 +52,9 @@ struct intel_dsi {
 	/* GPIO Desc for CRC based Panel control */
 	struct gpio_desc *gpio_panel;
 
+	/* PWM chip */
+	struct pwm_device *pwm;
+
 	struct intel_connector *attached_connector;
 
 	/* bit mask of ports being driven */
-- 
2.1.0

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

[PATCH 4/9] drivers/pwm: Add helper to configure pwm using clock divisor and duty percent

[Shobhit Kumar]

Some chips instead of using period_ns and duty_ns can be configured
using the clock divisor and duty percent. Adds an alternative
configuration method for such chips

v2: Corrected the chip validation for config_alternate in pwmchip_add

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/core.c  | 27 ++++++++++++++++++++++++++-
 include/linux/pwm.h | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 810aef3..604e93d 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -235,7 +235,8 @@ int pwmchip_add(struct pwm_chip *chip)
 	unsigned int i;
 	int ret;
 
-	if (!chip || !chip->dev || !chip->ops || !chip->ops->config ||
+	if (!chip || !chip->dev || !chip->ops ||
+	    !(chip->ops->config || chip->ops->config_alternate) ||
 	    !chip->ops->enable || !chip->ops->disable || !chip->npwm)
 		return -EINVAL;
 
@@ -422,6 +423,30 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
 EXPORT_SYMBOL_GPL(pwm_config);
 
 /**
+ * pwm_config_alternate() - change a PWM device configuration
+ * @pwm: PWM device
+ * @clk_div: Clock divider to configure
+ * @duty_percentage: duty cycle in percentage
+ */
+int pwm_config_alternate(struct pwm_device *pwm, int clk_div, int duty_percent)
+{
+	int err;
+
+	if (!pwm || clk_div < 0 || duty_percent < 0 || duty_percent > 100)
+		return -EINVAL;
+
+	err = pwm->chip->ops->config_alternate(pwm->chip, pwm, clk_div, duty_percent);
+	if (err)
+		return err;
+
+	pwm->clk_div = clk_div;
+	pwm->duty_percent = duty_percent;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pwm_config_alternate);
+
+/**
  * pwm_set_polarity() - configure the polarity of a PWM signal
  * @pwm: PWM device
  * @polarity: new polarity of the PWM signal
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index e90628c..739cb2b 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -24,6 +24,12 @@ void pwm_free(struct pwm_device *pwm);
 int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
 
 /*
+ * pwm_config_alternate - change a PWM device configuration
+ *			  based on clk_dic and duty_percent
+ */
+int pwm_config_alternate(struct pwm_device *pwm, int clk_div, int duty_percent);
+
+/*
  * pwm_enable - start a PWM output toggling
  */
 int pwm_enable(struct pwm_device *pwm);
@@ -89,6 +95,8 @@ struct pwm_device {
 
 	unsigned int		period; 	/* in nanoseconds */
 	unsigned int		duty_cycle;	/* in nanoseconds */
+	unsigned int		clk_div;
+	unsigned int		duty_percent;
 	enum pwm_polarity	polarity;
 };
 
@@ -114,6 +122,28 @@ static inline unsigned int pwm_get_duty_cycle(struct pwm_device *pwm)
 	return pwm ? pwm->duty_cycle : 0;
 }
 
+static inline void pwm_set_clk_div(struct pwm_device *pwm, unsigned int clk_div)
+{
+	if (pwm)
+		pwm->clk_div = clk_div;
+}
+
+static inline unsigned int pwm_get_clk_div(struct pwm_device *pwm)
+{
+	return pwm ? pwm->clk_div : 0;
+}
+
+static inline void pwm_set_duty_percent(struct pwm_device *pwm, unsigned int duty_percent)
+{
+	if (pwm)
+		pwm->duty_percent = duty_percent;
+}
+
+static inline unsigned int pwm_get_duty_percent(struct pwm_device *pwm)
+{
+	return pwm ? pwm->duty_percent : 0;
+}
+
 /*
  * pwm_set_polarity - configure the polarity of a PWM signal
  */
@@ -138,6 +168,9 @@ struct pwm_ops {
 	int			(*config)(struct pwm_chip *chip,
 					  struct pwm_device *pwm,
 					  int duty_ns, int period_ns);
+	int			(*config_alternate)(struct pwm_chip *chip,
+					  struct pwm_device *pwm,
+					  int clk_div, int duty_percent);
 	int			(*set_polarity)(struct pwm_chip *chip,
 					  struct pwm_device *pwm,
 					  enum pwm_polarity polarity);
-- 
2.1.0

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

Re: [RFC v5 3/9] drm/i915: Use the CRC gpio for panel enable/disable

[Ville Syrjälä]

On Thu, Mar 12, 2015 at 10:01:27PM +0530, Shobhit Kumar 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)

I definitely like how this looks. No pmic specific information has
leaked into i915. I can't really comment on the gpio/pwm specific bits
as I'm all that familiar with them, but based on a cursory look it
seemed pretty nice as well.

Just a few small bikesheds below about the i915 bits.

> 
> 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 | 34 ++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_dsi.h | 10 ++++++++++
>  2 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index c8c8b24..219421c 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,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>  
>  	DRM_DEBUG_KMS("\n");
>  
> +	/* Panel Enable over CRC PMIC */
> +	if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC &&
> +						intel_dsi->gpio_panel)

The vbt check here seems redundant.

> +		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 +440,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 +582,11 @@ 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 (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC &&
> +						 intel_dsi->gpio_panel)

ditto

> +		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 0);
>  }
>  
>  static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
> @@ -955,6 +966,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);
>  }
>  
> @@ -1070,6 +1086,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..8be75a7 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -29,6 +29,13 @@
>  #include <drm/drm_mipi_dsi.h>
>  #include "intel_drv.h"
>  
> +/* CRC PMIC GPIO Access */
> +#define GPIO_CHIP_NAME		"gpio_crystalcove"
> +#define GPIO_PANEL_EN		94

These defines seem to be unused.

> +
> +#define PPS_BLC_PMIC   0
> +#define PPS_BLC_SOC    1
> +
>  /* Dual Link support */
>  #define DSI_DUAL_LINK_NONE		0
>  #define DSI_DUAL_LINK_FRONT_BACK	1
> @@ -42,6 +49,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
> 
> _______________________________________________
> 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

Re: [RFC v5 9/9] drm/i915: Backlight control using CRC PMIC based PWM driver

[Ville Syrjälä]

On Thu, Mar 12, 2015 at 10:01:33PM +0530, Shobhit Kumar wrote:
> 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 | 25 +++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dsi.h |  3 +++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 219421c..511446f 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -32,6 +32,7 @@
>  #include <drm/drm_mipi_dsi.h>
>  #include <linux/slab.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/pwm.h>
>  #include "i915_drv.h"
>  #include "intel_drv.h"
>  #include "intel_dsi.h"
> @@ -402,6 +403,10 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>  
>  		intel_dsi_port_enable(encoder);
>  	}
> +
> +	/* Enable the backlight with default PWM as programmed by BIOS */
> +	pwm_enable(intel_dsi->pwm);
> +	pwm_config_alternate(intel_dsi->pwm, 0x7F, 100);

I suppose we migth want to wrap these in 'if (intel_dsi->pwm)' checks,
or does the pwm subsystem take care of NULL checks for us?

>  }
>  
>  static void intel_dsi_pre_enable(struct intel_encoder *encoder)
> @@ -467,6 +472,11 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder)
>  
>  	DRM_DEBUG_KMS("\n");
>  
> +	/* Disable the backlight */
> +	pwm_config_alternate(intel_dsi->pwm, 0x7F, 0);
> +	usleep_range(2000, 3000);
> +	pwm_disable(intel_dsi->pwm);

ditto

> +
>  	if (is_vid_mode(intel_dsi)) {
>  		/* Send Shutdown command to the panel in LP mode */
>  		for_each_dsi_port(port, intel_dsi->ports)
> @@ -971,6 +981,10 @@ static void intel_dsi_encoder_destroy(struct drm_encoder *encoder)
>  	if (intel_dsi->gpio_panel)
>  		gpiod_put(intel_dsi->gpio_panel);
>  
> +	/* dispose of the pwm */
> +	if (intel_dsi->pwm)
> +		pwm_put(intel_dsi->pwm);
> +
>  	intel_encoder_destroy(encoder);
>  }
>  
> @@ -1098,6 +1112,17 @@ void intel_dsi_init(struct drm_device *dev)
>  			DRM_ERROR("Failed to own gpio for panel control\n");
>  			intel_dsi->gpio_panel = NULL;
>  		}
> +
> +		/* Get the PWM chip for backlight control */
> +		intel_dsi->pwm = pwm_get(dev->dev, "pwm_backlight");
> +		if (IS_ERR(intel_dsi->pwm)) {
> +			DRM_ERROR("Faild to own the pwm chip\n");
> +			intel_dsi->pwm = NULL;
> +		} else if (pwm_config_alternate(intel_dsi->pwm, 0x7F, 100) < 0) {
> +			DRM_ERROR("Failed to configure the pwm chip\n");
> +			pwm_put(intel_dsi->pwm);
> +			intel_dsi->pwm = NULL;
> +		}
>  	}
>  
>  	intel_encoder->type = INTEL_OUTPUT_DSI;
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index 8be75a7..d8ec9c1 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -52,6 +52,9 @@ struct intel_dsi {
>  	/* GPIO Desc for CRC based Panel control */
>  	struct gpio_desc *gpio_panel;
>  
> +	/* PWM chip */
> +	struct pwm_device *pwm;
> +
>  	struct intel_connector *attached_connector;
>  
>  	/* bit mask of ports being driven */
> -- 
> 2.1.0
> 
> _______________________________________________
> 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

Re: [RFC v5 9/9] drm/i915: Backlight control using CRC PMIC based PWM driver

[Daniel Vetter]

On Fri, Mar 13, 2015 at 04:30:43PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 12, 2015 at 10:01:33PM +0530, Shobhit Kumar wrote:
> > 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 | 25 +++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dsi.h |  3 +++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > index 219421c..511446f 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -32,6 +32,7 @@
> >  #include <drm/drm_mipi_dsi.h>
> >  #include <linux/slab.h>
> >  #include <linux/gpio/consumer.h>
> > +#include <linux/pwm.h>
> >  #include "i915_drv.h"
> >  #include "intel_drv.h"
> >  #include "intel_dsi.h"
> > @@ -402,6 +403,10 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
> >  
> >  		intel_dsi_port_enable(encoder);
> >  	}
> > +
> > +	/* Enable the backlight with default PWM as programmed by BIOS */
> > +	pwm_enable(intel_dsi->pwm);
> > +	pwm_config_alternate(intel_dsi->pwm, 0x7F, 100);
> 
> I suppose we migth want to wrap these in 'if (intel_dsi->pwm)' checks,
> or does the pwm subsystem take care of NULL checks for us?
> 
> >  }
> >  
> >  static void intel_dsi_pre_enable(struct intel_encoder *encoder)
> > @@ -467,6 +472,11 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder)
> >  
> >  	DRM_DEBUG_KMS("\n");
> >  
> > +	/* Disable the backlight */
> > +	pwm_config_alternate(intel_dsi->pwm, 0x7F, 0);
> > +	usleep_range(2000, 3000);
> > +	pwm_disable(intel_dsi->pwm);
> 
> ditto
> 
> > +
> >  	if (is_vid_mode(intel_dsi)) {
> >  		/* Send Shutdown command to the panel in LP mode */
> >  		for_each_dsi_port(port, intel_dsi->ports)
> > @@ -971,6 +981,10 @@ static void intel_dsi_encoder_destroy(struct drm_encoder *encoder)
> >  	if (intel_dsi->gpio_panel)
> >  		gpiod_put(intel_dsi->gpio_panel);
> >  
> > +	/* dispose of the pwm */
> > +	if (intel_dsi->pwm)
> > +		pwm_put(intel_dsi->pwm);
> > +
> >  	intel_encoder_destroy(encoder);
> >  }
> >  
> > @@ -1098,6 +1112,17 @@ void intel_dsi_init(struct drm_device *dev)
> >  			DRM_ERROR("Failed to own gpio for panel control\n");
> >  			intel_dsi->gpio_panel = NULL;
> >  		}
> > +
> > +		/* Get the PWM chip for backlight control */
> > +		intel_dsi->pwm = pwm_get(dev->dev, "pwm_backlight");
> > +		if (IS_ERR(intel_dsi->pwm)) {
> > +			DRM_ERROR("Faild to own the pwm chip\n");
> > +			intel_dsi->pwm = NULL;

If the i915.ko driver loads before the mfd.ko driver we need to do a
deferred probe here I think. Just failing isn't robust.

This also means that if people don't configure their kernel properly then
we'll fall over here, but meh.
-Daniel

> > +		} else if (pwm_config_alternate(intel_dsi->pwm, 0x7F, 100) < 0) {
> > +			DRM_ERROR("Failed to configure the pwm chip\n");
> > +			pwm_put(intel_dsi->pwm);
> > +			intel_dsi->pwm = NULL;
> > +		}
> >  	}
> >  
> >  	intel_encoder->type = INTEL_OUTPUT_DSI;
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> > index 8be75a7..d8ec9c1 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.h
> > +++ b/drivers/gpu/drm/i915/intel_dsi.h
> > @@ -52,6 +52,9 @@ struct intel_dsi {
> >  	/* GPIO Desc for CRC based Panel control */
> >  	struct gpio_desc *gpio_panel;
> >  
> > +	/* PWM chip */
> > +	struct pwm_device *pwm;
> > +
> >  	struct intel_connector *attached_connector;
> >  
> >  	/* bit mask of ports being driven */
> > -- 
> > 2.1.0
> > 
> > _______________________________________________
> > 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v5 9/9] drm/i915: Backlight control using CRC PMIC based PWM driver

[Shobhit Kumar]

On 03/13/2015 10:50 PM, Daniel Vetter wrote:
> On Fri, Mar 13, 2015 at 04:30:43PM +0200, Ville Syrjälä wrote:
>> On Thu, Mar 12, 2015 at 10:01:33PM +0530, Shobhit Kumar wrote:
>>> 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 | 25 +++++++++++++++++++++++++
>>>  drivers/gpu/drm/i915/intel_dsi.h |  3 +++
>>>  2 files changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>>> index 219421c..511446f 100644
>>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>>> @@ -32,6 +32,7 @@
>>>  #include <drm/drm_mipi_dsi.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/gpio/consumer.h>
>>> +#include <linux/pwm.h>
>>>  #include "i915_drv.h"
>>>  #include "intel_drv.h"
>>>  #include "intel_dsi.h"
>>> @@ -402,6 +403,10 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>>>  
>>>  		intel_dsi_port_enable(encoder);
>>>  	}
>>> +
>>> +	/* Enable the backlight with default PWM as programmed by BIOS */
>>> +	pwm_enable(intel_dsi->pwm);
>>> +	pwm_config_alternate(intel_dsi->pwm, 0x7F, 100);
>>
>> I suppose we migth want to wrap these in 'if (intel_dsi->pwm)' checks,
>> or does the pwm subsystem take care of NULL checks for us?
>>
>>>  }
>>>  
>>>  static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>>> @@ -467,6 +472,11 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder)
>>>  
>>>  	DRM_DEBUG_KMS("\n");
>>>  
>>> +	/* Disable the backlight */
>>> +	pwm_config_alternate(intel_dsi->pwm, 0x7F, 0);
>>> +	usleep_range(2000, 3000);
>>> +	pwm_disable(intel_dsi->pwm);
>>
>> ditto
>>
>>> +
>>>  	if (is_vid_mode(intel_dsi)) {
>>>  		/* Send Shutdown command to the panel in LP mode */
>>>  		for_each_dsi_port(port, intel_dsi->ports)
>>> @@ -971,6 +981,10 @@ static void intel_dsi_encoder_destroy(struct drm_encoder *encoder)
>>>  	if (intel_dsi->gpio_panel)
>>>  		gpiod_put(intel_dsi->gpio_panel);
>>>  
>>> +	/* dispose of the pwm */
>>> +	if (intel_dsi->pwm)
>>> +		pwm_put(intel_dsi->pwm);
>>> +
>>>  	intel_encoder_destroy(encoder);
>>>  }
>>>  
>>> @@ -1098,6 +1112,17 @@ void intel_dsi_init(struct drm_device *dev)
>>>  			DRM_ERROR("Failed to own gpio for panel control\n");
>>>  			intel_dsi->gpio_panel = NULL;
>>>  		}
>>> +
>>> +		/* Get the PWM chip for backlight control */
>>> +		intel_dsi->pwm = pwm_get(dev->dev, "pwm_backlight");
>>> +		if (IS_ERR(intel_dsi->pwm)) {
>>> +			DRM_ERROR("Faild to own the pwm chip\n");
>>> +			intel_dsi->pwm = NULL;
> 
> If the i915.ko driver loads before the mfd.ko driver we need to do a
> deferred probe here I think. Just failing isn't robust.

The PMIC MFD driver is a boolean if you remember our discussions earlier
and hence I made the PWM driver also a bool config. Given that i915 is a
.ko in default kernel config, perhaps it should not be that big an issue
for now. But I will look into deferred probe as a fail safe.

> 
> This also means that if people don't configure their kernel properly then
> we'll fall over here, but meh.

True.

Regards
Shobhit

> -Daniel
> 
>>> +		} else if (pwm_config_alternate(intel_dsi->pwm, 0x7F, 100) < 0) {
>>> +			DRM_ERROR("Failed to configure the pwm chip\n");
>>> +			pwm_put(intel_dsi->pwm);
>>> +			intel_dsi->pwm = NULL;
>>> +		}
>>>  	}
>>>  
>>>  	intel_encoder->type = INTEL_OUTPUT_DSI;
>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
>>> index 8be75a7..d8ec9c1 100644
>>> --- a/drivers/gpu/drm/i915/intel_dsi.h
>>> +++ b/drivers/gpu/drm/i915/intel_dsi.h
>>> @@ -52,6 +52,9 @@ struct intel_dsi {
>>>  	/* GPIO Desc for CRC based Panel control */
>>>  	struct gpio_desc *gpio_panel;
>>>  
>>> +	/* PWM chip */
>>> +	struct pwm_device *pwm;
>>> +
>>>  	struct intel_connector *attached_connector;
>>>  
>>>  	/* bit mask of ports being driven */
>>> -- 
>>> 2.1.0
>>>
>>> _______________________________________________
>>> 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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v5 9/9] drm/i915: Backlight control using CRC PMIC based PWM driver

[Shobhit Kumar]

On 03/13/2015 08:00 PM, Ville Syrjälä wrote:
> On Thu, Mar 12, 2015 at 10:01:33PM +0530, Shobhit Kumar wrote:
>> 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 | 25 +++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_dsi.h |  3 +++
>>  2 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 219421c..511446f 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -32,6 +32,7 @@
>>  #include <drm/drm_mipi_dsi.h>
>>  #include <linux/slab.h>
>>  #include <linux/gpio/consumer.h>
>> +#include <linux/pwm.h>
>>  #include "i915_drv.h"
>>  #include "intel_drv.h"
>>  #include "intel_dsi.h"
>> @@ -402,6 +403,10 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>>  
>>  		intel_dsi_port_enable(encoder);
>>  	}
>> +
>> +	/* Enable the backlight with default PWM as programmed by BIOS */
>> +	pwm_enable(intel_dsi->pwm);
>> +	pwm_config_alternate(intel_dsi->pwm, 0x7F, 100);
> 
> I suppose we migth want to wrap these in 'if (intel_dsi->pwm)' checks,
> or does the pwm subsystem take care of NULL checks for us?

PWM subsystem does make the checks. Actually this patch is kind of
prelim patch and I have to still hook up pwm usage in our back-light
infra in intel_panel.c. I have a working patch for that though not
posted yet. Will take care in that final patch for enabling this.

Regards
Shobhit

> 
>>  }
>>  
>>  static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>> @@ -467,6 +472,11 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder)
>>  
>>  	DRM_DEBUG_KMS("\n");
>>  
>> +	/* Disable the backlight */
>> +	pwm_config_alternate(intel_dsi->pwm, 0x7F, 0);
>> +	usleep_range(2000, 3000);
>> +	pwm_disable(intel_dsi->pwm);
> 
> ditto
> 
>> +
>>  	if (is_vid_mode(intel_dsi)) {
>>  		/* Send Shutdown command to the panel in LP mode */
>>  		for_each_dsi_port(port, intel_dsi->ports)
>> @@ -971,6 +981,10 @@ static void intel_dsi_encoder_destroy(struct drm_encoder *encoder)
>>  	if (intel_dsi->gpio_panel)
>>  		gpiod_put(intel_dsi->gpio_panel);
>>  
>> +	/* dispose of the pwm */
>> +	if (intel_dsi->pwm)
>> +		pwm_put(intel_dsi->pwm);
>> +
>>  	intel_encoder_destroy(encoder);
>>  }
>>  
>> @@ -1098,6 +1112,17 @@ void intel_dsi_init(struct drm_device *dev)
>>  			DRM_ERROR("Failed to own gpio for panel control\n");
>>  			intel_dsi->gpio_panel = NULL;
>>  		}
>> +
>> +		/* Get the PWM chip for backlight control */
>> +		intel_dsi->pwm = pwm_get(dev->dev, "pwm_backlight");
>> +		if (IS_ERR(intel_dsi->pwm)) {
>> +			DRM_ERROR("Faild to own the pwm chip\n");
>> +			intel_dsi->pwm = NULL;
>> +		} else if (pwm_config_alternate(intel_dsi->pwm, 0x7F, 100) < 0) {
>> +			DRM_ERROR("Failed to configure the pwm chip\n");
>> +			pwm_put(intel_dsi->pwm);
>> +			intel_dsi->pwm = NULL;
>> +		}
>>  	}
>>  
>>  	intel_encoder->type = INTEL_OUTPUT_DSI;
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
>> index 8be75a7..d8ec9c1 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.h
>> +++ b/drivers/gpu/drm/i915/intel_dsi.h
>> @@ -52,6 +52,9 @@ struct intel_dsi {
>>  	/* GPIO Desc for CRC based Panel control */
>>  	struct gpio_desc *gpio_panel;
>>  
>> +	/* PWM chip */
>> +	struct pwm_device *pwm;
>> +
>>  	struct intel_connector *attached_connector;
>>  
>>  	/* bit mask of ports being driven */
>> -- 
>> 2.1.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable

[Shobhit Kumar]

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 c8c8b24..06a5c30 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);
 }
 
@@ -1070,6 +1084,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
+
 /* 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

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

Re: [RFC v5 2/9] gpio/crystalcove: Add additional GPIO for Panel control

[Linus Walleij]

On Thu, Mar 12, 2015 at 5:31 PM, Shobhit Kumar <shobhit.kumar@intel.com> wrote:

> Export PANEL_EN/DISABLE (offset 0x52) as additional GPIO. Needed
> by display driver to enable the DSI panel on BYT platform where
> the Panel EN/Disable control is routed thorugh CRC PMIC
>
> 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>

NACK.

Spawn a separate MFD cell and write a fixed voltage
regulator driver for this.

> @@ -39,6 +39,7 @@
>  #define GPIO0P0CTLI            0x33
>  #define GPIO1P0CTLO            0x3b
>  #define GPIO1P0CTLI            0x43
> +#define GPIOPANELCTL           0x52

This is even a lie, you say in the commit message that the
register is indeed named PANEL_EN/DISABLE and is not
a GPIO.

Yours,
Linus Walleij
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable

[Linus Walleij]

On Mon, Mar 16, 2015 at 5:42 AM, 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>

NACK.

This is not a GPIO but a special-purpose register as can be
seen from other discussions.

Use a fixed voltage regulator spun from an MFD cell instead
of shoehorning this into GPIO.

Yours,
Linus Walleij
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drivers/pwm: Add helper to configure pwm using clock divisor and duty percent

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 760 bytes --]

On Fri, Mar 13, 2015 at 07:28:02PM +0530, Shobhit Kumar wrote:
> Some chips instead of using period_ns and duty_ns can be configured
> using the clock divisor and duty percent. Adds an alternative
> configuration method for such chips

I don't see a need to introduce this alternative configuration
mechanism. Most, of not all, of the other drivers program a clock
divisor and some percentage of the duty cycle as well and it should be
easy to convert to that internally from the period and duty_cycle
parameters that you get in ->config().

Adding an alternative means of configuring the PWM also means that every
user driver now potentially needs to support both the traditional and
the alternative way because PWM providers may not implement both.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable

[Daniel Vetter]

On Wed, Mar 18, 2015 at 01:19:33PM +0100, Linus Walleij wrote:
> On Mon, Mar 16, 2015 at 5:42 AM, 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>
> 
> NACK.
> 
> This is not a GPIO but a special-purpose register as can be
> seen from other discussions.
> 
> Use a fixed voltage regulator spun from an MFD cell instead
> of shoehorning this into GPIO.

Hm, somehow my reply here got eaten. I've looked at the regulator stuff
and that doesn't make sense. The problem here is that i915.ko is in
control of the regulator crap like on/off delays and proper ordering with
everything else that must happen to enable/disable the panel without it
falling over. Also i915.ko is the only thing that actually knows which
gpio line to use (there's atm 3 different board designs afaik).

The crystalcove pmic thing here really is just a dumb gpio line that for
the reference design gets routed to the panel (and hence has that as the
usual name). And what we want to do here is get some reasonable way to
route that gpio line control between two totally different drivers.

Also please note that x86 is a lot shittier than arm for this kind of
inter-driver-depencies since we don't even have board files. Hence also
why this series has some patches to expose the board file init stuff to
the pmic driver for setup.

Anyway if you still think gpio is totally the wrong thing then we'll just
roll our own using the component framework. regulator is definitely a even
bigger mismatch at least for our usage. I just figured it would make sense
to reuse existing common infrastructure where it fits.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v5 1/9] drivers/mfd: Add lookup table for Panel Control as GPIO signal

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2609 bytes --]

On Thu, Mar 12, 2015 at 10:01:25PM +0530, 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
> 
> 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 | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c
> index 80cef04..365d5de 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 */
> +struct gpiod_lookup_table panel_gpio_table = {

Should this be static?

> +	/* Intel GFX is consumer */
> +	.dev_id = "0000:00:02.0",
> +	.table = {
> +		/* Panel EN/DISABLE */
> +		GPIO_LOOKUP("gpio_crystalcove", 94, "panel", GPIO_ACTIVE_HIGH),
> +	},
> +};
> +
>  /*
>   * On some boards the PMIC interrupt may come from a GPIO line.
>   * Try to lookup the ACPI table and see if such connection exists. If not,
> @@ -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);
> +

There's no corresponding gpiod_remove_lookup_table() call anywhere. That
is understandable, given that that function doesn't exist. However, this
driver can be unloaded (or at least unbound from the device), at which
point the data effectively becomes stale. This shouldn't pose much of a
problem because the driver can't be built as a module, so the data will
stick around. However, what happens if you unload and then reload the
driver? You'll be adding a second instance of the GPIO table. Given that
the data is identical maybe that isn't even a problem. Then again, it's
probably going to mess up the global list because you're adding the same
entry twice.

So I think that's something we need to fix. The same is true for the PWM
lookup table in a later patch.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [RFC v5 9/9] drm/i915: Backlight control using CRC PMIC based PWM driver

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2111 bytes --]

On Fri, Mar 13, 2015 at 04:30:43PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 12, 2015 at 10:01:33PM +0530, Shobhit Kumar wrote:
> > 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 | 25 +++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dsi.h |  3 +++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > index 219421c..511446f 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -32,6 +32,7 @@
> >  #include <drm/drm_mipi_dsi.h>
> >  #include <linux/slab.h>
> >  #include <linux/gpio/consumer.h>
> > +#include <linux/pwm.h>
> >  #include "i915_drv.h"
> >  #include "intel_drv.h"
> >  #include "intel_dsi.h"
> > @@ -402,6 +403,10 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
> >  
> >  		intel_dsi_port_enable(encoder);
> >  	}
> > +
> > +	/* Enable the backlight with default PWM as programmed by BIOS */
> > +	pwm_enable(intel_dsi->pwm);
> > +	pwm_config_alternate(intel_dsi->pwm, 0x7F, 100);
> 
> I suppose we migth want to wrap these in 'if (intel_dsi->pwm)' checks,
> or does the pwm subsystem take care of NULL checks for us?

The PWM core checks for valid PWM devices and will return an error if
you pass in an invalid device. What this is completely missing is any
kind of error checking. But perhaps you don't care about failure here
for this particular platform? It would be useful to debug issues with
black screens and such I can imagine.

Also, though it's admittedly somewhat underdocumented, the typical
sequence of calls should be pwm_config() followed by pwm_enable(). The
reason is that some devices don't support being configured while
enabled. But you may not care about this here since you're always
dealing with a fixed device anyway.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable

[Linus Walleij]

On Tue, Mar 24, 2015 at 9:32 AM, Daniel Vetter <daniel@ffwll.ch> wrote:

> The crystalcove pmic thing here really is just a dumb gpio line that for
> the reference design gets routed to the panel (and hence has that as the
> usual name).

So obviously the refman calls this register at offset 0x52
PANEL_EN/DISABLE not GPIO_FOO.

> And what we want to do here is get some reasonable way to
> route that gpio line control between two totally different drivers.
>
> Also please note that x86 is a lot shittier than arm for this kind of
> inter-driver-depencies since we don't even have board files. Hence also
> why this series has some patches to expose the board file init stuff to
> the pmic driver for setup.

We don't do board files for ARM anymore either. We do this
using device tree which is similar to how x86 use ACPI.

> Anyway if you still think gpio is totally the wrong thing then we'll just
> roll our own using the component framework.

I guess I better not say no if the alternative is even uglier.

The problem I have is GPIO being used as kitchen sink, and
a recent incident where someone instantiated generic GPIO
chips over some 32bit register just to basically poke bits in that
register to turn LEDs on/off. So a layer cake like:
GPIO LEDs <-> generic GPIO <-> Register.

It's nice and simple for that user as it's using existing kernel code
and just needs some device tree abstract stuff to get going, and
it works. The problem is that it takes something that is not
GPIO (rather a set of bits controlling LEDs) and pretends that
it is GPIO, while it is not, just to be able to use GPIO LEDs.
While the real solution is to write a pure LED driver, possibly
one using some random 32bit registers, LED MMIO or whatever.

Anyway. I still think a fixed voltage regulator would be suitable
for this.

Here is an example of how we do device tree:

        en_3v3_reg: en_3v3 {
                compatible = "regulator-fixed";
                regulator-name = "en-3v3-fixed-supply";
                regulator-min-microvolt = <3300000>;
                regulator-max-microvolt = <3300000>;
                gpio = <&ab8500_gpio 25 0x4>;
                startup-delay-us = <5000>;
                enable-active-high;
        };

In this case the regulator is based on top of a GPIO pin,
so by setting that GPIO line high, some feature on the PCB
will drive a voltage at 3.3V.

A MFD cell spawned regulator can be a very smallish
thing in drivers/regulator. Sure, not 5 lines in the existing
GPIO driver, but still smallish.

You may actually *need* to use the fixed voltage regulator
too: if you have a power-on-delay for it, that the software need
to take into account, the regulator framework is your friend.
Else there will be kludgy code surrounding the GPIO enable()
operation to reimplement the same solution (I have seen this
happen).

So I imagine something like this in drivers/regulator/crystal-panel.c:

#include <linux/platform_device.h>
#include <linux/bitops.h>
#include <linux/regmap.h>
#include <linux/mfd/intel_soc_pmic.h>

#define PANEL_EN 0x52

static int crystal_enable_regulator(struct regulator_dev *reg)
{
        struct intel_soc_pmic *pmic = rdev_get_drvdata(reg);

        return regmap_update_bits(pmic->regmap, PANEL_EN, 1, 1);
}

static int crystal_disable_regulator(struct regulator_dev *reg)
{
        struct intel_soc_pmic *pmic = rdev_get_drvdata(reg);

        return regmap_update_bits(pmic->regmap, PANEL_EN, 1, 0);
}

static int crystal_is_enabled_regulator(struct regulator_dev *reg)
{
        struct intel_soc_pmic *pmic = rdev_get_drvdata(reg);
        int ret;
        unsigned int val;

        ret = regmap_read(pmic->regmap, PANEL_EN, &val);
        if (ret)
                return ret;

        return val & 0x1;
}

static struct regulator_ops crystal_panel_reg_ops = {
        .enable      = crystal_enable_regulator,
        .disable     = crystal_disable_regulator,
        .is_enabled  = crystal_is_enabled_regulator,
};

static struct regulator_desc crystal_panel_regulator = {
       .name = "crystal-panel",
       .fixed_uV = 3300000, /* Or whatever voltage the panel has */
       .ops = &crystal_panel_reg_ops,
       .id = 1,
       .type = REGULATOR_VOLTAGE,
       .owner = THIS_MODULE,
       .enable_time = NNN, /* This may be relevant! */
};

static int crystalcove_panel_regulator_probe(struct platform_device *pdev)
{
        struct device *dev = pdev->dev.parent;
        struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
        struct regulator_config config = { };

        config.dev = &pdev->dev;
        config.driver_data = pmic;
        return devm_regulator_register(&pdev->dev,
&crystal_panel_regulator, config);
}

Untested, but to me simple and straight-forward.

Some stuff may be needed to associate the regulator with the right
device indeed but nothing horribly complicated.

Yours,
Linus Walleij
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v5 1/9] drivers/mfd: Add lookup table for Panel Control as GPIO signal

[Linus Walleij]

On Thu, Mar 12, 2015 at 5:31 PM, Shobhit Kumar <shobhit.kumar@intel.com> 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
>
> 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>

So as I think it is better to use a fixed voltage regulator (see code
example in the i915 patch) I think this
should register a regulator lookup instead.

Yours,
Linus Walleij
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable

[Daniel Vetter]

On Tue, Mar 24, 2015 at 10:35:32AM +0100, Linus Walleij wrote:
> On Tue, Mar 24, 2015 at 9:32 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > The crystalcove pmic thing here really is just a dumb gpio line that for
> > the reference design gets routed to the panel (and hence has that as the
> > usual name).
> 
> So obviously the refman calls this register at offset 0x52
> PANEL_EN/DISABLE not GPIO_FOO.
> 
> > And what we want to do here is get some reasonable way to
> > route that gpio line control between two totally different drivers.
> >
> > Also please note that x86 is a lot shittier than arm for this kind of
> > inter-driver-depencies since we don't even have board files. Hence also
> > why this series has some patches to expose the board file init stuff to
> > the pmic driver for setup.
> 
> We don't do board files for ARM anymore either. We do this
> using device tree which is similar to how x86 use ACPI.

Maybe I wasnt' clear: This is the i915 gfx driver, we noodle this
information out of an even internally underspecified vbios table. It _is_
worse than board files.

Also these tables specify the precise power sequence using forth (that
part isn't in this series).  Implementing a regulator for this would be a
even bigger lie than whatever kind of lie use see in exposing this as a
gpio.

> > Anyway if you still think gpio is totally the wrong thing then we'll just
> > roll our own using the component framework.
> 
> I guess I better not say no if the alternative is even uglier.
> 
> The problem I have is GPIO being used as kitchen sink, and
> a recent incident where someone instantiated generic GPIO
> chips over some 32bit register just to basically poke bits in that
> register to turn LEDs on/off. So a layer cake like:
> GPIO LEDs <-> generic GPIO <-> Register.
> 
> It's nice and simple for that user as it's using existing kernel code
> and just needs some device tree abstract stuff to get going, and
> it works. The problem is that it takes something that is not
> GPIO (rather a set of bits controlling LEDs) and pretends that
> it is GPIO, while it is not, just to be able to use GPIO LEDs.
> While the real solution is to write a pure LED driver, possibly
> one using some random 32bit registers, LED MMIO or whatever.
> 
> Anyway. I still think a fixed voltage regulator would be suitable
> for this.
> 
> Here is an example of how we do device tree:
> 
>         en_3v3_reg: en_3v3 {
>                 compatible = "regulator-fixed";
>                 regulator-name = "en-3v3-fixed-supply";
>                 regulator-min-microvolt = <3300000>;
>                 regulator-max-microvolt = <3300000>;
>                 gpio = <&ab8500_gpio 25 0x4>;
>                 startup-delay-us = <5000>;
>                 enable-active-high;
>         };
> 
> In this case the regulator is based on top of a GPIO pin,
> so by setting that GPIO line high, some feature on the PCB
> will drive a voltage at 3.3V.
> 
> A MFD cell spawned regulator can be a very smallish
> thing in drivers/regulator. Sure, not 5 lines in the existing
> GPIO driver, but still smallish.
> 
> You may actually *need* to use the fixed voltage regulator
> too: if you have a power-on-delay for it, that the software need
> to take into account, the regulator framework is your friend.
> Else there will be kludgy code surrounding the GPIO enable()
> operation to reimplement the same solution (I have seen this
> happen).
> 
> So I imagine something like this in drivers/regulator/crystal-panel.c:
> 
> #include <linux/platform_device.h>
> #include <linux/bitops.h>
> #include <linux/regmap.h>
> #include <linux/mfd/intel_soc_pmic.h>
> 
> #define PANEL_EN 0x52
> 
> static int crystal_enable_regulator(struct regulator_dev *reg)
> {
>         struct intel_soc_pmic *pmic = rdev_get_drvdata(reg);
> 
>         return regmap_update_bits(pmic->regmap, PANEL_EN, 1, 1);
> }
> 
> static int crystal_disable_regulator(struct regulator_dev *reg)
> {
>         struct intel_soc_pmic *pmic = rdev_get_drvdata(reg);
> 
>         return regmap_update_bits(pmic->regmap, PANEL_EN, 1, 0);
> }
> 
> static int crystal_is_enabled_regulator(struct regulator_dev *reg)
> {
>         struct intel_soc_pmic *pmic = rdev_get_drvdata(reg);
>         int ret;
>         unsigned int val;
> 
>         ret = regmap_read(pmic->regmap, PANEL_EN, &val);
>         if (ret)
>                 return ret;
> 
>         return val & 0x1;
> }
> 
> static struct regulator_ops crystal_panel_reg_ops = {
>         .enable      = crystal_enable_regulator,
>         .disable     = crystal_disable_regulator,
>         .is_enabled  = crystal_is_enabled_regulator,
> };
> 
> static struct regulator_desc crystal_panel_regulator = {
>        .name = "crystal-panel",
>        .fixed_uV = 3300000, /* Or whatever voltage the panel has */
>        .ops = &crystal_panel_reg_ops,
>        .id = 1,
>        .type = REGULATOR_VOLTAGE,
>        .owner = THIS_MODULE,
>        .enable_time = NNN, /* This may be relevant! */
> };
> 
> static int crystalcove_panel_regulator_probe(struct platform_device *pdev)
> {
>         struct device *dev = pdev->dev.parent;
>         struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
>         struct regulator_config config = { };
> 
>         config.dev = &pdev->dev;
>         config.driver_data = pmic;
>         return devm_regulator_register(&pdev->dev,
> &crystal_panel_regulator, config);
> }
> 
> Untested, but to me simple and straight-forward.
> 
> Some stuff may be needed to associate the regulator with the right
> device indeed but nothing horribly complicated.

Nack, really. We've had an epic discussion at ks two years ago about how
arm gpu drivers go overboard with abstraction. We have all the insanity
with power domains, clock trees and whatnoelse in i915.ko, but since it's
all mostly from the same company we have it integrated into one driver
with our own infrastructure. Dave Airlie was telling this everyone and I
fully agree with him - pushing abstraction in the middle of the driver
without the need for it just causes tears.

The problem I have here is that two different pieces on the same board
from the same company which won't ever get reused anywhere else need to do
cross-driver communication about a gpio line. I don't want any kind of
additional abstraction to get into the way at all, the only thing I want
are:
- some function to switch that line without delays or cleverness
  interspersed.
- dynamic lookup.

GPIO seemed a perfect fit, but apparently it isn't. We'll roll our own.

And yeah your code isn't any longer than the gpio version, but regulators
drag in all that conceptual stuff about voltage/delays and depencies that
imo just isn't controlled by the pmic. We've originally abused the panel
interface for all this and Thierry (imo rightfully suggested) that this
isn't a panel but it'd be better to expose the underlying stuff. Again imo
faking all that stuff since you think this looks like a regulator is imo a
worse lie than the exposing this as a gpio.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable

[Linus Walleij]

On Tue, Mar 24, 2015 at 10:50 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Mar 24, 2015 at 10:35:32AM +0100, Linus Walleij wrote:

>> Some stuff may be needed to associate the regulator with the right
>> device indeed but nothing horribly complicated.
>
> Nack, really. We've had an epic discussion at ks two years ago about how
> arm gpu drivers go overboard with abstraction.

I remember it clearly as I was in the room.

And yes IIRC that was indeed very much about the panel abstractions
suggested by Laurent Pinchart.

> We have all the insanity
> with power domains, clock trees and whatnoelse in i915.ko, but since it's
> all mostly from the same company we have it integrated into one driver
> with our own infrastructure. Dave Airlie was telling this everyone and I
> fully agree with him - pushing abstraction in the middle of the driver
> without the need for it just causes tears.

I fully understand this stance and I kind of understand why it came to
this. I had the same discussion a bit with some different graphics
people and DRM+panel drivers really are a special chapter when it
comes to sequencing & stuff. (As is ALSA-soundcard type audio
it seems, or anything multimedia.)

> The problem I have here is that two different pieces on the same board
> from the same company which won't ever get reused anywhere else need to do
> cross-driver communication about a gpio line. I don't want any kind of
> additional abstraction to get into the way at all, the only thing I want
> are:
> - some function to switch that line without delays or cleverness
>   interspersed.
> - dynamic lookup.
>
> GPIO seemed a perfect fit, but apparently it isn't. We'll roll our own.

I'm not dealing in absolutes so I want to know what makes GPIO
such a good fit compared to rolling your own.

I guess the alternative is that the i915 driver gets a handle from
another platform_device on the MFD cell (a different one, say
named panel-power or whatever) and pokes that register itself
(using regmap in the same way that my suggested regulator
code does basically).

I kind of like that because it makes the usecase all evident
like I wanted. And arguably it's a better solution if the i915 driver
want to be as self-contained as possible, using this pattern
that the DRM drivers should take care of all sequencing business.

The DRM driver can then also be used even if GPIO is configured
out of the kernel.

> And yeah your code isn't any longer than the gpio version, but regulators
> drag in all that conceptual stuff about voltage/delays and depencies that
> imo just isn't controlled by the pmic. We've originally abused the panel
> interface for all this and Thierry (imo rightfully suggested) that this
> isn't a panel but it'd be better to expose the underlying stuff. Again imo
> faking all that stuff since you think this looks like a regulator is imo a
> worse lie than the exposing this as a gpio.

Nah it's all lies :)

I think you are right: if the DRM driver wants to control everything
itself it should not use the regulator framework, neither the GPIO
framework, it seems to be an established pattern to roll your own
in these drivers so let's keep with that.

I guess the use case is analogous to a monitor cable sticking out
of a graphics card and sending these power-up/down sequences
to that monitor in some magic way, and that is how DRM thinks
of things. For what I know DRM frowns at the abstracted out
backlight control used by framebuffers in drivers/video/backlight
as well, and prefer to be on top of stuff, so then it should access
stuff on as low level as possible.

Yours,
Linus Walleij
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable

[Daniel Vetter]

On Tue, Mar 24, 2015 at 11:16 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Tue, Mar 24, 2015 at 10:50 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, Mar 24, 2015 at 10:35:32AM +0100, Linus Walleij wrote:
>
>>> Some stuff may be needed to associate the regulator with the right
>>> device indeed but nothing horribly complicated.
>>
>> Nack, really. We've had an epic discussion at ks two years ago about how
>> arm gpu drivers go overboard with abstraction.
>
> I remember it clearly as I was in the room.
>
> And yes IIRC that was indeed very much about the panel abstractions
> suggested by Laurent Pinchart.

Yeah the discussion started because of Laurent's panel framework, but
it was very much a general concern. A bunch of the arm gfx drivers are
a pile of modules, and the resulting coordinatino chaos makes
suspend/resume and driver load a real challenge to get right. Drivers
which only modularize where it's absolutely needed are much easier to
understand.

>> We have all the insanity
>> with power domains, clock trees and whatnoelse in i915.ko, but since it's
>> all mostly from the same company we have it integrated into one driver
>> with our own infrastructure. Dave Airlie was telling this everyone and I
>> fully agree with him - pushing abstraction in the middle of the driver
>> without the need for it just causes tears.
>
> I fully understand this stance and I kind of understand why it came to
> this. I had the same discussion a bit with some different graphics
> people and DRM+panel drivers really are a special chapter when it
> comes to sequencing & stuff. (As is ALSA-soundcard type audio
> it seems, or anything multimedia.)
>
>> The problem I have here is that two different pieces on the same board
>> from the same company which won't ever get reused anywhere else need to do
>> cross-driver communication about a gpio line. I don't want any kind of
>> additional abstraction to get into the way at all, the only thing I want
>> are:
>> - some function to switch that line without delays or cleverness
>>   interspersed.
>> - dynamic lookup.
>>
>> GPIO seemed a perfect fit, but apparently it isn't. We'll roll our own.
>
> I'm not dealing in absolutes so I want to know what makes GPIO
> such a good fit compared to rolling your own.
>
> I guess the alternative is that the i915 driver gets a handle from
> another platform_device on the MFD cell (a different one, say
> named panel-power or whatever) and pokes that register itself
> (using regmap in the same way that my suggested regulator
> code does basically).

The alternative would be to use the component framework with some
hardcoded names. Think board file distributed through all drivers with
no abstraction to get at handles for different pieces. We already have
that between i915 and snd-hda since the coordination to get
audio-over-dp/hdmi right is a bit tricky. We could have partially
reused e.g. power domains, but since the power domain/clock tree i915
manages isn't using generic infrastructure (yet, who knows) that
didn't look like a good idea.

power domains are actually a nice example for the tradeoffs: Currently
the i915 power domain code doen't implement hystersis. So for now we
just implement it ourselves for the one domain that really needs it.
But it might make sense to rebase the i915 power domain code to use
core power domains internally if we need hystersis in more places.

> I kind of like that because it makes the usecase all evident
> like I wanted. And arguably it's a better solution if the i915 driver
> want to be as self-contained as possible, using this pattern
> that the DRM drivers should take care of all sequencing business.
>
> The DRM driver can then also be used even if GPIO is configured
> out of the kernel.

So the main reason I thought reusing something common would make sense
here is that we wouldn't need to roll our own lookup structure stuff
and could reuse the logic provided by gpio for board files.
Longer-term we might even have switched to gpio abstraction internally
(maybe, not sure) since there's a bunch more board design floating
around (and sometimes shipping already like this one here) where the
panel line isn't controlled by i915 directly.

Also I just wanted to figure out whether use-cases like this
(irrespective of gpio, pwm, regulator or whatever) which are in some
sense worse than board files would be even acceptable. At least I
expect even more fun in the future since intel socs get integrated
ever tighter. Most of the interactions are handled by magic in some
power/clock firmware controllers (to make windows work), but there's
always small rifts in the seams like here where they didn't remap some
random register across the entire chip. Which the hw people generally
do, e.g. we can access the southbridge i2c engine through the same
mmio window as we access the gpu on the main die ;-)

Fundamentally we just don't have a nice description of the hw like in
dt with all the power domains/regulators and depencies described in an
abstract and unified way. It's a bad mess of hard-coded knowledge in
the driver, magic firmware to orchestrate cross-driver interactions
and the occasional explicit cross-driver communication in software
like this one where it all falls apart.

>> And yeah your code isn't any longer than the gpio version, but regulators
>> drag in all that conceptual stuff about voltage/delays and depencies that
>> imo just isn't controlled by the pmic. We've originally abused the panel
>> interface for all this and Thierry (imo rightfully suggested) that this
>> isn't a panel but it'd be better to expose the underlying stuff. Again imo
>> faking all that stuff since you think this looks like a regulator is imo a
>> worse lie than the exposing this as a gpio.
>
> Nah it's all lies :)
>
> I think you are right: if the DRM driver wants to control everything
> itself it should not use the regulator framework, neither the GPIO
> framework, it seems to be an established pattern to roll your own
> in these drivers so let's keep with that.
>
> I guess the use case is analogous to a monitor cable sticking out
> of a graphics card and sending these power-up/down sequences
> to that monitor in some magic way, and that is how DRM thinks
> of things. For what I know DRM frowns at the abstracted out
> backlight control used by framebuffers in drivers/video/backlight
> as well, and prefer to be on top of stuff, so then it should access
> stuff on as low level as possible.

Yeah backlight is similar, for most panels we need to frob it at
precise points or otherwise the source port or panel sink falls over.
So overall it's a judgement call, but here I've thought the benefits
of using the gpio lookup stuff (and maybe the benefits to the
subsystem for accepting yet another crazy use-case and learning some
interesting things) outweight the costs - also because the gpio
abstraction is a very dumb/simple one.

What I wanted is something that magically gives me a driver to flip a
line without asking questions or imposing anything, that seemed to fit
the gpio abstraction. Of course there's a full panel driver sitting on
top, but in a way that's one layer up. At least to me. And in that
upper layer the abstraction will most likely get in the way sooner or
later.

So summary:
- Reusing the dynamic gpio lookup stuff would be nice, and might be
interesting as a new crazy use-case (or maybe not). But not a
requirement since we have the component framework to handroll
something.
- More abstraction than "pls flick this line for me" is probably too
much since all the higher level logic must be in i915 because of the
already shipping design of the vbios tables and all that.
- I expect more of this kind of remote panel stuff and if we handroll
we'll probably end up reinventing gpio&pwm partially.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable

[Linus Walleij]

On Tue, Mar 24, 2015 at 11:53 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Mar 24, 2015 at 11:16 AM, Linus Walleij
> <linus.walleij@linaro.org> wrote:

> So summary:
> - Reusing the dynamic gpio lookup stuff would be nice, and might be
> interesting as a new crazy use-case (or maybe not). But not a
> requirement since we have the component framework to handroll
> something.

OK I guess you have me convinced, I will apply the patch from
Shobhit. If it turns out ugly we can always revert it. If you believe
in it, it's worth a try.

Also as you say else it will be reinvented, let's go this way as it
is likely the lesser of two evils.

Yours,
Linus Walleij
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable

[Daniel Vetter]

On Wed, Mar 25, 2015 at 01:24:01PM +0100, Linus Walleij wrote:
> On Tue, Mar 24, 2015 at 11:53 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Mar 24, 2015 at 11:16 AM, Linus Walleij
> > <linus.walleij@linaro.org> wrote:
> 
> > So summary:
> > - Reusing the dynamic gpio lookup stuff would be nice, and might be
> > interesting as a new crazy use-case (or maybe not). But not a
> > requirement since we have the component framework to handroll
> > something.
> 
> OK I guess you have me convinced, I will apply the patch from
> Shobhit. If it turns out ugly we can always revert it. If you believe
> in it, it's worth a try.
> 
> Also as you say else it will be reinvented, let's go this way as it
> is likely the lesser of two evils.

Thanks for reconsidering.

I quickly checked out your linux-gpio and it only has patch 2 to implement
the gpio. We also need patch 1 (but with the leak Thierry spotted fixed).
Should we drop Shobit's patch until that's done?

Wrt merging the 4.1 window on the drm side is pretty much closed so I
think I'll have to postpone the i915 side to 4.2 anyway. Luckily there's
no direct depencies because we look up everything dynamically, so patches
can go in in any order.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable

[Shobhit Kumar]

On 03/25/2015 06:43 PM, Daniel Vetter wrote:
> On Wed, Mar 25, 2015 at 01:24:01PM +0100, Linus Walleij wrote:
>> On Tue, Mar 24, 2015 at 11:53 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> On Tue, Mar 24, 2015 at 11:16 AM, Linus Walleij
>>> <linus.walleij@linaro.org> wrote:
>>
>>> So summary:
>>> - Reusing the dynamic gpio lookup stuff would be nice, and might be
>>> interesting as a new crazy use-case (or maybe not). But not a
>>> requirement since we have the component framework to handroll
>>> something.
>>
>> OK I guess you have me convinced, I will apply the patch from
>> Shobhit. If it turns out ugly we can always revert it. If you believe
>> in it, it's worth a try.
>>
>> Also as you say else it will be reinvented, let's go this way as it
>> is likely the lesser of two evils.
> 
> Thanks for reconsidering.
> 
> I quickly checked out your linux-gpio and it only has patch 2 to implement
> the gpio. We also need patch 1 (but with the leak Thierry spotted fixed).
> Should we drop Shobit's patch until that's done?

Will work on this.

Regards
Shobhit

> 
> Wrt merging the 4.1 window on the drm side is pretty much closed so I
> think I'll have to postpone the i915 side to 4.2 anyway. Luckily there's
> no direct depencies because we look up everything dynamically, so patches
> can go in in any order.
> 
> Cheers, Daniel
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v5 2/9] gpio/crystalcove: Add additional GPIO for Panel control

[Linus Walleij]

On Thu, Mar 12, 2015 at 5:31 PM, Shobhit Kumar <shobhit.kumar@intel.com> wrote:

> Export PANEL_EN/DISABLE (offset 0x52) as additional GPIO. Needed
> by display driver to enable the DSI panel on BYT platform where
> the Panel EN/Disable control is routed thorugh CRC PMIC
>
> 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>

OK applied *this* version of the patch...

Yours,
Linus Walleij
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC v5 1/9] drivers/mfd: Add lookup table for Panel Control as GPIO signal

[Linus Walleij]

On Thu, Mar 12, 2015 at 5:31 PM, Shobhit Kumar <shobhit.kumar@intel.com> 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
>
> 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>

I have changed my mind.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Lee, please apply this.

Yours,
Linus Walleij
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable

[Linus Walleij]

On Wed, Mar 25, 2015 at 2:13 PM, Daniel Vetter <daniel@ffwll.ch> wrote:

> I quickly checked out your linux-gpio and it only has patch 2 to implement
> the gpio. We also need patch 1 (but with the leak Thierry spotted fixed).
> Should we drop Shobit's patch until that's done?

Nah I trust that Lee will merge it to MFD in due time, there is no
compile-time dependence anyway. I Reviewed/ACKed the patch
anyway, probably should have pointed out to fix the leak
pointed out by Thierry but I count on you folks to fix it.

Can I add you ACK tag on the GPIO patch?

Yours,
Linus Wallei
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm/i915: Use the CRC gpio for panel enable/disable

[Daniel Vetter]

On Wed, Mar 25, 2015 at 03:55:43PM +0100, Linus Walleij wrote:
> On Wed, Mar 25, 2015 at 2:13 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > I quickly checked out your linux-gpio and it only has patch 2 to implement
> > the gpio. We also need patch 1 (but with the leak Thierry spotted fixed).
> > Should we drop Shobit's patch until that's done?
> 
> Nah I trust that Lee will merge it to MFD in due time, there is no
> compile-time dependence anyway. I Reviewed/ACKed the patch
> anyway, probably should have pointed out to fix the leak
> pointed out by Thierry but I count on you folks to fix it.
> 
> Can I add you ACK tag on the GPIO patch?

Sure. And yes Shobit is already working on the leak fix, so either it'll
be a fixup or new patch version. Shobit, to make sure that your patch will
apply please base the series on top of linux-next and not
drm-intel-nightly for the next iteration.

https://www.kernel.org/doc/man-pages/linux-next.html

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drivers/pwm: Add helper to configure pwm using clock divisor and duty percent

[Shobhit Kumar]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/24/2015 01:53 PM, Thierry Reding wrote:
> On Fri, Mar 13, 2015 at 07:28:02PM +0530, Shobhit Kumar wrote:
>> Some chips instead of using period_ns and duty_ns can be
>> configured using the clock divisor and duty percent. Adds an
>> alternative configuration method for such chips
> 
> I don't see a need to introduce this alternative configuration 
> mechanism. Most, of not all, of the other drivers program a clock 
> divisor and some percentage of the duty cycle as well and it should
> be easy to convert to that internally from the period and
> duty_cycle parameters that you get in ->config().

Perhaps. Probably I misunderstood but as per Documentation/pwm.txt, it
is suggested that rather than calculating in the driver, we can add
additional helpers. So I tried doing just that. And it also means that
the consumer(which is directly aware of the percent it wants) has to
do the calculation and pass as ns values and we internally again
convert back to percentage ?

> 
> Adding an alternative means of configuring the PWM also means that
> every user driver now potentially needs to support both the
> traditional and the alternative way because PWM providers may not
> implement both.

I just assumed either or implementation should suffice. Even in my
implementation the error checks assumes either of the two should be
available else to fail the pwmchip_add

Regards
Shobhit

> 
> Thierry
> 
> 
> 
> _______________________________________________ Intel-gfx mailing
> list Intel-gfx@lists.freedesktop.org 
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJVG5AiAAoJEHuQFv2//5Kq9LYIAKlRfsm4hnDFTlOmhBs5hPhT
2xOx03Vwf5V0U1FisNFr1ZhK9CuJJGGUq4zTvVDmokHNHEtQk4o751fxxY4eVE+a
quw7o6BhLcQkwKdHvcHzLYHR+Szs5h60bR8qtAg1UhmGMpPkWiiPytIVOiKHjSfg
yxHyAEqjxn9Q07yuhj0g2U/nqvNwODQ72cQXoI8nKNYJsRzNVhlJh8nZ1CxyYqBZ
wubMJvjaM7jyFQZf3YDb7zaW3CXqkkxLhJpb/iL3grxQICO6DCnAbqnIagPgjq0H
dNetFOAQfmL4i4+gonnSWEW+UBmszeDNlVKTiTH5DNuNigju+HEDGWZaF2e1hKY=
=r1M/
-----END PGP SIGNATURE-----
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drivers/pwm: Add helper to configure pwm using clock divisor and duty percent

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2390 bytes --]

On Wed, Apr 01, 2015 at 11:58:50AM +0530, Shobhit Kumar wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 03/24/2015 01:53 PM, Thierry Reding wrote:
> > On Fri, Mar 13, 2015 at 07:28:02PM +0530, Shobhit Kumar wrote:
> >> Some chips instead of using period_ns and duty_ns can be
> >> configured using the clock divisor and duty percent. Adds an
> >> alternative configuration method for such chips
> > 
> > I don't see a need to introduce this alternative configuration 
> > mechanism. Most, of not all, of the other drivers program a clock 
> > divisor and some percentage of the duty cycle as well and it should
> > be easy to convert to that internally from the period and
> > duty_cycle parameters that you get in ->config().
> 
> Perhaps. Probably I misunderstood but as per Documentation/pwm.txt, it
> is suggested that rather than calculating in the driver, we can add
> additional helpers. So I tried doing just that. And it also means that
> the consumer(which is directly aware of the percent it wants) has to
> do the calculation and pass as ns values and we internally again
> convert back to percentage ?

Yes. The interface assumes that you'll pass in absolute values for the
period and duty cycle. Existing drivers, such as pwm-backlight, already
convert a percentage or other internal representation to these absolute
values. If your driver internally works with percent you can easily
convert to that from the absolute values.

The documentation only makes a suggestion. I think it'd be fine if you
kept this conversion internal to the driver. We can turn it into a more
generic helper if a second driver appears that needs the same
conversion.

> > Adding an alternative means of configuring the PWM also means that
> > every user driver now potentially needs to support both the
> > traditional and the alternative way because PWM providers may not
> > implement both.
> 
> I just assumed either or implementation should suffice. Even in my
> implementation the error checks assumes either of the two should be
> available else to fail the pwmchip_add

Your implementation requires that users call either pwm_config() or
pwm_config_alternate(). PWM drivers may only have to implement either
callback, but users will be required to support both (or otherwise
only work with a subset of PWM drivers).

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 4/9] drivers/pwm: Add helper to configure pwm using clock divisor and duty percent

[Shobhit Kumar]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/10/2015 01:59 PM, Thierry Reding wrote:
> On Wed, Apr 01, 2015 at 11:58:50AM +0530, Shobhit Kumar wrote:
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>> 
>> On 03/24/2015 01:53 PM, Thierry Reding wrote:
>>> On Fri, Mar 13, 2015 at 07:28:02PM +0530, Shobhit Kumar wrote:
>>>> Some chips instead of using period_ns and duty_ns can be 
>>>> configured using the clock divisor and duty percent. Adds an 
>>>> alternative configuration method for such chips
>>> 
>>> I don't see a need to introduce this alternative configuration
>>>  mechanism. Most, of not all, of the other drivers program a
>>> clock divisor and some percentage of the duty cycle as well and
>>> it should be easy to convert to that internally from the period
>>> and duty_cycle parameters that you get in ->config().
>> 
>> Perhaps. Probably I misunderstood but as per
>> Documentation/pwm.txt, it is suggested that rather than
>> calculating in the driver, we can add additional helpers. So I
>> tried doing just that. And it also means that the consumer(which
>> is directly aware of the percent it wants) has to do the
>> calculation and pass as ns values and we internally again convert
>> back to percentage ?
> 
> Yes. The interface assumes that you'll pass in absolute values for
> the period and duty cycle. Existing drivers, such as pwm-backlight,
> already convert a percentage or other internal representation to
> these absolute values. If your driver internally works with percent
> you can easily convert to that from the absolute values.
> 
> The documentation only makes a suggestion. I think it'd be fine if
> you kept this conversion internal to the driver. We can turn it
> into a more generic helper if a second driver appears that needs
> the same conversion.

Okay, will change driver implementation and avoid this patch

> 
>>> Adding an alternative means of configuring the PWM also means
>>> that every user driver now potentially needs to support both
>>> the traditional and the alternative way because PWM providers
>>> may not implement both.
>> 
>> I just assumed either or implementation should suffice. Even in
>> my implementation the error checks assumes either of the two
>> should be available else to fail the pwmchip_add
> 
> Your implementation requires that users call either pwm_config()
> or pwm_config_alternate(). PWM drivers may only have to implement
> either callback, but users will be required to support both (or
> otherwise only work with a subset of PWM drivers).

Yeah, I overlooked this. Will push a new patch for the driver.

Regards
Shobhit

> 
> Thierry
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJVK3gXAAoJEHuQFv2//5KqRhIIAMKvvSuJ3yyiPrBULOk6PyZg
AyNpICHg/pwhnjdns45eui1YnWb6Hrasbs5+UZRxlUAubs/+CDa1ZvGvtAZZQCO0
g8YO0EiafdGUg8KMif2qblJZf0oJFWs1j8sUQaarA7Uh2/1m4elvijQ39J30yzCt
+4N2JQ3Nazx2KWS5P8Wo9i2Km733vz7p8nY5lqXlstHer1x4QoaCz6utNPMgcUE+
N5wCUpOzEzqM4Lle63R2UO/uCfC+169Q+bZ2r9a1UxSeLhA+fhkZWgusaUeqi1UL
kIy4YSyelTNYIBa8dufp+IQL1w2cSbZ9JoPj7Zc7agTYqbhOuLhbM1wC9DWMWW0=
=1wV2
-----END PGP SIGNATURE-----
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx