| Message ID | 1360765345-19312-6-git-send-email-archit@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Archit, On 02/13/13 16:21, Archit Taneja wrote: > The cm-t35 board file currently requests gpios required to configure the tdo35s > panel, and provides platform_enable/disable callbacks to configure them. > > These tasks have been moved to the generic dpi panel driver itself and shouldn't > be done in the board files. > > Remove the gpio requests and the platform callbacks from the board file. > Add the gpio information to generic dpi panel's platform data so that it's > passed to the panel driver. > > Note: In cm_t35_init_display(), the gpios were disabled, and the LCD_EN gpio was > enabled after a 50 millisecond delay. This code has been removed and is not > taken care of in the generic panel driver. The impact of this needs to be > tested. The panel's gpios are also not exported any more. Hence, they can't be > accessed via sysfs interface. Indeed, there is an impact - the LCD no longer works. The reason for the LCD_EN gpio being pushed high after the 50ms delay, is to get the LCD out of reset, so the SPI transaction will succeed and initialize the LCD. Now, when you remove the gpio handling for the LCD_EN pin, the LCD no longer works. I don't agree with this breakage. > > Cc: Tony Lindgren <tony@atomide.com> > Signed-off-by: Archit Taneja <archit@ti.com> > --- > arch/arm/mach-omap2/board-cm-t35.c | 46 ++++-------------------------------- > 1 file changed, 5 insertions(+), 41 deletions(-) > > diff --git a/arch/arm/mach-omap2/board-cm-t35.c b/arch/arm/mach-omap2/board-cm-t35.c > index f940a79..563d5ab 100644 > --- a/arch/arm/mach-omap2/board-cm-t35.c > +++ b/arch/arm/mach-omap2/board-cm-t35.c > @@ -189,32 +189,6 @@ static inline void cm_t35_init_nand(void) {} > #define CM_T35_LCD_BL_GPIO 58 > #define CM_T35_DVI_EN_GPIO 54 > > -static int lcd_enabled; > -static int dvi_enabled; > - > -static int cm_t35_panel_enable_lcd(struct omap_dss_device *dssdev) > -{ > - if (dvi_enabled) { > - printk(KERN_ERR "cannot enable LCD, DVI is enabled\n"); > - return -EINVAL; > - } > - > - gpio_set_value(CM_T35_LCD_EN_GPIO, 1); > - gpio_set_value(CM_T35_LCD_BL_GPIO, 1); > - > - lcd_enabled = 1; > - > - return 0; > -} > - > -static void cm_t35_panel_disable_lcd(struct omap_dss_device *dssdev) > -{ > - lcd_enabled = 0; > - > - gpio_set_value(CM_T35_LCD_BL_GPIO, 0); > - gpio_set_value(CM_T35_LCD_EN_GPIO, 0); > -} > - > static int cm_t35_panel_enable_tv(struct omap_dss_device *dssdev) > { > return 0; > @@ -226,8 +200,11 @@ static void cm_t35_panel_disable_tv(struct omap_dss_device *dssdev) > > static struct panel_generic_dpi_data lcd_panel = { > .name = "toppoly_tdo35s", > - .platform_enable = cm_t35_panel_enable_lcd, > - .platform_disable = cm_t35_panel_disable_lcd, > + .num_gpios = 2, > + .gpios = { > + CM_T35_LCD_BL_GPIO, > + CM_T35_LCD_EN_GPIO > + }, > }; > > static struct omap_dss_device cm_t35_lcd_device = { > @@ -303,19 +280,6 @@ static void __init cm_t35_init_display(void) > spi_register_board_info(cm_t35_lcd_spi_board_info, > ARRAY_SIZE(cm_t35_lcd_spi_board_info)); > > - err = gpio_request_array(cm_t35_dss_gpios, > - ARRAY_SIZE(cm_t35_dss_gpios)); > - if (err) { > - pr_err("CM-T35: failed to request DSS control GPIOs\n"); > - return; > - } > - > - gpio_export(CM_T35_LCD_EN_GPIO, 0); > - gpio_export(CM_T35_LCD_BL_GPIO, 0); > - > - msleep(50); > - gpio_set_value(CM_T35_LCD_EN_GPIO, 1); > - > err = omap_display_init(&cm_t35_dss_data); > if (err) { > pr_err("CM-T35: failed to register DSS device\n"); >
On 2013-02-13 17:16, Igor Grinberg wrote: > Hi Archit, > > On 02/13/13 16:21, Archit Taneja wrote: >> The cm-t35 board file currently requests gpios required to configure the tdo35s >> panel, and provides platform_enable/disable callbacks to configure them. >> >> These tasks have been moved to the generic dpi panel driver itself and shouldn't >> be done in the board files. >> >> Remove the gpio requests and the platform callbacks from the board file. >> Add the gpio information to generic dpi panel's platform data so that it's >> passed to the panel driver. >> >> Note: In cm_t35_init_display(), the gpios were disabled, and the LCD_EN gpio was >> enabled after a 50 millisecond delay. This code has been removed and is not >> taken care of in the generic panel driver. The impact of this needs to be >> tested. The panel's gpios are also not exported any more. Hence, they can't be >> accessed via sysfs interface. > > Indeed, there is an impact - the LCD no longer works. > The reason for the LCD_EN gpio being pushed high after the 50ms delay, > is to get the LCD out of reset, so the SPI transaction will succeed > and initialize the LCD. > Now, when you remove the gpio handling for the LCD_EN pin, > the LCD no longer works. So between what is the sleep done? It's not clear from the code. LCD_EN needs to be 0 for 50ms, or...? If the panel requires specific reset handling, does it work right even currently when the panel is turned off and later turned on? The msleep is only used at boot time. Tomi
On 2013-02-13 17:28, Tomi Valkeinen wrote: > On 2013-02-13 17:16, Igor Grinberg wrote: >> Hi Archit, >> >> On 02/13/13 16:21, Archit Taneja wrote: >>> The cm-t35 board file currently requests gpios required to configure the tdo35s >>> panel, and provides platform_enable/disable callbacks to configure them. >>> >>> These tasks have been moved to the generic dpi panel driver itself and shouldn't >>> be done in the board files. >>> >>> Remove the gpio requests and the platform callbacks from the board file. >>> Add the gpio information to generic dpi panel's platform data so that it's >>> passed to the panel driver. >>> >>> Note: In cm_t35_init_display(), the gpios were disabled, and the LCD_EN gpio was >>> enabled after a 50 millisecond delay. This code has been removed and is not >>> taken care of in the generic panel driver. The impact of this needs to be >>> tested. The panel's gpios are also not exported any more. Hence, they can't be >>> accessed via sysfs interface. >> >> Indeed, there is an impact - the LCD no longer works. >> The reason for the LCD_EN gpio being pushed high after the 50ms delay, >> is to get the LCD out of reset, so the SPI transaction will succeed >> and initialize the LCD. >> Now, when you remove the gpio handling for the LCD_EN pin, >> the LCD no longer works. > > So between what is the sleep done? It's not clear from the code. LCD_EN > needs to be 0 for 50ms, or...? > > If the panel requires specific reset handling, does it work right even > currently when the panel is turned off and later turned on? The msleep > is only used at boot time. Okay, so I just realized there's an spi backlight driver used here, and that backlight driver is actually handling the SPI transactions with the panel, instead of the panel driver. So this looks quite messed up. For a quick solution, can we just set the LCD_EN at boot time (with the msleep), and not touch it after that? Tomi
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 02/13/13 17:59, Tomi Valkeinen wrote: > On 2013-02-13 17:28, Tomi Valkeinen wrote: >> On 2013-02-13 17:16, Igor Grinberg wrote: >>> Hi Archit, >>> >>> On 02/13/13 16:21, Archit Taneja wrote: >>>> The cm-t35 board file currently requests gpios required to configure the tdo35s >>>> panel, and provides platform_enable/disable callbacks to configure them. >>>> >>>> These tasks have been moved to the generic dpi panel driver itself and shouldn't >>>> be done in the board files. >>>> >>>> Remove the gpio requests and the platform callbacks from the board file. >>>> Add the gpio information to generic dpi panel's platform data so that it's >>>> passed to the panel driver. >>>> >>>> Note: In cm_t35_init_display(), the gpios were disabled, and the LCD_EN gpio was >>>> enabled after a 50 millisecond delay. This code has been removed and is not >>>> taken care of in the generic panel driver. The impact of this needs to be >>>> tested. The panel's gpios are also not exported any more. Hence, they can't be >>>> accessed via sysfs interface. >>> >>> Indeed, there is an impact - the LCD no longer works. >>> The reason for the LCD_EN gpio being pushed high after the 50ms delay, >>> is to get the LCD out of reset, so the SPI transaction will succeed >>> and initialize the LCD. >>> Now, when you remove the gpio handling for the LCD_EN pin, >>> the LCD no longer works. >> >> So between what is the sleep done? It's not clear from the code. LCD_EN >> needs to be 0 for 50ms, or...? >> >> If the panel requires specific reset handling, does it work right even >> currently when the panel is turned off and later turned on? The msleep >> is only used at boot time. > > Okay, so I just realized there's an spi backlight driver used here, and > that backlight driver is actually handling the SPI transactions with the > panel, instead of the panel driver. So this looks quite messed up. Yep, it always was. The whole DSS specific panel handling inside the drivers/video/omap2/displays is a mess. Those panels can be (and are) used not only with OMAP based boards. > > For a quick solution, can we just set the LCD_EN at boot time (with the > msleep), and not touch it after that? That would be sensible for now, so this series can be merged. As a more appropriate (and long term) solution, I plan on moving the panel reset pin handling to the spi backlight driver itself. - -- Regards, Igor. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.17 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJRHIqCAAoJEBDE8YO64Efamw0P/R2wt0tpCI1ecrnutVGMX4bF Pyjk2B65uDWqoqZ/cpJUqnvmupXl5UdrA7eqKjTBh1A+g81UVFcNDMuJsbPIIiYI 1pimZieAq0T6Vag00PKImKlkhJYfC7JVBbESij/NONlzYtPkbZ91Y+Ik4DZXnyZf 1TS4GbHQ25tjl73PkwlzLUcJIDIogsimSrkM+aWXPE8LmvrBEQs0LhAObPsAFtgL 1An3hvA2Tkhh9QgerWQd9YiqX994tv1PGRLBEXTbjh1yihzKSNleuvw3NdM+wf9i 9Y5l9IV6L2dtYBMLCpzkiGQBDdOzoq+fObRnSgK6Kr1mfXot+MAlLrk9gCeWcq1b c2oU/imKWB4sZys21pTnjIxAIzzRDoGW40qXuibTW4DoAYaVHuEBPtphjMVCBCcQ sJaIVXpsChQ3vvtHOgllnInMjCnlXJ3Piqr4y5glTPxu9mZHdPr6VDpWdqRmvyr9 V7fRQztwXB3Td+SZVDD1yBqoXKlKCX4QPlLAqH3FI9s1WhDHcJePcoDJY0/QyXB1 IeQRlEwBBEZAYy/kr9/pwbZzXeh5V5dK6wAq8aT+thS22zl3nJbKjW//vN06+ib+ WAnHRSZ8iCbUX2cRVF1k+DCQOTi8QCbI6WTcLsgenLeSrbEuzilfgsrsvd6LHfjD oTODiiD9QInP2sBfknUp =tWsB -----END PGP SIGNATURE----- -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013-02-14 08:56, Igor Grinberg wrote: > On 02/13/13 17:59, Tomi Valkeinen wrote: >> Okay, so I just realized there's an spi backlight driver used here, and >> that backlight driver is actually handling the SPI transactions with the >> panel, instead of the panel driver. So this looks quite messed up. > > Yep, it always was. > The whole DSS specific panel handling inside the > drivers/video/omap2/displays is a mess. Well, that's not mess itself, it's just omap specific panel framework. But dividing single device handling into two separate places is a mess. > Those panels can be (and are) used not only with OMAP based boards. True, but as there's no generic panel framework, that's the best we can do. But see CDF (common display framework) discussions if you're interested in what's hopefully coming soon. >> For a quick solution, can we just set the LCD_EN at boot time (with the >> msleep), and not touch it after that? > > That would be sensible for now, so this series can be merged. > As a more appropriate (and long term) solution, > I plan on moving the panel reset pin handling to the spi backlight > driver itself. Well, if you must. But I suggest moving the whole panel handling into a (omap specific) panel driver, as it's done for other panels. That way you'll have a proper panel driver for it, for omap, and when CDF comes, you'll get a platform independent panel driver for it. Of course, if you have multiple platforms already using that backlight driver, the omap specific approach may not be enticing. So perhaps it's easier to just do the quick fix and wait for CDF. Tomi
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 02/14/13 09:09, Tomi Valkeinen wrote: > On 2013-02-14 08:56, Igor Grinberg wrote: >> On 02/13/13 17:59, Tomi Valkeinen wrote: > >>> Okay, so I just realized there's an spi backlight driver used here, and >>> that backlight driver is actually handling the SPI transactions with the >>> panel, instead of the panel driver. So this looks quite messed up. >> >> Yep, it always was. >> The whole DSS specific panel handling inside the >> drivers/video/omap2/displays is a mess. > > Well, that's not mess itself, it's just omap specific panel framework. > But dividing single device handling into two separate places is a mess. Yes, you are right it is not the mess, but it prevents the panel to be used on other systems and that is BAD. At the very least, drivers/video/backlight is a generic place that can be used not just on OMAP. And since the toppoly was and is used on other systems, why the hell should anyone duplicate the driver just to please the OMAP specific panel framework? The real problem is that this framework should not be OMAP specific... Of course I'm aware of the fact that currently there is no generic panel framework, but forging something OMAP specific which is obviously used on most of the other architectures/platforms (and I mean panel<->controller relations), is not a good way to go. Although, I'm also aware of the fact that most things are done this way: do several specific drivers/frameworks, find the common stuff, and extract it into a core driver/framework. So I don't want to blame anyone - that's just the way how we do things, right? > >> Those panels can be (and are) used not only with OMAP based boards. > > True, but as there's no generic panel framework, that's the best we can > do. But see CDF (common display framework) discussions if you're > interested in what's hopefully coming soon. Yep, I've seen the CDF discussion and I think this is a good way to go. > >>> For a quick solution, can we just set the LCD_EN at boot time (with the >>> msleep), and not touch it after that? >> >> That would be sensible for now, so this series can be merged. >> As a more appropriate (and long term) solution, >> I plan on moving the panel reset pin handling to the spi backlight >> driver itself. > > Well, if you must. But I suggest moving the whole panel handling into a > (omap specific) panel driver, as it's done for other panels. That way > you'll have a proper panel driver for it, for omap, and when CDF comes, > you'll get a platform independent panel driver for it. You can't just move generic architecture/platform independent stuff into OMAP specific framework... Just think about this... It's insane. > > Of course, if you have multiple platforms already using that backlight > driver, the omap specific approach may not be enticing. So perhaps it's > easier to just do the quick fix and wait for CDF. That is exactly what I am talking about. In addition, AFAIR, the reset pin is the property of the toppoly panel hardware, so that is why I think, we should let the toppoly driver (currently spi backlight, later hopefully CDF) handle it correctly along with the spi sequences. - -- Regards, Igor. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.17 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJRHKJGAAoJEBDE8YO64EfacksP/j79jaDbnXpFcwT9KlInp8OE e+XNi5Vt8zbqhj4gHtxZlN/eIQsVRfuivm9CTp5aJSZHBDAJlPNKobmwjFrDLO9V RtYTwLAcuWyOdnutIQ52xNwXSntQknd8yxm1qJZMEBjEP+mcQxISWXXMsdxlQEiT emNtU42W16ZOR34kHUoVfYLkV0v02/JVygt3oaU71+mrNBOt+5L6cHcXaQZPKSes LUOcyz0qJfzKbnmmZnP/+clTIids83u8rVCNZ1/JoIIlR4rvtNcxRM8Apa8KFJx/ PVT38ds62F0L0qbxL3UmI1uJS2KuEHuJyjYo0uDeQqeeSyz7Q3ZG4TwAJYkWZdWQ TdFbVrsXbK408FT33VIP4rOzDjqO93IK6f5ld0tZoIvL59NLwgXIejJn6jTNNcU4 p25mUXGSDnaZrNU5cC7d/MzSMt60XQx3UiHjEXD3eJAT33yb+DdBaQwloMCXJQOx vnseFqhuAzgFHd9LEl47LBg7eXudjaSvWYfJOV0SoB9s7QM8m/YUhnmqmtvdCqZL fKMJcAjCgm0BG2P6ss79sl6P4XDoBF1LOwSwz4dRmocA3TP7vBNkuRoK08vQe6gv Qi7hJ05ioa8THt77FxMHtf+ZrO34/L6gHxZqrOD++OgPPdL6qtegemyp4IaKKbUg q3Mpgsr4ODyStdjEXxTC =lIHm -----END PGP SIGNATURE----- -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013-02-14 10:37, Igor Grinberg wrote: > On 02/14/13 09:09, Tomi Valkeinen wrote: >> On 2013-02-14 08:56, Igor Grinberg wrote: >>> On 02/13/13 17:59, Tomi Valkeinen wrote: > >>>> Okay, so I just realized there's an spi backlight driver used here, and >>>> that backlight driver is actually handling the SPI transactions with the >>>> panel, instead of the panel driver. So this looks quite messed up. >>> >>> Yep, it always was. >>> The whole DSS specific panel handling inside the >>> drivers/video/omap2/displays is a mess. > >> Well, that's not mess itself, it's just omap specific panel framework. >> But dividing single device handling into two separate places is a mess. > > Yes, you are right it is not the mess, but it prevents the panel to > be used on other systems and that is BAD. > At the very least, drivers/video/backlight is a generic place that can be > used not just on OMAP. True, it's generic, but does it work reliably? The panel hardware is now partly handled in the backlight driver, and partly in the omap's panel driver (and wherever on other platforms). At least currently there's a dependency between the two, as the LCD_EN gpio is handled by the panel driver, which affects the functioning of the backlight driver. Is it ensured that the panel driver does not disable the panel when the backlight driver does spi transactions? That's what I meant with the mess, it's difficult to make it work reliably. I know that for some panels such a two-driver approach would not work at all. Although I guess it's working well enough for you for this panel. Thinking about it, if you do move the gpio handling to the backlight driver, the panel driver will only handle the DPI video stream. Then it should not have any effect on the SPI side (presuming the panel doesn't use the pixel clock as func clock), although there's probably still possibility for display artifacts on enable and disable, if the order of operations goes the wrong way. > And since the toppoly was and is used on other systems, why the hell > should anyone duplicate the driver just to please the OMAP specific > panel framework? The real problem is that this framework should not be Not to please. To make it reliable. > OMAP specific... > Of course I'm aware of the fact that currently there is no generic > panel framework, but forging something OMAP specific which is obviously > used on most of the other architectures/platforms (and I mean > panel<->controller relations), is not a good way to go. Well, if duplicating the code gives us reliable drivers, versus unreliable without duplicating, then... I don't see it as that bad. > Although, I'm also aware of the fact that most things are done this way: > do several specific drivers/frameworks, find the common stuff, and extract > it into a core driver/framework. So I don't want to blame anyone - that's > just the way how we do things, right? If it was easy, somebody would've done it. >>>> For a quick solution, can we just set the LCD_EN at boot time (with the >>>> msleep), and not touch it after that? >>> >>> That would be sensible for now, so this series can be merged. >>> As a more appropriate (and long term) solution, >>> I plan on moving the panel reset pin handling to the spi backlight >>> driver itself. > >> Well, if you must. But I suggest moving the whole panel handling into a >> (omap specific) panel driver, as it's done for other panels. That way >> you'll have a proper panel driver for it, for omap, and when CDF comes, >> you'll get a platform independent panel driver for it. > > You can't just move generic architecture/platform independent stuff > into OMAP specific framework... Just think about this... It's insane. As I said, reliable vs unreliable. That's not insane. But again, if you can handle this particular panel reliably with the two-driver approach, I'm fine with it. > In addition, AFAIR, the reset pin is the property of the toppoly panel > hardware, so that is why I think, we should let the toppoly driver > (currently spi backlight, later hopefully CDF) handle it correctly > along with the spi sequences. Yes, that sounds ok. Tomi
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 02/14/13 11:09, Tomi Valkeinen wrote: > On 2013-02-14 10:37, Igor Grinberg wrote: >> On 02/14/13 09:09, Tomi Valkeinen wrote: >>> On 2013-02-14 08:56, Igor Grinberg wrote: >>>> On 02/13/13 17:59, Tomi Valkeinen wrote: >> >>>>> Okay, so I just realized there's an spi backlight driver used here, and >>>>> that backlight driver is actually handling the SPI transactions with the >>>>> panel, instead of the panel driver. So this looks quite messed up. >>>> >>>> Yep, it always was. >>>> The whole DSS specific panel handling inside the >>>> drivers/video/omap2/displays is a mess. >> >>> Well, that's not mess itself, it's just omap specific panel framework. >>> But dividing single device handling into two separate places is a mess. >> >> Yes, you are right it is not the mess, but it prevents the panel to >> be used on other systems and that is BAD. >> At the very least, drivers/video/backlight is a generic place that can be >> used not just on OMAP. > > True, it's generic, but does it work reliably? The panel hardware is now > partly handled in the backlight driver, and partly in the omap's panel > driver (and wherever on other platforms). It works reliably on other platforms, but not on OMAP - because we need to cope with the OMAP specific framework... > > At least currently there's a dependency between the two, as the LCD_EN > gpio is handled by the panel driver, which affects the functioning of > the backlight driver. Is it ensured that the panel driver does not > disable the panel when the backlight driver does spi transactions? > > That's what I meant with the mess, it's difficult to make it work > reliably. I know that for some panels such a two-driver approach would > not work at all. Although I guess it's working well enough for you for > this panel. Yep, that is correct - this is the mess. > > Thinking about it, if you do move the gpio handling to the backlight > driver, the panel driver will only handle the DPI video stream. Then it > should not have any effect on the SPI side (presuming the panel doesn't > use the pixel clock as func clock), although there's probably still > possibility for display artifacts on enable and disable, if the order of > operations goes the wrong way. Yep, again, that is correct. > >> And since the toppoly was and is used on other systems, why the hell >> should anyone duplicate the driver just to please the OMAP specific >> panel framework? The real problem is that this framework should not be > > Not to please. To make it reliable. Well, there are multiple ways to make it reliable. And I don't think that the best would be: make it OMAP specific. > >> OMAP specific... >> Of course I'm aware of the fact that currently there is no generic >> panel framework, but forging something OMAP specific which is obviously >> used on most of the other architectures/platforms (and I mean >> panel<->controller relations), is not a good way to go. > > Well, if duplicating the code gives us reliable drivers, versus > unreliable without duplicating, then... I don't see it as that bad. Hmmm... I don't think this fits the mainline (upstream) philosophy. This can be also extrapolated into: let's make our own Linux ARM fork so it will be more reliable... This is the way how vendor specific kernel releases work. > >> Although, I'm also aware of the fact that most things are done this way: >> do several specific drivers/frameworks, find the common stuff, and extract >> it into a core driver/framework. So I don't want to blame anyone - that's >> just the way how we do things, right? > > If it was easy, somebody would've done it. In fact this is done all the time on multiple drivers and frameworks. Also, I don't say this is easy, but I also don't think this too hard. It is also a function of resources (time/will/experience/etc.). > >>>>> For a quick solution, can we just set the LCD_EN at boot time (with the >>>>> msleep), and not touch it after that? >>>> >>>> That would be sensible for now, so this series can be merged. >>>> As a more appropriate (and long term) solution, >>>> I plan on moving the panel reset pin handling to the spi backlight >>>> driver itself. >> >>> Well, if you must. But I suggest moving the whole panel handling into a >>> (omap specific) panel driver, as it's done for other panels. That way >>> you'll have a proper panel driver for it, for omap, and when CDF comes, >>> you'll get a platform independent panel driver for it. >> >> You can't just move generic architecture/platform independent stuff >> into OMAP specific framework... Just think about this... It's insane. > > As I said, reliable vs unreliable. That's not insane. > > But again, if you can handle this particular panel reliably with the > two-driver approach, I'm fine with it. Again, it works reliably on other platforms, why would OMAP be an exception? > >> In addition, AFAIR, the reset pin is the property of the toppoly panel >> hardware, so that is why I think, we should let the toppoly driver >> (currently spi backlight, later hopefully CDF) handle it correctly >> along with the spi sequences. > > Yes, that sounds ok. > > Tomi > - -- Regards, Igor. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.17 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJRHLGzAAoJEBDE8YO64EfaOJMP/AtR9IkN+eWwdbt0R1Vu9vKH mFgSCENqErAjYi3zM0YScj+E2zSby4rfXCY14Goh46DBCBXjRWYms6P/nZgGSLYT lIup5YljWxWJxM0nvrykok872Atx+TSJukx3nD5foWieu4tNRRvhqN7+ckBO7R4D J1D5uy3wH8Ea3SZ/foPrzewTeajnOeZxzhprfodLdmKIuqxInVE0KrWqrcefspNI TvWAjLCHtoM4LDCZRaiHs3mN03QMdcJc1BfWeJe1eVx6YXSBqNTTG6mSgUegQyOG PnC1T3kzS5Vuhmk9NfUmL19LInAljPVDoQomUqG6N140M6jol4ru+A5yE/NZ/tSl j/8vz5pE8JXp0ueQt1X1vkAGL+Lgzbyrf38xQTxnjSLggO3OFHOv6AzlY453Lfem gz6Xpjq+2Kcqxghfndd4yXnOjdlyWDN6dvYBthBBixmt34c6nNtdoXmakAXyw8wW qSyT3sO6WgE53ROZRh9W2FCiLXdJ1rHYMBRRY4nbKNhOhtC/vSF4UVsFBUuAaqul a6QToMggpugY8n3lm/SZ6LFWJDaHjnkUAVXxq3/GiclJSFwBnHqsOT1bfjsF5OfC YnaldNBbH0WvmFN89Ds/inW1MLcFc/yWirB0Utj7ysb5AL4vH4QLs1dokzjxnTyK WJmOMyQi7Jk+ocUSBgu2 =G6lL -----END PGP SIGNATURE----- -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013-02-14 11:43, Igor Grinberg wrote: >> True, it's generic, but does it work reliably? The panel hardware is now >> partly handled in the backlight driver, and partly in the omap's panel >> driver (and wherever on other platforms). > > It works reliably on other platforms, but not on OMAP - because > we need to cope with the OMAP specific framework... How do you handle the gpios on other platform? Those are the ones causing the issues here, right? Or is there something else with OMAP DSS that you need to specifically cope with? >> Thinking about it, if you do move the gpio handling to the backlight >> driver, the panel driver will only handle the DPI video stream. Then it >> should not have any effect on the SPI side (presuming the panel doesn't >> use the pixel clock as func clock), although there's probably still >> possibility for display artifacts on enable and disable, if the order of >> operations goes the wrong way. > > Yep, again, that is correct. It's correct that there may be artifacts? How do you manage the ordering of the operations on other platforms? >>> And since the toppoly was and is used on other systems, why the hell >>> should anyone duplicate the driver just to please the OMAP specific >>> panel framework? The real problem is that this framework should not be > >> Not to please. To make it reliable. > > Well, there are multiple ways to make it reliable. > And I don't think that the best would be: make it OMAP specific. I'm not saying it's the best option. I'm saying it's a realistic option to get it working. >> Well, if duplicating the code gives us reliable drivers, versus >> unreliable without duplicating, then... I don't see it as that bad. > > Hmmm... I don't think this fits the mainline (upstream) philosophy. > This can be also extrapolated into: let's make our own Linux ARM fork > so it will be more reliable... > This is the way how vendor specific kernel releases work. Well, we are talking about a smallish driver here. Not an arch fork. If the options are a) platform specific driver that works, or b) generic driver that's not reliable, or c) no driver at all, I can't really see why a) would be such a horrible option for the time being. But this discussion is getting a bit out of hand. It sounds to me that for this panel in question we can manage with the current approach, so this whole line of discussion doesn't matter for this specific problem. >> If it was easy, somebody would've done it. > > In fact this is done all the time on multiple drivers and frameworks. > Also, I don't say this is easy, but I also don't think this too hard. > It is also a function of resources (time/will/experience/etc.). I think the CDF discussions have already proven that it is quite hard. But feel free to contribute. And I've talked about a common display framework already years ago, and I've tried to design OMAP DSS panels from start in such a way that they try to depend on OMAP DSS features as little as possible, to make it easier to generalize them. Just to prove I'm not indifferent about the issue =). Tomi
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 02/14/13 12:59, Tomi Valkeinen wrote: > On 2013-02-14 11:43, Igor Grinberg wrote: > >>> True, it's generic, but does it work reliably? The panel hardware is now >>> partly handled in the backlight driver, and partly in the omap's panel >>> driver (and wherever on other platforms). >> >> It works reliably on other platforms, but not on OMAP - because >> we need to cope with the OMAP specific framework... > > How do you handle the gpios on other platform? Those are the ones > causing the issues here, right? Well, I'm also talking about something that is a history already. Remember, we had multiple panel drivers inside the video/omap2/displays and then they were consolidated into the "generic dpi/dsi/whatever". And yes you are right, on the platforms I'm aware of, the GPIO is not handled. Apparently its hardware default (pull resistor) is always on... > > Or is there something else with OMAP DSS that you need to specifically > cope with? The fact there is a need to create a OMAP specific driver. I'm not talking about the generic driver which only needs to have the controller specific data (e.g. porches, pixel clock, bus width). The generic driver was one of the good ways to go. > >>> Thinking about it, if you do move the gpio handling to the backlight >>> driver, the panel driver will only handle the DPI video stream. Then it >>> should not have any effect on the SPI side (presuming the panel doesn't >>> use the pixel clock as func clock), although there's probably still >>> possibility for display artifacts on enable and disable, if the order of >>> operations goes the wrong way. >> >> Yep, again, that is correct. > > It's correct that there may be artifacts? How do you manage the ordering > of the operations on other platforms? Yep, there might be artifacts if the ordering is incorrect. The ordering is something that should be solved by the CDF. > >>>> And since the toppoly was and is used on other systems, why the hell >>>> should anyone duplicate the driver just to please the OMAP specific >>>> panel framework? The real problem is that this framework should not be >> >>> Not to please. To make it reliable. >> >> Well, there are multiple ways to make it reliable. >> And I don't think that the best would be: make it OMAP specific. > > I'm not saying it's the best option. I'm saying it's a realistic option > to get it working. > >>> Well, if duplicating the code gives us reliable drivers, versus >>> unreliable without duplicating, then... I don't see it as that bad. >> >> Hmmm... I don't think this fits the mainline (upstream) philosophy. >> This can be also extrapolated into: let's make our own Linux ARM fork >> so it will be more reliable... >> This is the way how vendor specific kernel releases work. > > Well, we are talking about a smallish driver here. Not an arch fork. If > the options are a) platform specific driver that works, or b) generic > driver that's not reliable, or c) no driver at all, I can't really see > why a) would be such a horrible option for the time being. I think there is b.5) where the driver is both generic and reliable, but I haven't looked into this deep enough. > > But this discussion is getting a bit out of hand. It sounds to me that > for this panel in question we can manage with the current approach, so > this whole line of discussion doesn't matter for this specific problem. > >>> If it was easy, somebody would've done it. >> >> In fact this is done all the time on multiple drivers and frameworks. >> Also, I don't say this is easy, but I also don't think this too hard. >> It is also a function of resources (time/will/experience/etc.). > > I think the CDF discussions have already proven that it is quite hard. > But feel free to contribute. I will contribute as much as I can in terms of my paid work. I always do... > > And I've talked about a common display framework already years ago, and > I've tried to design OMAP DSS panels from start in such a way that they > try to depend on OMAP DSS features as little as possible, to make it > easier to generalize them. Just to prove I'm not indifferent about the > issue =). Yep, your work was fruitful and no one can take it from you! That's why I said that I'm not trying to blame or accuse anyone. That is more of a wish that the CDF will come as quickly as it can. Because currently it is clear that there are cases where we don't have a proper support... - -- Regards, Igor. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.17 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJRHNqRAAoJEBDE8YO64Efac08QAKBgU7xFkdBHU0ekAgRIafhO kd0EgPXwPZDvuAaoj5pzdMI65alRSuoQxp5ohHyz0aFfGRk0Q66f6/zXzTsGXPLP pRTkqGGTHD5Qtv2IvhHPvLMRGW+87QiuK0NU0BJqV/1JRPz1UyKMJrwc5U3Acf4J B7bQnWLlIcPftIt2zHwuvZZMYZUL8f5/dXGWMhxmTF7rse7YcTjCwQO4eabTIX5r /GUanF+qlwKJzk7nxQR1DFQdN+7Vv8vdumJi38sq9fouLgXlhmzOgXvxMgT7lq8T l+7Bg7l+E9yrHaXmWf3zjZBAk6/wBnzzrmmK5V6Gjg2PiEh6Rs5ARWPrdc/FQCpt 4bXTKEktLonHj2rya8intrPLw7lP4AQN81QsQOLeRIPGoSCcDsi4RGAFrWdXR0Xt Du9ea/liAD3+qiNIErQcTlR5zDKAIhMvWycG+ZFj0kVFUFb6p/F1TKcqVy0O7HAG SLSsrCyO8jA/UxAKQifhiUU2Hoq9a3VhQi507lbXiN0NTsk686mp/W8YJPB/di+v 44wayaE5Kyw5iawLj9WUyTpw29sOysiewp+z7XuaF3fM5lWeMy2e0/mNbe4iCcCG LYMl0NWcQn0S7okkQz5HHmCJp3L3/Se5sMfGKJRzpG0vK1DR+m8nJh0MGbEu/14C B9+3VrImRZS8te/wnPqE =BAnK -----END PGP SIGNATURE----- -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013-02-14 14:37, Igor Grinberg wrote: > On 02/14/13 12:59, Tomi Valkeinen wrote: >> On 2013-02-14 11:43, Igor Grinberg wrote: > >>>> True, it's generic, but does it work reliably? The panel hardware is now >>>> partly handled in the backlight driver, and partly in the omap's panel >>>> driver (and wherever on other platforms). >>> >>> It works reliably on other platforms, but not on OMAP - because >>> we need to cope with the OMAP specific framework... > >> How do you handle the gpios on other platform? Those are the ones >> causing the issues here, right? > > Well, I'm also talking about something that is a history already. > Remember, we had multiple panel drivers inside the > video/omap2/displays and then they were consolidated into the > "generic dpi/dsi/whatever". Sorry, I miss the point. Was that a bad thing? Didn't it simplify the task for you with simple panels? It could've been taken even further, though (see below). > And yes you are right, on the platforms I'm aware of, the GPIO is not > handled. Apparently its hardware default (pull resistor) is always on... Ok, so the simple fix of setting the GPIOs only in the board file is acceptable for now. Can the LCD_BL_GPIO be handled by the omap panel driver? Otherwise the backlight will supposedly be always on. Is it just a simple switch for the BL power, which does not affect the SPI in any way? >> Or is there something else with OMAP DSS that you need to specifically >> cope with? > > The fact there is a need to create a OMAP specific driver. > I'm not talking about the generic driver which only needs to have the > controller specific data (e.g. porches, pixel clock, bus width). > The generic driver was one of the good ways to go. Well, we could also have an even more generic driver that takes the video timings from the board file as platform data. Then all you would need to do is to define the timings in the board file, as I think is done for other platforms also. I'm not very fond of that idea, as I think hardcoded device specific data should not be given as parameters, but they should be handled by the device driver internally, as it should know that device specific data already. But, in practice, making this kind of even more generic panel driver will probably make life easier for everyone, so I think we'll have one with CDF. Tomi
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 02/14/13 14:52, Tomi Valkeinen wrote: > On 2013-02-14 14:37, Igor Grinberg wrote: >> On 02/14/13 12:59, Tomi Valkeinen wrote: >>> On 2013-02-14 11:43, Igor Grinberg wrote: >> >>>>> True, it's generic, but does it work reliably? The panel hardware is now >>>>> partly handled in the backlight driver, and partly in the omap's panel >>>>> driver (and wherever on other platforms). >>>> >>>> It works reliably on other platforms, but not on OMAP - because >>>> we need to cope with the OMAP specific framework... >> >>> How do you handle the gpios on other platform? Those are the ones >>> causing the issues here, right? >> >> Well, I'm also talking about something that is a history already. >> Remember, we had multiple panel drivers inside the >> video/omap2/displays and then they were consolidated into the >> "generic dpi/dsi/whatever". > > Sorry, I miss the point. Was that a bad thing? Didn't it simplify the > task for you with simple panels? It could've been taken even further, > though (see below). Yes it was a good thing (I have already told this below). > >> And yes you are right, on the platforms I'm aware of, the GPIO is not >> handled. Apparently its hardware default (pull resistor) is always on... > > Ok, so the simple fix of setting the GPIOs only in the board file is > acceptable for now. Yep. I also told this already in one of the previous emails. > > Can the LCD_BL_GPIO be handled by the omap panel driver? Otherwise the > backlight will supposedly be always on. Is it just a simple switch for > the BL power, which does not affect the SPI in any way? Yes, it can for now. Also, I think we should also take into account the backlight framework, including PMW. > >>> Or is there something else with OMAP DSS that you need to specifically >>> cope with? >> >> The fact there is a need to create a OMAP specific driver. >> I'm not talking about the generic driver which only needs to have the >> controller specific data (e.g. porches, pixel clock, bus width). >> The generic driver was one of the good ways to go. > > Well, we could also have an even more generic driver that takes the > video timings from the board file as platform data. Then all you would > need to do is to define the timings in the board file, as I think is > done for other platforms also. Yep. That sounds reasonable also for cases where the bus width is the hardware (board) property. > > I'm not very fond of that idea, as I think hardcoded device specific > data should not be given as parameters, but they should be handled by > the device driver internally, as it should know that device specific > data already. Yes, that is why I think the generic driver for "simple" panels was a good idea. There was some flexibility missing though. For example the resolution setting which in turn drags another set of timings and pixel clock. There are panels that support more than one resolution. > > But, in practice, making this kind of even more generic panel driver > will probably make life easier for everyone, so I think we'll have one > with CDF. I'm looking forward to see it happening! - -- Regards, Igor. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.17 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJRHOvEAAoJEBDE8YO64EfaCZ0P/32SYC7MZRJMfUVrnzZtJZpn 9BKzrvO0h/GKZMbKoKKNFwmvlTxEocELLkljF35ipbc51C/dpD45ZmMBvu4s8owE 6bopGw6ssTahTKk0zY5PekTamZT7UlY86hI9ZcZBxSzY8xaj7UCSPnJ3qzY2ZGzA 4heJW01mlHr9i1qkOzxz0IHA2CdQmQltAsW12NFCWYBRpDfhrYtECwhlnvLXHEzy nqXNpRHOnrL/hqvJwor1X+D/O1JlyxUiVr6+7sAZqXxvv/iMX8Nc1XeTObgGbse6 1bpipPD57etqISsN1g9ur1tU5f6KvoR4IA35RaCCkBFINIXMEBl7kr5X9Bcg3giE 3pz23k91KRloOcXJ0OvLHp7RnSrwswU/4C3Cse+95cY0wyChaVlwJtySEnfGALWV aiuw89v6DXaC5WDQq55UtTc3qjc23Ehut8i184PAv5HKrDHLLjVg4HqgGjC9uGEn JKOxOnfMTstqyKC2whW+Pep3g4npJAHsn/0QmpdSJQ/vEwm7CmXBZeR6DwcTSLAT xR1SUWwHav2HqpYUz4y7TgIPuwsR+VNKn/mSOTbhbLB4s9bowPMxMiqz2AQn3ige YyMTo7KnObivXZydrVrx0/e/DJC4ENGpE3macVQytr0BpjJhK1+XiNMHa17+Mymm U45VDKrUgaYcYXW2L4JN =xeSz -----END PGP SIGNATURE----- -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-omap2/board-cm-t35.c b/arch/arm/mach-omap2/board-cm-t35.c index f940a79..563d5ab 100644 --- a/arch/arm/mach-omap2/board-cm-t35.c +++ b/arch/arm/mach-omap2/board-cm-t35.c @@ -189,32 +189,6 @@ static inline void cm_t35_init_nand(void) {} #define CM_T35_LCD_BL_GPIO 58 #define CM_T35_DVI_EN_GPIO 54 -static int lcd_enabled; -static int dvi_enabled; - -static int cm_t35_panel_enable_lcd(struct omap_dss_device *dssdev) -{ - if (dvi_enabled) { - printk(KERN_ERR "cannot enable LCD, DVI is enabled\n"); - return -EINVAL; - } - - gpio_set_value(CM_T35_LCD_EN_GPIO, 1); - gpio_set_value(CM_T35_LCD_BL_GPIO, 1); - - lcd_enabled = 1; - - return 0; -} - -static void cm_t35_panel_disable_lcd(struct omap_dss_device *dssdev) -{ - lcd_enabled = 0; - - gpio_set_value(CM_T35_LCD_BL_GPIO, 0); - gpio_set_value(CM_T35_LCD_EN_GPIO, 0); -} - static int cm_t35_panel_enable_tv(struct omap_dss_device *dssdev) { return 0; @@ -226,8 +200,11 @@ static void cm_t35_panel_disable_tv(struct omap_dss_device *dssdev) static struct panel_generic_dpi_data lcd_panel = { .name = "toppoly_tdo35s", - .platform_enable = cm_t35_panel_enable_lcd, - .platform_disable = cm_t35_panel_disable_lcd, + .num_gpios = 2, + .gpios = { + CM_T35_LCD_BL_GPIO, + CM_T35_LCD_EN_GPIO + }, }; static struct omap_dss_device cm_t35_lcd_device = { @@ -303,19 +280,6 @@ static void __init cm_t35_init_display(void) spi_register_board_info(cm_t35_lcd_spi_board_info, ARRAY_SIZE(cm_t35_lcd_spi_board_info)); - err = gpio_request_array(cm_t35_dss_gpios, - ARRAY_SIZE(cm_t35_dss_gpios)); - if (err) { - pr_err("CM-T35: failed to request DSS control GPIOs\n"); - return; - } - - gpio_export(CM_T35_LCD_EN_GPIO, 0); - gpio_export(CM_T35_LCD_BL_GPIO, 0); - - msleep(50); - gpio_set_value(CM_T35_LCD_EN_GPIO, 1); - err = omap_display_init(&cm_t35_dss_data); if (err) { pr_err("CM-T35: failed to register DSS device\n");
The cm-t35 board file currently requests gpios required to configure the tdo35s panel, and provides platform_enable/disable callbacks to configure them. These tasks have been moved to the generic dpi panel driver itself and shouldn't be done in the board files. Remove the gpio requests and the platform callbacks from the board file. Add the gpio information to generic dpi panel's platform data so that it's passed to the panel driver. Note: In cm_t35_init_display(), the gpios were disabled, and the LCD_EN gpio was enabled after a 50 millisecond delay. This code has been removed and is not taken care of in the generic panel driver. The impact of this needs to be tested. The panel's gpios are also not exported any more. Hence, they can't be accessed via sysfs interface. Cc: Tony Lindgren <tony@atomide.com> Signed-off-by: Archit Taneja <archit@ti.com> --- arch/arm/mach-omap2/board-cm-t35.c | 46 ++++-------------------------------- 1 file changed, 5 insertions(+), 41 deletions(-)