| Message ID | 1424261894-24983-3-git-send-email-shobhit.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 18, 2015 at 9:18 PM, 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. > > 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 | 35 +++++++++++++++++++++++++++++++++-- > drivers/gpu/drm/i915/intel_dsi.h | 11 +++++++++++ > 2 files changed, 44 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > index c8c8b24..6b56ca0 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.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 needed */ > + if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) > + gpio_set_value_cansleep( > + intel_dsi->crc_base + GPIO_PANEL_EN, 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 needed */ > + if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) > + gpio_set_value_cansleep( > + intel_dsi->crc_base + GPIO_PANEL_EN, 0); > } > > static bool intel_dsi_get_hw_state(struct intel_encoder *encoder, > @@ -977,6 +988,12 @@ static const struct drm_connector_funcs intel_dsi_connector_funcs = { > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > }; > > +static int match_gpio_chip_by_label(struct gpio_chip *chip, > + void *data) > +{ > + return !strcmp(chip->label, data); > +} > + > void intel_dsi_init(struct drm_device *dev) > { > struct intel_dsi *intel_dsi; > @@ -1070,6 +1087,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. Store the GPIO base > + */ > + if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) { > + struct gpio_chip *gpio; > + gpio = gpiochip_find(GPIO_CHIP_NAME, match_gpio_chip_by_label); > + if (!gpio) { > + printk("Failed to find crc gpio chip\n"); > + intel_dsi->crc_base = 0; > + } else > + intel_dsi->crc_base = gpio->base; > + } This looks terribly wrong - you lookup a particular GPIO chip by name, use a forged GPIO number without even requesting it, and are using deprecated functions. Please use the GPIO descriptor interface when adding new GPIO code, see Documentation/gpio/consumer.h. The integer-based GPIO interface is considered deprecated and should not be used for new code. Using gpiod_* functions will prevent you from doing the other mistakes I mentioned, since it forces you to request your GPIO properly and will not allow you to forge a descriptor. See also Documentation/gpio/board.txt for how you can associate GPIOs to your device's functions depending on which firmware your platform is using.
On 2/26/2015 3:43 PM, Alexandre Courbot wrote: > On Wed, Feb 18, 2015 at 9:18 PM, 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. >> >> 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 | 35 +++++++++++++++++++++++++++++++++-- >> drivers/gpu/drm/i915/intel_dsi.h | 11 +++++++++++ >> 2 files changed, 44 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c >> index c8c8b24..6b56ca0 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.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 needed */ >> + if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) >> + gpio_set_value_cansleep( >> + intel_dsi->crc_base + GPIO_PANEL_EN, 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 needed */ >> + if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) >> + gpio_set_value_cansleep( >> + intel_dsi->crc_base + GPIO_PANEL_EN, 0); >> } >> >> static bool intel_dsi_get_hw_state(struct intel_encoder *encoder, >> @@ -977,6 +988,12 @@ static const struct drm_connector_funcs intel_dsi_connector_funcs = { >> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >> }; >> >> +static int match_gpio_chip_by_label(struct gpio_chip *chip, >> + void *data) >> +{ >> + return !strcmp(chip->label, data); >> +} >> + >> void intel_dsi_init(struct drm_device *dev) >> { >> struct intel_dsi *intel_dsi; >> @@ -1070,6 +1087,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. Store the GPIO base >> + */ >> + if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) { >> + struct gpio_chip *gpio; >> + gpio = gpiochip_find(GPIO_CHIP_NAME, match_gpio_chip_by_label); >> + if (!gpio) { >> + printk("Failed to find crc gpio chip\n"); >> + intel_dsi->crc_base = 0; >> + } else >> + intel_dsi->crc_base = gpio->base; >> + } > > This looks terribly wrong - you lookup a particular GPIO chip by name, > use a forged GPIO number without even requesting it, and are using > deprecated functions. > > Please use the GPIO descriptor interface when adding new GPIO code, > see Documentation/gpio/consumer.h. The integer-based GPIO interface is > considered deprecated and should not be used for new code. > > Using gpiod_* functions will prevent you from doing the other mistakes > I mentioned, since it forces you to request your GPIO properly and > will not allow you to forge a descriptor. > > See also Documentation/gpio/board.txt for how you can associate GPIOs > to your device's functions depending on which firmware your platform > is using. Thanks for confirming the bad feeling that I already had with this patch as I knew I was not requesting gpio. Was already starting to look into the directions you pointed out. Will send corrected implementation soon as suggested. Regards Shobhit
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index c8c8b24..6b56ca0 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.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 needed */ + if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) + gpio_set_value_cansleep( + intel_dsi->crc_base + GPIO_PANEL_EN, 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 needed */ + if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) + gpio_set_value_cansleep( + intel_dsi->crc_base + GPIO_PANEL_EN, 0); } static bool intel_dsi_get_hw_state(struct intel_encoder *encoder, @@ -977,6 +988,12 @@ static const struct drm_connector_funcs intel_dsi_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, }; +static int match_gpio_chip_by_label(struct gpio_chip *chip, + void *data) +{ + return !strcmp(chip->label, data); +} + void intel_dsi_init(struct drm_device *dev) { struct intel_dsi *intel_dsi; @@ -1070,6 +1087,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. Store the GPIO base + */ + if (dev_priv->vbt.dsi.config->pwm_blc == PPS_BLC_PMIC) { + struct gpio_chip *gpio; + gpio = gpiochip_find(GPIO_CHIP_NAME, match_gpio_chip_by_label); + if (!gpio) { + printk("Failed to find crc gpio chip\n"); + intel_dsi->crc_base = 0; + } else + intel_dsi->crc_base = gpio->base; + } + 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..31074a7 100644 --- a/drivers/gpu/drm/i915/intel_dsi.h +++ b/drivers/gpu/drm/i915/intel_dsi.h @@ -29,6 +29,14 @@ #include <drm/drm_mipi_dsi.h> #include "intel_drv.h" +/* CRC PMIC GPIO Access */ +#define GPIO_CHIP_NAME "gpio_crystalcove" +#define GPIO_BACKLIGHT_EN 94 +#define GPIO_PANEL_EN 95 + +#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 @@ -44,6 +52,9 @@ struct intel_dsi { struct intel_connector *attached_connector; + /* base for GPIOs from CRC chip */ + int crc_base; + /* bit mask of ports being driven */ u16 ports;
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. 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 | 35 +++++++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_dsi.h | 11 +++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-)