* [PATCH 0/2] Crystal Cove PMIC based Panel Control @ 2015-02-18 12:18 Shobhit Kumar 2015-02-18 12:18 ` [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO Shobhit Kumar 2015-02-18 12:18 ` [PATCH 2/2] drm/i915: Use the CRC gpio_chip for panel enable/disable Shobhit Kumar 0 siblings, 2 replies; 15+ messages in thread From: Shobhit Kumar @ 2015-02-18 12:18 UTC (permalink / raw) To: intel-gfx Cc: gnurou, Jani Nikula, Shobhit Kumar, linus.walleij, thierry.reding, Daniel Vetter This is rework of the same functionaility as discussed in http://lists.freedesktop.org/archives/intel-gfx/2015-January/058756.html and then rework needed summarized in - http://lists.freedesktop.org/archives/intel-gfx/2015-February/059458.html Basically the suggestion was to go with using GPIO framework for panel enable and disbale and use PWM frameowrk for backlight control. These patches are only for panel and backlight enable/disable signals as GPOI. PWM based backlight control is WIP Regards Shobhit Shobhit Kumar (2): gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO drm/i915: Use the CRC gpio_chip for panel enable/disable drivers/gpio/gpio-crystalcove.c | 8 +++++++- drivers/gpu/drm/i915/intel_dsi.c | 35 +++++++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_dsi.h | 11 +++++++++++ 3 files changed, 51 insertions(+), 3 deletions(-) -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO 2015-02-18 12:18 [PATCH 0/2] Crystal Cove PMIC based Panel Control Shobhit Kumar @ 2015-02-18 12:18 ` Shobhit Kumar 2015-03-06 11:04 ` Linus Walleij 2015-03-25 12:27 ` Linus Walleij 2015-02-18 12:18 ` [PATCH 2/2] drm/i915: Use the CRC gpio_chip for panel enable/disable Shobhit Kumar 1 sibling, 2 replies; 15+ messages in thread From: Shobhit Kumar @ 2015-02-18 12:18 UTC (permalink / raw) To: intel-gfx Cc: gnurou, Jani Nikula, Shobhit Kumar, linus.walleij, thierry.reding, Daniel Vetter Export Panel BACKLIGHT_EN(offset 0x51) and PANEL_EN(offset 0x52) as two additional GPIOs. Needed by display driver to enable the DSI panel on BYT platform where the Panel EN/Disable and Backlight control are routed thorugh CRC PMIC 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 | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpio/gpio-crystalcove.c b/drivers/gpio/gpio-crystalcove.c index 3d9e08f..9e94dc64 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 96 #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 0x51 #define CTLI_INTCNT_DIS (0) #define CTLI_INTCNT_NE (1 << 1) @@ -93,6 +94,11 @@ static inline int to_reg(int gpio, enum ctrl_register reg_type) { int reg; + if (gpio >= 94) { + reg = GPIOPANELCTL + gpio - 94; + return reg; + } + if (reg_type == CTRL_IN) { if (gpio < 8) reg = GPIO0P0CTLI; -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO 2015-02-18 12:18 ` [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO Shobhit Kumar @ 2015-03-06 11:04 ` Linus Walleij 2015-03-06 16:23 ` Kumar, Shobhit 2015-03-25 12:27 ` Linus Walleij 1 sibling, 1 reply; 15+ messages in thread From: Linus Walleij @ 2015-03-06 11:04 UTC (permalink / raw) To: Shobhit Kumar, Zhu, Lejun, Aaron Lu Cc: Jani Nikula, Daniel Vetter, intel-gfx, Thierry Reding, Alexandre Courbot On Wed, Feb 18, 2015 at 1:18 PM, Shobhit Kumar <shobhit.kumar@intel.com> wrote: > Export Panel BACKLIGHT_EN(offset 0x51) and PANEL_EN(offset 0x52) as two > additional GPIOs. Needed by display driver to enable the DSI panel on > BYT platform where the Panel EN/Disable and Backlight control are > routed thorugh CRC PMIC > > 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> This seems very unintuitive. I have a hard time believeing this: > +#define GPIOPANELCTL 0x51 A special GPIO only designated to panel control? That is totally counter to the *meaning* of "general purpose input/output". It is not general purpose at all, it is special-purpose. If the data sheet says a special-purpose pin is a "GPIO" when it isn't the map of the world (the datasheet terminology) is wrong, not the world, the usage is dedicated for the panel. Look at the usecase, not the name. Obviously that line is only for controlling the backlight, and you should spin a new child device from the Inte SoC PMIC MFD hub and put a driver for it in drivers/video/backlight or wherever this goes. Lee: do you agree? Yours, Linus Walleij _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO 2015-03-06 11:04 ` Linus Walleij @ 2015-03-06 16:23 ` Kumar, Shobhit 2015-03-09 17:15 ` Linus Walleij 0 siblings, 1 reply; 15+ messages in thread From: Kumar, Shobhit @ 2015-03-06 16:23 UTC (permalink / raw) To: linus.walleij Cc: Lu, Aaron, gnurou, Nikula, Jani, intel-gfx, thierry.reding, lejun.zhu, Vetter, Daniel On Fri, 2015-03-06 at 12:04 +0100, Linus Walleij wrote: > On Wed, Feb 18, 2015 at 1:18 PM, Shobhit Kumar <shobhit.kumar@intel.com> wrote: > > > Export Panel BACKLIGHT_EN(offset 0x51) and PANEL_EN(offset 0x52) as two > > additional GPIOs. Needed by display driver to enable the DSI panel on > > BYT platform where the Panel EN/Disable and Backlight control are > > routed thorugh CRC PMIC > > > > 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> > > This seems very unintuitive. I have a hard time believeing this: > > > +#define GPIOPANELCTL 0x51 > > A special GPIO only designated to panel control? That is totally > counter to the *meaning* of "general purpose input/output". > It is not general purpose at all, it is special-purpose. > > If the data sheet says a special-purpose pin is a "GPIO" when > it isn't the map of the world (the datasheet terminology) is wrong, > not the world, the usage is dedicated for the panel. Look > at the usecase, not the name. > > Obviously that line is only for controlling the backlight, and you > should spin a new child device from the Inte SoC PMIC MFD hub > and put a driver for it in drivers/video/backlight or wherever this > goes. There are actually two lines for Panel Power control and Backlight enable/disable. I have already moved towards adding a new Cell device for PWM child device and a new pwm driver for the same. That will take care of backlight thingy. But for the Panel power control, I am at loss for how best to program that. It is not strictly a GPIO but behaves similarly and thought of simply extending existing gpio-crystalcove. My next implementation(not posted yet) is having that exported by adding a lookup table in PMIC MFD probe routine. Regards Shobhit > > Lee: do you agree? > > Yours, > Linus Walleij _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO 2015-03-06 16:23 ` Kumar, Shobhit @ 2015-03-09 17:15 ` Linus Walleij 2015-03-12 15:06 ` Kumar, Shobhit 0 siblings, 1 reply; 15+ messages in thread From: Linus Walleij @ 2015-03-09 17:15 UTC (permalink / raw) To: Kumar, Shobhit Cc: Lu, Aaron, gnurou, Nikula, Jani, intel-gfx, thierry.reding, lejun.zhu, Vetter, Daniel On Fri, Mar 6, 2015 at 5:23 PM, Kumar, Shobhit <shobhit.kumar@intel.com> wrote: > There are actually two lines for Panel Power control and Backlight > enable/disable. I have already moved towards adding a new Cell device > for PWM child device and a new pwm driver for the same. That will take > care of backlight thingy. But for the Panel power control, I am at loss > for how best to program that. Isn't it just a very simple regulator (just on/off of fixed voltage) cell? Yours, Linus Walleij _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO 2015-03-09 17:15 ` Linus Walleij @ 2015-03-12 15:06 ` Kumar, Shobhit 2015-03-18 11:50 ` Linus Walleij 0 siblings, 1 reply; 15+ messages in thread From: Kumar, Shobhit @ 2015-03-12 15:06 UTC (permalink / raw) To: linus.walleij Cc: Lu, Aaron, gnurou, Nikula, Jani, intel-gfx, thierry.reding, Vetter, Daniel, lejun.zhu On Mon, 2015-03-09 at 18:15 +0100, Linus Walleij wrote: > On Fri, Mar 6, 2015 at 5:23 PM, Kumar, Shobhit <shobhit.kumar@intel.com> wrote: > > > There are actually two lines for Panel Power control and Backlight > > enable/disable. I have already moved towards adding a new Cell device > > for PWM child device and a new pwm driver for the same. That will take > > care of backlight thingy. But for the Panel power control, I am at loss > > for how best to program that. > > Isn't it just a very simple regulator (just on/off of fixed voltage) cell? > It is just behaving as an output GPIO line and for that reason I feel that rather than adding a new regulator driver, it is better to augment the gpio-crystalcove driver. I am planning to export as GPIO from PMIC MFD driver by way of lookup table and use in display side. Will send RFC patch soon. Backlight control as I said is a new pwm cell. Regards Shobhit > Yours, > Linus Walleij _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO 2015-03-12 15:06 ` Kumar, Shobhit @ 2015-03-18 11:50 ` Linus Walleij 2015-03-18 13:27 ` Daniel Vetter 0 siblings, 1 reply; 15+ messages in thread From: Linus Walleij @ 2015-03-18 11:50 UTC (permalink / raw) To: Kumar, Shobhit Cc: Lu, Aaron, gnurou, Nikula, Jani, intel-gfx, thierry.reding, Vetter, Daniel, lejun.zhu On Thu, Mar 12, 2015 at 4:06 PM, Kumar, Shobhit <shobhit.kumar@intel.com> wrote: > On Mon, 2015-03-09 at 18:15 +0100, Linus Walleij wrote: >> On Fri, Mar 6, 2015 at 5:23 PM, Kumar, Shobhit <shobhit.kumar@intel.com> wrote: >> >> > There are actually two lines for Panel Power control and Backlight >> > enable/disable. I have already moved towards adding a new Cell device >> > for PWM child device and a new pwm driver for the same. That will take >> > care of backlight thingy. But for the Panel power control, I am at loss >> > for how best to program that. >> >> Isn't it just a very simple regulator (just on/off of fixed voltage) cell? >> > > It is just behaving as an output GPIO line and for that reason I feel > that rather than adding a new regulator driver, I disagree. GPIO reads "general purpose input/output". This is not a GPIO, this is a special purpose thing and IMO it should be modeled directly as a regulator. Yours, Linus Walleij _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO 2015-03-18 11:50 ` Linus Walleij @ 2015-03-18 13:27 ` Daniel Vetter 2015-03-24 9:50 ` Linus Walleij 0 siblings, 1 reply; 15+ messages in thread From: Daniel Vetter @ 2015-03-18 13:27 UTC (permalink / raw) To: Linus Walleij Cc: gnurou, Lu, Aaron, Nikula, Jani, Kumar, Shobhit, intel-gfx, thierry.reding, lejun.zhu, Vetter, Daniel On Wed, Mar 18, 2015 at 12:50:51PM +0100, Linus Walleij wrote: > On Thu, Mar 12, 2015 at 4:06 PM, Kumar, Shobhit <shobhit.kumar@intel.com> wrote: > > On Mon, 2015-03-09 at 18:15 +0100, Linus Walleij wrote: > >> On Fri, Mar 6, 2015 at 5:23 PM, Kumar, Shobhit <shobhit.kumar@intel.com> wrote: > >> > >> > There are actually two lines for Panel Power control and Backlight > >> > enable/disable. I have already moved towards adding a new Cell device > >> > for PWM child device and a new pwm driver for the same. That will take > >> > care of backlight thingy. But for the Panel power control, I am at loss > >> > for how best to program that. > >> > >> Isn't it just a very simple regulator (just on/off of fixed voltage) cell? > >> > > > > It is just behaving as an output GPIO line and for that reason I feel > > that rather than adding a new regulator driver, > > I disagree. GPIO reads "general purpose input/output". > > This is not a GPIO, this is a special purpose thing and IMO > it should be modeled directly as a regulator. The idea behind reusing gpio was that we can reuse the dynamic lookup stuff so that i915 can find the gpio. And it is a gpio (there's lots more on that chip), it's just that intel tends to hand you recommended board layouts. And there's different ones so only really i915.ko can tell you if it's indeed used to control the panel or not (after consulting a bunch of vbios tables). Regulator seems way too overkill for this, especially since it tends to be a lie on systems where the panel is not connected to that gpio line. And afaiui the point of the regulator subsystem is that you just ask for the regulator for your IP block and then magically get handed the right bit (or a dummy one). This is very much not the case, hw descriptions on x86 in this area are a kludge worse than board tables since we can't even fix up what the bios hands us anywhere. So for me plan B is to just handroll our own thing using the component framework when reusing gpios isn't acceptable. -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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO 2015-03-18 13:27 ` Daniel Vetter @ 2015-03-24 9:50 ` Linus Walleij 0 siblings, 0 replies; 15+ messages in thread From: Linus Walleij @ 2015-03-24 9:50 UTC (permalink / raw) To: Daniel Vetter, Mark Brown, Kumar, Shobhit Cc: Lu, Aaron, gnurou, Nikula, Jani, intel-gfx, thierry.reding, lejun.zhu, Vetter, Daniel On Wed, Mar 18, 2015 at 2:27 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Mar 18, 2015 at 12:50:51PM +0100, Linus Walleij wrote: >> This is not a GPIO, this is a special purpose thing and IMO >> it should be modeled directly as a regulator. > > The idea behind reusing gpio was that we can reuse the dynamic lookup > stuff so that i915 can find the gpio. (Fixed) regulators also have dynamic lookups so they provide the exact same mechanism of associating themselves to devices. > And it is a gpio (there's lots more > on that chip), Well the other GPIO registers seem to have 8 GPIOs each, so one register control 8 random lines. In this case they obviously put a single bit to control a line out of the PMIC. The register was intended to that purpose only, the register is named PANEL_EN/DISABLE in the refman (right?) and I bet a million to one that the pin on the PMIC and the rail connected to it is also named PANEL_EN/DISABLE or similar. So the hardware engineers had a special purpose in mind for this. Not general purpose (GPIO). > it's just that intel tends to hand you recommended board > layouts. And there's different ones so only really i915.ko can tell you if > it's indeed used to control the panel or not (after consulting a bunch of > vbios tables). What about the schematic or PMIC pinout. I suspect it tells you a lot about the intended usage by calling that pin PANEL_EN or so. I'm not saying other usecases are impossible, all Rube Goldberg machine variants are possible, but are other use cases really realistic amongst implementers? > Regulator seems way too overkill for this, especially since it tends to be > a lie on systems where the panel is not connected to that gpio line. And > afaiui the point of the regulator subsystem is that you just ask for the > regulator for your IP block and then magically get handed the right bit > (or a dummy one). This is very much not the case, hw descriptions on x86 > in this area are a kludge worse than board tables since we can't even fix > up what the bios hands us anywhere. Depends on the intended use case I would say. Regulator isn't overkill if it fits the use case better. And I think you have a power-on delay, typically so that the panel driver cannot immediately start transmitting data to the panel after driving this line high. Probably the panel needs to settle first. This is referred to as a power-on-delay and the regulator framework can handle it for you. I strongly suspect that you would be planting some delay code in the site enabling the GPIO instead maybe already now or when later running into this while enabling suspend/resume or similar power saving mechanisms. That is basically reimplementing some helpful stuff from the regulator framework. > So for me plan B is to just handroll our own thing using the component > framework when reusing gpios isn't acceptable. Let's just figure out if GPIO is the best fit first, or if you actually could be helped by fixed regulators. Yours, Linus Walleij _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO 2015-02-18 12:18 ` [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO Shobhit Kumar 2015-03-06 11:04 ` Linus Walleij @ 2015-03-25 12:27 ` Linus Walleij 2015-03-25 14:15 ` Kumar, Shobhit 1 sibling, 1 reply; 15+ messages in thread From: Linus Walleij @ 2015-03-25 12:27 UTC (permalink / raw) To: Shobhit Kumar Cc: Jani Nikula, Daniel Vetter, intel-gfx, Thierry Reding, Alexandre Courbot On Wed, Feb 18, 2015 at 1:18 PM, Shobhit Kumar <shobhit.kumar@intel.com> wrote: > Export Panel BACKLIGHT_EN(offset 0x51) and PANEL_EN(offset 0x52) as two > additional GPIOs. Needed by display driver to enable the DSI panel on > BYT platform where the Panel EN/Disable and Backlight control are > routed thorugh CRC PMIC > > 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> Patch applied, after Daniel convinced me that this is the lesser of several possible evils to the kernel code base. Yours, Linus Walleij _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO 2015-03-25 12:27 ` Linus Walleij @ 2015-03-25 14:15 ` Kumar, Shobhit 2015-03-25 14:39 ` Linus Walleij 0 siblings, 1 reply; 15+ messages in thread From: Kumar, Shobhit @ 2015-03-25 14:15 UTC (permalink / raw) To: linus.walleij Cc: Nikula, Jani, Vetter, Daniel, intel-gfx, thierry.reding, gnurou On Wed, 2015-03-25 at 13:27 +0100, Linus Walleij wrote: > On Wed, Feb 18, 2015 at 1:18 PM, Shobhit Kumar <shobhit.kumar@intel.com> wrote: > > > Export Panel BACKLIGHT_EN(offset 0x51) and PANEL_EN(offset 0x52) as two > > additional GPIOs. Needed by display driver to enable the DSI panel on > > BYT platform where the Panel EN/Disable and Backlight control are > > routed thorugh CRC PMIC > > > > 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> > > Patch applied, after Daniel convinced me that this is the lesser > of several possible evils to the kernel code base. > Thanks, but I hope you pulled in the patch from the latest series which only exports one of them not both. Regards Shobhit _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO 2015-03-25 14:15 ` Kumar, Shobhit @ 2015-03-25 14:39 ` Linus Walleij 0 siblings, 0 replies; 15+ messages in thread From: Linus Walleij @ 2015-03-25 14:39 UTC (permalink / raw) To: Kumar, Shobhit Cc: Nikula, Jani, Vetter, Daniel, intel-gfx, thierry.reding, gnurou On Wed, Mar 25, 2015 at 3:15 PM, Kumar, Shobhit <shobhit.kumar@intel.com> wrote: > On Wed, 2015-03-25 at 13:27 +0100, Linus Walleij wrote: >> On Wed, Feb 18, 2015 at 1:18 PM, Shobhit Kumar <shobhit.kumar@intel.com> wrote: >> >> > Export Panel BACKLIGHT_EN(offset 0x51) and PANEL_EN(offset 0x52) as two >> > additional GPIOs. Needed by display driver to enable the DSI panel on >> > BYT platform where the Panel EN/Disable and Backlight control are >> > routed thorugh CRC PMIC >> > >> > 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> >> >> Patch applied, after Daniel convinced me that this is the lesser >> of several possible evils to the kernel code base. >> > > Thanks, but I hope you pulled in the patch from the latest series which > only exports one of them not both. Ummm I'll double-check. Yours, Linus Walleij _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] drm/i915: Use the CRC gpio_chip for panel enable/disable 2015-02-18 12:18 [PATCH 0/2] Crystal Cove PMIC based Panel Control Shobhit Kumar 2015-02-18 12:18 ` [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO Shobhit Kumar @ 2015-02-18 12:18 ` Shobhit Kumar 2015-02-26 10:13 ` Alexandre Courbot 1 sibling, 1 reply; 15+ messages in thread From: Shobhit Kumar @ 2015-02-18 12:18 UTC (permalink / raw) To: intel-gfx Cc: gnurou, Jani Nikula, Shobhit Kumar, linus.walleij, thierry.reding, Daniel Vetter 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; + } + 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; -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/i915: Use the CRC gpio_chip for panel enable/disable 2015-02-18 12:18 ` [PATCH 2/2] drm/i915: Use the CRC gpio_chip for panel enable/disable Shobhit Kumar @ 2015-02-26 10:13 ` Alexandre Courbot 2015-03-01 13:24 ` Kumar, Shobhit 0 siblings, 1 reply; 15+ messages in thread From: Alexandre Courbot @ 2015-02-26 10:13 UTC (permalink / raw) To: Shobhit Kumar Cc: Jani Nikula, Daniel Vetter, intel-gfx, Thierry Reding, Linus Walleij 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. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/i915: Use the CRC gpio_chip for panel enable/disable 2015-02-26 10:13 ` Alexandre Courbot @ 2015-03-01 13:24 ` Kumar, Shobhit 0 siblings, 0 replies; 15+ messages in thread From: Kumar, Shobhit @ 2015-03-01 13:24 UTC (permalink / raw) To: Alexandre Courbot, Shobhit Kumar Cc: Jani Nikula, Daniel Vetter, intel-gfx, Thierry Reding, Linus Walleij 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2015-03-25 14:39 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-02-18 12:18 [PATCH 0/2] Crystal Cove PMIC based Panel Control Shobhit Kumar 2015-02-18 12:18 ` [PATCH 1/2] gpio/crystalcove: Export Panel and backlight en/disable signals as GPIO Shobhit Kumar 2015-03-06 11:04 ` Linus Walleij 2015-03-06 16:23 ` Kumar, Shobhit 2015-03-09 17:15 ` Linus Walleij 2015-03-12 15:06 ` Kumar, Shobhit 2015-03-18 11:50 ` Linus Walleij 2015-03-18 13:27 ` Daniel Vetter 2015-03-24 9:50 ` Linus Walleij 2015-03-25 12:27 ` Linus Walleij 2015-03-25 14:15 ` Kumar, Shobhit 2015-03-25 14:39 ` Linus Walleij 2015-02-18 12:18 ` [PATCH 2/2] drm/i915: Use the CRC gpio_chip for panel enable/disable Shobhit Kumar 2015-02-26 10:13 ` Alexandre Courbot 2015-03-01 13:24 ` Kumar, Shobhit
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.