| Message ID | 5358DEEA.1000506@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tomi, On Thursday 24 April 2014 12:52:42 Tomi Valkeinen wrote: > On 18/04/14 18:51, Tony Lindgren wrote: > >> + gpio = of_get_gpio(node, 0); > >> + if (gpio_is_valid(gpio) || gpio == -ENOENT) { > >> + ddata->enable_gpio = gpio; > >> + } else { > >> + dev_err(&pdev->dev, "failed to parse enable gpio\n"); > >> + return gpio; > >> + } > > > > We should set the GPIO polarity based on the OF_GPIO_ACTIVE_LOW like > > gpio_backlight_probe_dt is doing. > > Instead of doing it with the old gpio API, and checking the 'active' > flag everywhere, I think we can use the new gpiod API which handles the > polarity automatically. > > I attached prototype patches (based on -rc2) for panel dpi using that > approach. It's a bit messier than I'd like, because for non-DT boot we > need to request the gpio using the old API, and then convert it to > gpio_desc. We can remove that code when all the boards use DT. Just FYI, you can use the gpiod API with non-DT platforms if you register GPIO lookup entries in board code with gpiod_add_lookup_table(). > I've compiled tested this only, as I don't have DPI panels I could use. > I did try similar approach for TFP410, and it seemed to work fine.
On 24/04/14 15:44, Laurent Pinchart wrote: >> I attached prototype patches (based on -rc2) for panel dpi using that >> approach. It's a bit messier than I'd like, because for non-DT boot we >> need to request the gpio using the old API, and then convert it to >> gpio_desc. We can remove that code when all the boards use DT. > > Just FYI, you can use the gpiod API with non-DT platforms if you register GPIO > lookup entries in board code with gpiod_add_lookup_table(). Right, but as the board files are going away, I'd rather not touch them at all if possible. Tomi
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140424 02:53]: > On 18/04/14 18:51, Tony Lindgren wrote: > > >> + gpio = of_get_gpio(node, 0); > >> + if (gpio_is_valid(gpio) || gpio == -ENOENT) { > >> + ddata->enable_gpio = gpio; > >> + } else { > >> + dev_err(&pdev->dev, "failed to parse enable gpio\n"); > >> + return gpio; > >> + } > > > > We should set the GPIO polarity based on the OF_GPIO_ACTIVE_LOW like > > gpio_backlight_probe_dt is doing. > > Instead of doing it with the old gpio API, and checking the 'active' > flag everywhere, I think we can use the new gpiod API which handles the > polarity automatically. > > I attached prototype patches (based on -rc2) for panel dpi using that > approach. It's a bit messier than I'd like, because for non-DT boot we > need to request the gpio using the old API, and then convert it to > gpio_desc. We can remove that code when all the boards use DT. > > I've compiled tested this only, as I don't have DPI panels I could use. > I did try similar approach for TFP410, and it seemed to work fine. Got these working by updating my test patch to use enable-gpios instead of gpios, and had to change from GPIO_ACTIVE_LOW to GPIO_ACTIVE_HIGH. Are we now also breaking legacy booting by reversing the polarity? In any case, looks like we have some duplicate panel code.. Turns out most panel dpi users for omap3 board-*.c files are just sharp-ls037v7dw01 panels but configured in QVGA mode. At least for EVM and and LDP based on looking at the pictures and the configuration pins (using the names kernel): QVGA = lcd MO reset = lcd RESB ... Then the enable_gpio should be just a GPIO controlled 3.3V regulator in most cases. I suggest we move them over to ls037v7dw01 and allow configuring them both for VGA and QVGA depending on the orientation. I guess you do have some device with ls037v7dw01 since you've been patching it? Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26/04/14 02:53, Tony Lindgren wrote: > * Tomi Valkeinen <tomi.valkeinen@ti.com> [140424 02:53]: >> On 18/04/14 18:51, Tony Lindgren wrote: >> >>>> + gpio = of_get_gpio(node, 0); >>>> + if (gpio_is_valid(gpio) || gpio == -ENOENT) { >>>> + ddata->enable_gpio = gpio; >>>> + } else { >>>> + dev_err(&pdev->dev, "failed to parse enable gpio\n"); >>>> + return gpio; >>>> + } >>> >>> We should set the GPIO polarity based on the OF_GPIO_ACTIVE_LOW like >>> gpio_backlight_probe_dt is doing. >> >> Instead of doing it with the old gpio API, and checking the 'active' >> flag everywhere, I think we can use the new gpiod API which handles the >> polarity automatically. >> >> I attached prototype patches (based on -rc2) for panel dpi using that >> approach. It's a bit messier than I'd like, because for non-DT boot we >> need to request the gpio using the old API, and then convert it to >> gpio_desc. We can remove that code when all the boards use DT. >> >> I've compiled tested this only, as I don't have DPI panels I could use. >> I did try similar approach for TFP410, and it seemed to work fine. > > Got these working by updating my test patch to use enable-gpios instead > of gpios, and had to change from GPIO_ACTIVE_LOW to GPIO_ACTIVE_HIGH. > Are we now also breaking legacy booting by reversing the polarity? I don't think so. The GPIOs should be active-high by default, if I'm not mistaken, so the polarities should be the same for legacy boot with or without those patches. Of course, I don't have the boards so I have no idea if the polarities have been correct even before. debugfs/gpio shows the actual value of the gpio, so you could check from there what it is. > In any case, looks like we have some duplicate panel code.. Turns > out most panel dpi users for omap3 board-*.c files are just > sharp-ls037v7dw01 panels but configured in QVGA mode. At least for > EVM and and LDP based on looking at the pictures and the configuration Hmm, true, board-ldp.c's panel looks very much like sharp-ls037v7dw01. Which EVM are you talking about? > pins (using the names kernel): > > QVGA = lcd MO > reset = lcd RESB > ... > > Then the enable_gpio should be just a GPIO controlled 3.3V regulator > in most cases. I suggest we move them over to ls037v7dw01 and allow > configuring them both for VGA and QVGA depending on the orientation. Looking at the panel spec, it has the following pins: RESB - reset MO - VGA/QVGA UD - vertical scanning direction LR - horizontal scanning direction INI - power on control And it needs 3.3V power. Are you saying that on some boards the gpio used for enable_gpio is actually used to switch on a 3.3V regulator? > I guess you do have some device with ls037v7dw01 since you've been > patching it? No, I don't have any boards with that panel. Tomi
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140428 03:44]: > On 26/04/14 02:53, Tony Lindgren wrote: > > * Tomi Valkeinen <tomi.valkeinen@ti.com> [140424 02:53]: > >> On 18/04/14 18:51, Tony Lindgren wrote: > >> > >>>> + gpio = of_get_gpio(node, 0); > >>>> + if (gpio_is_valid(gpio) || gpio == -ENOENT) { > >>>> + ddata->enable_gpio = gpio; > >>>> + } else { > >>>> + dev_err(&pdev->dev, "failed to parse enable gpio\n"); > >>>> + return gpio; > >>>> + } > >>> > >>> We should set the GPIO polarity based on the OF_GPIO_ACTIVE_LOW like > >>> gpio_backlight_probe_dt is doing. > >> > >> Instead of doing it with the old gpio API, and checking the 'active' > >> flag everywhere, I think we can use the new gpiod API which handles the > >> polarity automatically. > >> > >> I attached prototype patches (based on -rc2) for panel dpi using that > >> approach. It's a bit messier than I'd like, because for non-DT boot we > >> need to request the gpio using the old API, and then convert it to > >> gpio_desc. We can remove that code when all the boards use DT. > >> > >> I've compiled tested this only, as I don't have DPI panels I could use. > >> I did try similar approach for TFP410, and it seemed to work fine. > > > > Got these working by updating my test patch to use enable-gpios instead > > of gpios, and had to change from GPIO_ACTIVE_LOW to GPIO_ACTIVE_HIGH. > > Are we now also breaking legacy booting by reversing the polarity? > > I don't think so. The GPIOs should be active-high by default, if I'm not > mistaken, so the polarities should be the same for legacy boot with or > without those patches. Of course, I don't have the boards so I have no > idea if the polarities have been correct even before. OK > debugfs/gpio shows the actual value of the gpio, so you could check from > there what it is. Yeah that should be checked. > > In any case, looks like we have some duplicate panel code.. Turns > > out most panel dpi users for omap3 board-*.c files are just > > sharp-ls037v7dw01 panels but configured in QVGA mode. At least for > > EVM and and LDP based on looking at the pictures and the configuration > > Hmm, true, board-ldp.c's panel looks very much like sharp-ls037v7dw01. Yes it seems so also based on the photos of the LCD panel. And 3430sdp also seems to have something similar but it's upside down. > Which EVM are you talking about? The LogicPD omap3 EVMs TMDSEVM3530 and TMDSEVM3730. The am335x EVM has a different larger panel. > > pins (using the names kernel): > > > > QVGA = lcd MO > > reset = lcd RESB > > ... > > > > Then the enable_gpio should be just a GPIO controlled 3.3V regulator > > in most cases. I suggest we move them over to ls037v7dw01 and allow > > configuring them both for VGA and QVGA depending on the orientation. > > Looking at the panel spec, it has the following pins: > > RESB - reset > MO - VGA/QVGA > UD - vertical scanning direction > LR - horizontal scanning direction > INI - power on control > > And it needs 3.3V power. > > Are you saying that on some boards the gpio used for enable_gpio is > actually used to switch on a 3.3V regulator? The 3.3V GPIO regulator is needed in addition to the configurable pins, I guess some ls037v7dw01 panels only have a subset of the pins controllable by GPIOs, and the GPIO regulator may be used instead of the INI. But that's hard to know without schematics. > > I guess you do have some device with ls037v7dw01 since you've been > > patching it? > > No, I don't have any boards with that panel. OK. I'll move my boards over to ls037v7dw01 and do a DT conversion patch for it that just sets a standard gpios property for panel ls037v7dw01. The gpios entry can have 0 entries in the middle depending on wiring configuration and there should not be any need to name each GPIO. As far as I'm concerned, your panel-dpi patch is fine with me, and we should not add more configuration to it for the ls037v7dw01 panels. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From fe2a85da34499fab70212c4cc5870378678da709 Mon Sep 17 00:00:00 2001 From: Tomi Valkeinen <tomi.valkeinen@ti.com> Date: Thu, 16 May 2013 15:14:16 +0300 Subject: [PATCH 2/3] OMAPDSS: panel-dpi: Add DT support Add DT support for panel-dpi. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> Reviewed-by: Archit Taneja <archit@ti.com> --- drivers/video/fbdev/omap2/displays-new/panel-dpi.c | 59 +++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/drivers/video/fbdev/omap2/displays-new/panel-dpi.c b/drivers/video/fbdev/omap2/displays-new/panel-dpi.c index d379dec3bd4a..dca6b10d1157 100644 --- a/drivers/video/fbdev/omap2/displays-new/panel-dpi.c +++ b/drivers/video/fbdev/omap2/displays-new/panel-dpi.c @@ -13,9 +13,12 @@ #include <linux/module.h> #include <linux/platform_device.h> #include <linux/slab.h> +#include <linux/of.h> +#include <linux/of_gpio.h> #include <video/omapdss.h> #include <video/omap-panel-data.h> +#include <video/of_display_timing.h> struct panel_drv_data { struct omap_dss_device dssdev; @@ -72,7 +75,8 @@ static int panel_dpi_enable(struct omap_dss_device *dssdev) if (omapdss_device_is_enabled(dssdev)) return 0; - in->ops.dpi->set_data_lines(in, ddata->data_lines); + if (ddata->data_lines) + in->ops.dpi->set_data_lines(in, ddata->data_lines); in->ops.dpi->set_timings(in, &ddata->videomode); r = in->ops.dpi->enable(in); @@ -195,6 +199,47 @@ err_gpio: return r; } +static int panel_dpi_probe_of(struct platform_device *pdev) +{ + struct panel_drv_data *ddata = platform_get_drvdata(pdev); + struct device_node *node = pdev->dev.of_node; + struct omap_dss_device *in; + int r; + struct display_timing timing; + struct videomode vm; + struct gpio_desc *gpio; + + gpio = devm_gpiod_get(&pdev->dev, "enable"); + if (IS_ERR(gpio)) { + dev_err(&pdev->dev, "failed to parse enable gpio\n"); + return PTR_ERR(gpio); + } else { + gpiod_direction_output(gpio, 0); + ddata->enable_gpio = gpio; + } + + ddata->backlight_gpio = -ENOENT; + + r = of_get_display_timing(node, "panel-timing", &timing); + if (r) { + dev_err(&pdev->dev, "failed to get video timing\n"); + return r; + } + + videomode_from_timing(&timing, &vm); + videomode_to_omap_video_timings(&vm, &ddata->videomode); + + in = omapdss_of_find_source_for_first_ep(node); + if (IS_ERR(in)) { + dev_err(&pdev->dev, "failed to find video source\n"); + return PTR_ERR(in); + } + + ddata->in = in; + + return 0; +} + static int panel_dpi_probe(struct platform_device *pdev) { struct panel_drv_data *ddata; @@ -211,6 +256,10 @@ static int panel_dpi_probe(struct platform_device *pdev) r = panel_dpi_probe_pdata(pdev); if (r) return r; + } else if (pdev->dev.of_node) { + r = panel_dpi_probe_of(pdev); + if (r) + return r; } else { return -ENODEV; } @@ -260,12 +309,20 @@ static int __exit panel_dpi_remove(struct platform_device *pdev) return 0; } +static const struct of_device_id panel_dpi_of_match[] = { + { .compatible = "omapdss,panel-dpi", }, + {}, +}; + +MODULE_DEVICE_TABLE(of, panel_dpi_of_match); + static struct platform_driver panel_dpi_driver = { .probe = panel_dpi_probe, .remove = __exit_p(panel_dpi_remove), .driver = { .name = "panel-dpi", .owner = THIS_MODULE, + .of_match_table = panel_dpi_of_match, }, }; -- 1.9.1