| Message ID | 20140513212639.GA18001@atomide.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 14/05/14 00:26, Tony Lindgren wrote: > + /* lcd MO */ > + ddata->mo_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 1, "mode"); > + if (PTR_ERR(ddata->mo_gpio) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > + if (!IS_ERR(ddata->mo_gpio)) > + if (gpiod_get_raw_value_cansleep(ddata->mo_gpio)) > + ddata->flags |= SHARP_LS_QVGA; Shouldn't there be an explicit flag in the DT data for this? If the panel's MO pin is hardwired to, say, pull up, then the mode-gpios won't have MO gpio, right? So something like: mode-gpios = <0 /* high, lcd MO */ &gpio1 2 GPIO_ACTIVE_HIGH /* gpio2, lcd LR */ &gpio1 3 GPIO_ACTIVE_HIGH>; /* gpio3, lcd UD */ vga-mode; /* MO hardwired high */ Btw, the gpio.txt has each gpio inside <>: chipsel-gpios = <&gpio1 12 0>, <&gpio1 13 0>, <0>, /* holes are permitted, means no GPIO 2 */ <&gpio2 2>; Is that equivalent to having all gpios inside <>? Tomi
On 14/05/14 00:26, Tony Lindgren wrote: > +static int sharp_ls_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; > + > + ddata->vcc = devm_regulator_get(&pdev->dev, "envdd"); > + if (IS_ERR(ddata->vcc)) { > + dev_err(&pdev->dev, "failed to get regulator\n"); > + return PTR_ERR(ddata->vcc); > + } > + > + /* lcd INI */ > + ddata->ini_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 0, "enable"); > + if (PTR_ERR(ddata->ini_gpio) == -EPROBE_DEFER) > + return -EPROBE_DEFER; Hmm, the GPIOs are optional, but shouldn't we react somehow to real errors? I guess we should do something like: ddata->ini_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 0, "enable"); if (IS_ERR(ddata->ini_gpio) { int err = PTR_ERR(ddata->ini_gpio); if (err == -EPROBE_DEFER || err != -ENOENT) return err; } Tomi
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140515 01:42]: > On 14/05/14 00:26, Tony Lindgren wrote: > > > + /* lcd MO */ > > + ddata->mo_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 1, "mode"); > > + if (PTR_ERR(ddata->mo_gpio) == -EPROBE_DEFER) > > + return -EPROBE_DEFER; > > + > > + if (!IS_ERR(ddata->mo_gpio)) > > + if (gpiod_get_raw_value_cansleep(ddata->mo_gpio)) > > + ddata->flags |= SHARP_LS_QVGA; > > Shouldn't there be an explicit flag in the DT data for this? If the > panel's MO pin is hardwired to, say, pull up, then the mode-gpios won't > have MO gpio, right? So something like: > > > mode-gpios = <0 /* high, lcd MO */ > &gpio1 2 GPIO_ACTIVE_HIGH /* gpio2, lcd LR */ > &gpio1 3 GPIO_ACTIVE_HIGH>; /* gpio3, lcd UD */ > > vga-mode; /* MO hardwired high */ Yeah holes there are just fine. I figured let's keep the custom vga-mode property out of the way until we actually run into a panel that's not using a GPIO for mode. So far it seems that mode GPIO is there for the panels I've seen, just the scan direction pins seem to be hard wired on LDP. But then again, maybe I'm still having trouble locating all the GPIOs in the LDP schematics. > Btw, the gpio.txt has each gpio inside <>: > > chipsel-gpios = <&gpio1 12 0>, > <&gpio1 13 0>, > <0>, /* holes are permitted, means no GPIO 2 */ > <&gpio2 2>; > > Is that equivalent to having all gpios inside <>? Yeah, just less <> braces. The number of elements for each entry is what matters and that's known by the GPIO parsing code. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140515 06:08]: > On 14/05/14 00:26, Tony Lindgren wrote: > > > +static int sharp_ls_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; > > + > > + ddata->vcc = devm_regulator_get(&pdev->dev, "envdd"); > > + if (IS_ERR(ddata->vcc)) { > > + dev_err(&pdev->dev, "failed to get regulator\n"); > > + return PTR_ERR(ddata->vcc); > > + } > > + > > + /* lcd INI */ > > + ddata->ini_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 0, "enable"); > > + if (PTR_ERR(ddata->ini_gpio) == -EPROBE_DEFER) > > + return -EPROBE_DEFER; > > Hmm, the GPIOs are optional, but shouldn't we react somehow to real > errors? I guess we should do something like: > > ddata->ini_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 0, "enable"); > if (IS_ERR(ddata->ini_gpio) { > int err = PTR_ERR(ddata->ini_gpio); > if (err == -EPROBE_DEFER || err != -ENOENT) > return err; > } Yeah that makes sense to me. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15/05/14 21:25, Tony Lindgren wrote: > * Tomi Valkeinen <tomi.valkeinen@ti.com> [140515 01:42]: >> On 14/05/14 00:26, Tony Lindgren wrote: >> >>> + /* lcd MO */ >>> + ddata->mo_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 1, "mode"); >>> + if (PTR_ERR(ddata->mo_gpio) == -EPROBE_DEFER) >>> + return -EPROBE_DEFER; >>> + >>> + if (!IS_ERR(ddata->mo_gpio)) >>> + if (gpiod_get_raw_value_cansleep(ddata->mo_gpio)) >>> + ddata->flags |= SHARP_LS_QVGA; >> >> Shouldn't there be an explicit flag in the DT data for this? If the >> panel's MO pin is hardwired to, say, pull up, then the mode-gpios won't >> have MO gpio, right? So something like: >> >> >> mode-gpios = <0 /* high, lcd MO */ >> &gpio1 2 GPIO_ACTIVE_HIGH /* gpio2, lcd LR */ >> &gpio1 3 GPIO_ACTIVE_HIGH>; /* gpio3, lcd UD */ >> >> vga-mode; /* MO hardwired high */ > > Yeah holes there are just fine. I figured let's keep the custom > vga-mode property out of the way until we actually run into a panel > that's not using a GPIO for mode. Ok, I'm fine with that, but in that case I think we have to use all the panels in the same mode, i.e. the driver either sets the MO gpio always low and uses VGA mode, or sets the MO always high and uses QVGA mode. In your omap3-evm.dts patch, you set the MO gpio ACTIVE_LOW in order to change the mode, even if it really should be ACTIVE_HIGH. But if you do that, and we later add the support to the panel driver to manage the MO gpio dynamically (i.e. the user can select VGA/QVGA at runtime), then the panel won't work as the driver uses wrong polarity for the pin. Tomi
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140515 22:51]: > On 15/05/14 21:25, Tony Lindgren wrote: > > * Tomi Valkeinen <tomi.valkeinen@ti.com> [140515 01:42]: > >> On 14/05/14 00:26, Tony Lindgren wrote: > >> > >>> + /* lcd MO */ > >>> + ddata->mo_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 1, "mode"); > >>> + if (PTR_ERR(ddata->mo_gpio) == -EPROBE_DEFER) > >>> + return -EPROBE_DEFER; > >>> + > >>> + if (!IS_ERR(ddata->mo_gpio)) > >>> + if (gpiod_get_raw_value_cansleep(ddata->mo_gpio)) > >>> + ddata->flags |= SHARP_LS_QVGA; > >> > >> Shouldn't there be an explicit flag in the DT data for this? If the > >> panel's MO pin is hardwired to, say, pull up, then the mode-gpios won't > >> have MO gpio, right? So something like: > >> > >> > >> mode-gpios = <0 /* high, lcd MO */ > >> &gpio1 2 GPIO_ACTIVE_HIGH /* gpio2, lcd LR */ > >> &gpio1 3 GPIO_ACTIVE_HIGH>; /* gpio3, lcd UD */ > >> > >> vga-mode; /* MO hardwired high */ > > > > Yeah holes there are just fine. I figured let's keep the custom > > vga-mode property out of the way until we actually run into a panel > > that's not using a GPIO for mode. > > Ok, I'm fine with that, but in that case I think we have to use all the > panels in the same mode, i.e. the driver either sets the MO gpio always > low and uses VGA mode, or sets the MO always high and uses QVGA mode. OK let's default to VGA mode for now. > In your omap3-evm.dts patch, you set the MO gpio ACTIVE_LOW in order to > change the mode, even if it really should be ACTIVE_HIGH. But if you do > that, and we later add the support to the panel driver to manage the MO > gpio dynamically (i.e. the user can select VGA/QVGA at runtime), then > the panel won't work as the driver uses wrong polarity for the pin. Getting the configured value seemed to work just fine with gpiod_get_raw_value_cansleep(ddata->mo_gpio). But yeah I agree the lack and confusion between polarity and desired default value for a GPIO is is sucky. The ACTIVE_HIGH probably should mean polarity. I don't know if there's an easy solution to that short of adding a new GPIO binding that contains both the polarity and the desired default value. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- /dev/null +++ b/Documentation/devicetree/bindings/video/sharp,ls037v7dw01.txt @@ -0,0 +1,44 @@ +SHARP LS037V7DW01 TFT-LCD panel +=================================== + +Required properties: +- compatible: "sharp,ls037v7dw01" + +Optional properties: +- label: a symbolic name for the panel +- enable-gpios: a GPIO spec for the optional enable pin + this pin is the INI pin as specified in the LS037V7DW01.pdf file. +- reset-gpios: a GPIO spec for the optional reset pin + this pin is the RESB pin as specified in the LS037V7DW01.pdf file. +- mode-gpios: a GPIO + ordered MO, LR, and UD as specified in the LS037V7DW01.pdf file. + +Required nodes: +- Video port for DPI input + +This panel can have zero to five GPIOs to configure +to change configuration between QVGA and VGA mode +and the scan direction. As these pins can be also +configured with external pulls, all the GPIOs are +considered optional with holes in the array. + +Example +------- + +Example when connected to a omap2+ based device: + +lcd0: display { + compatible = "sharp,ls037v7dw01"; + power-supply = <&lcd_3v3>; + enable-gpios = <&gpio5 24 GPIO_ACTIVE_HIGH>; /* gpio152, lcd INI */ + reset-gpios = <&gpio5 27 GPIO_ACTIVE_HIGH>; /* gpio155, lcd RESB */ + mode-gpios = <&gpio5 26 GPIO_ACTIVE_HIGH /* gpio154, lcd MO */ + &gpio1 2 GPIO_ACTIVE_HIGH /* gpio2, lcd LR */ + &gpio1 3 GPIO_ACTIVE_HIGH>; /* gpio3, lcd UD */ + + port { + lcd_in: endpoint { + remote-endpoint = <&dpi_out>; + }; + }; +}; --- a/arch/arm/mach-omap2/display.c +++ b/arch/arm/mach-omap2/display.c @@ -562,6 +562,7 @@ static const char * const dss_compat_conv_list[] __initconst = { "hdmi-connector", "panel-dpi", "panel-dsi-cm", + "sharp,ls037v7dw01", "sony,acx565akm", "svideo-connector", "ti,tfp410", --- a/drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c +++ b/drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c @@ -12,15 +12,18 @@ #include <linux/delay.h> #include <linux/gpio.h> #include <linux/module.h> +#include <linux/of.h> +#include <linux/of_gpio.h> #include <linux/platform_device.h> #include <linux/slab.h> - +#include <linux/regulator/consumer.h> #include <video/omapdss.h> #include <video/omap-panel-data.h> struct panel_drv_data { struct omap_dss_device dssdev; struct omap_dss_device *in; + struct regulator *vcc; int data_lines; @@ -31,9 +34,33 @@ struct panel_drv_data { struct gpio_desc *mo_gpio; /* low = 480x640, high = 240x320 */ struct gpio_desc *lr_gpio; /* high = conventional horizontal scanning */ struct gpio_desc *ud_gpio; /* high = conventional vertical scanning */ + +#define SHARP_LS_QVGA (1 << 0) + u32 flags; +}; + +static const struct omap_video_timings sharp_ls_qvga_timings = { + .x_res = 240, + .y_res = 320, + + .pixelclock = 5400000, + + .hsw = 3, + .hfp = 3, + .hbp = 39, + + .vsw = 1, + .vfp = 2, + .vbp = 7, + + .vsync_level = OMAPDSS_SIG_ACTIVE_LOW, + .hsync_level = OMAPDSS_SIG_ACTIVE_LOW, + .data_pclk_edge = OMAPDSS_DRIVE_SIG_RISING_EDGE, + .de_level = OMAPDSS_SIG_ACTIVE_HIGH, + .sync_pclk_edge = OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES, }; -static const struct omap_video_timings sharp_ls_timings = { +static const struct omap_video_timings sharp_ls_vga_timings = { .x_res = 480, .y_res = 640, @@ -95,12 +122,21 @@ static int sharp_ls_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); + if (ddata->vcc) { + r = regulator_enable(ddata->vcc); + if (r != 0) + return r; + } + r = in->ops.dpi->enable(in); - if (r) + if (r) { + regulator_disable(ddata->vcc); return r; + } /* wait couple of vsyncs until enabling the LCD */ msleep(50); @@ -136,6 +172,9 @@ static void sharp_ls_disable(struct omap_dss_device *dssdev) in->ops.dpi->disable(in); + if (ddata->vcc) + regulator_disable(ddata->vcc); + dssdev->state = OMAP_DSS_DISPLAY_DISABLED; } @@ -243,6 +282,72 @@ static int sharp_ls_probe_pdata(struct platform_device *pdev) return 0; } +static struct gpio_desc * +sharp_ls_get_gpio_of(struct device *dev, int index, int val, char *desc) +{ + struct gpio_desc *gpio; + + gpio = devm_gpiod_get_index(dev, desc, index); + if (IS_ERR(gpio)) + return gpio; + + gpiod_direction_output(gpio, val); + + return gpio; +} + +static int sharp_ls_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; + + ddata->vcc = devm_regulator_get(&pdev->dev, "envdd"); + if (IS_ERR(ddata->vcc)) { + dev_err(&pdev->dev, "failed to get regulator\n"); + return PTR_ERR(ddata->vcc); + } + + /* lcd INI */ + ddata->ini_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 0, "enable"); + if (PTR_ERR(ddata->ini_gpio) == -EPROBE_DEFER) + return -EPROBE_DEFER; + + /* lcd RESB */ + ddata->resb_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 1, "reset"); + if (PTR_ERR(ddata->resb_gpio) == -EPROBE_DEFER) + return -EPROBE_DEFER; + + /* lcd MO */ + ddata->mo_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 1, "mode"); + if (PTR_ERR(ddata->mo_gpio) == -EPROBE_DEFER) + return -EPROBE_DEFER; + + if (!IS_ERR(ddata->mo_gpio)) + if (gpiod_get_raw_value_cansleep(ddata->mo_gpio)) + ddata->flags |= SHARP_LS_QVGA; + + /* lcd LR */ + ddata->lr_gpio = sharp_ls_get_gpio_of(&pdev->dev, 1, 1, "mode"); + if (PTR_ERR(ddata->lr_gpio) == -EPROBE_DEFER) + return -EPROBE_DEFER; + + /* lcd UD */ + ddata->ud_gpio = sharp_ls_get_gpio_of(&pdev->dev, 2, 1, "mode"); + if (PTR_ERR(ddata->ud_gpio) == -EPROBE_DEFER) + return -EPROBE_DEFER; + + 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 sharp_ls_probe(struct platform_device *pdev) { struct panel_drv_data *ddata; @@ -259,11 +364,18 @@ static int sharp_ls_probe(struct platform_device *pdev) r = sharp_ls_probe_pdata(pdev); if (r) return r; + } else if (pdev->dev.of_node) { + r = sharp_ls_probe_of(pdev); + if (r) + return r; } else { return -ENODEV; } - ddata->videomode = sharp_ls_timings; + if (ddata->flags & SHARP_LS_QVGA) + ddata->videomode = sharp_ls_qvga_timings; + else + ddata->videomode = sharp_ls_vga_timings; dssdev = &ddata->dssdev; dssdev->dev = &pdev->dev; @@ -302,12 +414,20 @@ static int __exit sharp_ls_remove(struct platform_device *pdev) return 0; } +static const struct of_device_id sharp_ls_of_match[] = { + { .compatible = "omapdss,sharp,ls037v7dw01", }, + {}, +}; + +MODULE_DEVICE_TABLE(of, sharp_ls_of_match); + static struct platform_driver sharp_ls_driver = { .probe = sharp_ls_probe, .remove = __exit_p(sharp_ls_remove), .driver = { .name = "panel-sharp-ls037v7dw01", .owner = THIS_MODULE, + .of_match_table = sharp_ls_of_match, }, };