| Message ID | 1359514939-15653-2-git-send-email-acourbot@nvidia.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 01/30/2013 11:02 AM, Alexandre Courbot wrote: > Add support for the Chunghwa CLAA101WA01A display panel. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > --- [...] > + > +#include <video/display.h> > + > +#define CLAA101WA01A_WIDTH 223 > +#define CLAA101WA01A_HEIGHT 125 If I remember correct, the physical size of the panel can be fetched in EDID. If this is correct, we don't have to hard-code here. > + > +struct panel_claa101 { > + struct display_entity entity; > + struct regulator *vdd_pnl; > + struct regulator *vdd_bl; > + /* Enable GPIOs */ > + int pnl_enable; > + int bl_enable; > +}; > + > +#define to_panel_claa101(p) container_of(p, struct panel_claa101, entity) > + > +static int panel_claa101_set_state(struct display_entity *entity, > + enum display_entity_state state) > +{ > + struct panel_claa101 *panel = to_panel_claa101(entity); > + > + /* OFF and STANDBY are equivalent to us */ > + if (state == DISPLAY_ENTITY_STATE_STANDBY) > + state = DISPLAY_ENTITY_STATE_OFF; Do we need this? The "switch" below handles the same thing already. > + > + switch (state) { > + case DISPLAY_ENTITY_STATE_OFF: > + case DISPLAY_ENTITY_STATE_STANDBY: > + if (entity->source) > + display_entity_set_stream(entity->source, > + DISPLAY_ENTITY_STREAM_STOPPED); > + > + /* TODO error checking? */ > + gpio_set_value_cansleep(panel->bl_enable, 0); > + usleep_range(10000, 10000); > + regulator_disable(panel->vdd_bl); > + usleep_range(200000, 200000); > + gpio_set_value_cansleep(panel->pnl_enable, 0); > + regulator_disable(panel->vdd_pnl); > + break; > + > + case DISPLAY_ENTITY_STATE_ON: > + regulator_enable(panel->vdd_pnl); > + gpio_set_value_cansleep(panel->pnl_enable, 1); > + usleep_range(200000, 200000); > + regulator_enable(panel->vdd_bl); > + usleep_range(10000, 10000); > + gpio_set_value_cansleep(panel->bl_enable, 1); > + > + if (entity->source) > + display_entity_set_stream(entity->source, > + DISPLAY_ENTITY_STREAM_CONTINUOUS); > + break; > + } > + > + return 0; > +} > + > +static int panel_claa101_get_modes(struct display_entity *entity, > + const struct videomode **modes) > +{ > + /* TODO get modes from EDID? */ Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that case, you can get EDID here. I know drm has some helpers to fetch EDID but I recall there are some other functions which has no drm dependencies which may be suitable for you. > + return 0; > +} [...] > + > +static int __exit panel_claa101_remove(struct platform_device *pdev) > +{ > + struct panel_claa101 *panel = platform_get_drvdata(pdev); > + > + display_entity_unregister(&panel->entity); > + > + return 0; > +} > + > +#ifdef CONFIG_OF We don't need this kind of "ifdef" in upstream kernel. > +static struct of_device_id panel_claa101_of_match[] = { > + { .compatible = "chunghwa,claa101wa01a", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, pwm_backlight_of_match); What does this mean? Why we need this? > +#else > +#endif > + > +static const struct dev_pm_ops panel_claa101_dev_pm_ops = { > +}; > + > +static struct platform_driver panel_claa101_driver = { > + .probe = panel_claa101_probe, > + .remove = panel_claa101_remove, > + .driver = { > + .name = "panel_claa101wa01a", > + .owner = THIS_MODULE, > +#ifdef CONFIG_PM > + .pm = &panel_claa101_dev_pm_ops, If you haven't implemented this in this patch set, I suggest removing this. > +#endif > +#ifdef CONFIG_OF > + .of_match_table = of_match_ptr(panel_claa101_of_match), > +#endif > + }, > +}; > + > +module_platform_driver(panel_claa101_driver); > + > +MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>"); > +MODULE_DESCRIPTION("Chunghwa CLAA101WA01A Display Panel"); > +MODULE_LICENSE("GPL"); > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/30/2013 04:20 PM, Mark Zhang wrote: >> + /* OFF and STANDBY are equivalent to us */ >> + if (state == DISPLAY_ENTITY_STATE_STANDBY) >> + state = DISPLAY_ENTITY_STATE_OFF; > > Do we need this? The "switch" below handles the same thing already. Indeed, I have rewritten this part actually. >> +static int panel_claa101_get_modes(struct display_entity *entity, >> + const struct videomode **modes) >> +{ >> + /* TODO get modes from EDID? */ > > Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that > case, you can get EDID here. I know drm has some helpers to fetch EDID > but I recall there are some other functions which has no drm > dependencies which may be suitable for you. I explained this in the cover letter - I'm not sure we want to have a dependency on DRM here, as CDF entities could also be connected to other subsystems. That's something we need to figure out. But yes, ultimately this should be the place where EDID is retrieved. >> +static struct of_device_id panel_claa101_of_match[] = { >> + { .compatible = "chunghwa,claa101wa01a", }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(of, pwm_backlight_of_match); > > What does this mean? Why we need this? Well, now you know where I copy my code from. :) Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 30, 2013 at 04:27:11PM +0900, Alex Courbot wrote: > On 01/30/2013 04:20 PM, Mark Zhang wrote: [...] > >>+static int panel_claa101_get_modes(struct display_entity *entity, > >>+ const struct videomode **modes) > >>+{ > >>+ /* TODO get modes from EDID? */ > > > >Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that > >case, you can get EDID here. I know drm has some helpers to fetch EDID > >but I recall there are some other functions which has no drm > >dependencies which may be suitable for you. > > I explained this in the cover letter - I'm not sure we want to have > a dependency on DRM here, as CDF entities could also be connected to > other subsystems. That's something we need to figure out. But yes, > ultimately this should be the place where EDID is retrieved. I already said this in another thread. I think we should only be using the CDF .get_modes() for static modes that cannot be obtained from EDID. Thinking about it, I'm not quite sure why EDID would be needed for this kind of panel anyway. Ventana probably has it because it keeps us from having to hardcode things, but if we provide drivers for the panel anyway, we can just as well hard-code the supported mode(s) in those drivers. What I'm trying to say is that the existence of EDID is board-specific, so boards other than Ventana may not have an EDID EEPROM. Or perhaps this particular display has the EEPROM integrated? Even in that case, some boards may use this panel and simply not connect the EEPROM to an I2C controller. Thierry
On 01/30/2013 03:48 PM, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Wed, Jan 30, 2013 at 04:27:11PM +0900, Alex Courbot wrote: >> On 01/30/2013 04:20 PM, Mark Zhang wrote: > [...] >>>> +static int panel_claa101_get_modes(struct display_entity *entity, >>>> + const struct videomode **modes) >>>> +{ >>>> + /* TODO get modes from EDID? */ >>> >>> Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that >>> case, you can get EDID here. I know drm has some helpers to fetch EDID >>> but I recall there are some other functions which has no drm >>> dependencies which may be suitable for you. >> >> I explained this in the cover letter - I'm not sure we want to have >> a dependency on DRM here, as CDF entities could also be connected to >> other subsystems. That's something we need to figure out. But yes, >> ultimately this should be the place where EDID is retrieved. > > I already said this in another thread. I think we should only be using > the CDF .get_modes() for static modes that cannot be obtained from EDID. > Thinking about it, I'm not quite sure why EDID would be needed for this > kind of panel anyway. Ventana probably has it because it keeps us from > having to hardcode things, but if we provide drivers for the panel > anyway, we can just as well hard-code the supported mode(s) in those > drivers. I don't think so. I think it's better that we only have one entry for getting video modes. Since CDF already provides ".get_modes" for us, we can rely on that. And according to my understanding, get video modes in panel driver makes sense than getting it in crtc. In this way, panel driver may get video modes from EDID or from hard-coded display timing infos, but other modules such as crtc don't need to care about that. Mark > > What I'm trying to say is that the existence of EDID is board-specific, > so boards other than Ventana may not have an EDID EEPROM. Or perhaps > this particular display has the EEPROM integrated? Even in that case, > some boards may use this panel and simply not connect the EEPROM to an > I2C controller. > > Thierry > > * Unknown Key > * 0x7F3EB3A1 > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/30/2013 04:48 PM, Thierry Reding wrote: > I already said this in another thread. I think we should only be using > the CDF .get_modes() for static modes that cannot be obtained from EDID. > Thinking about it, I'm not quite sure why EDID would be needed for this > kind of panel anyway. Ventana probably has it because it keeps us from > having to hardcode things, but if we provide drivers for the panel > anyway, we can just as well hard-code the supported mode(s) in those > drivers. > > What I'm trying to say is that the existence of EDID is board-specific, > so boards other than Ventana may not have an EDID EEPROM. Or perhaps > this particular display has the EEPROM integrated? Even in that case, > some boards may use this panel and simply not connect the EEPROM to an > I2C controller. Actually this display has its own EEPROM and pins for the I2C bus. That's why it would seems out-of-place to have EDID taking place outside its driver. But you are right that we should also handle the case where the I2C bus is not connected and provide a static list of videomodes (that could be an "offline" dump of the EDID data). However, the driver could take the decision to use it if the EDID bus is not specified in the DT or if transfer failed for some reason. But then as you mention we might as well save time and directly serve that offline version, since we know the content of the EEPROM anyway. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/30/2013 12:20 AM, Mark Zhang wrote: > On 01/30/2013 11:02 AM, Alexandre Courbot wrote: >> Add support for the Chunghwa CLAA101WA01A display panel. >> +static int panel_claa101_get_modes(struct display_entity *entity, >> + const struct videomode **modes) >> +{ >> + /* TODO get modes from EDID? */ > > Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that > case, you can get EDID here. I know drm has some helpers to fetch EDID > but I recall there are some other functions which has no drm > dependencies which may be suitable for you. DDC access is a property of the display controller, not the panel itself. The panel might be hooked up to a display controller's DDC/I2C channel as the target, but it isn't the host/controller of the DDC/I2C channel. As such, placing the nvidia,ddc property into the display controller node makes sense. Re: the other discussion in this thread: Probably the simplest is if tegradrm/other-CDF-users do something like: 1) If DDC I2C channel available for this display channel, query DDC. 2) If not, or perhaps also if that fails, query the panel driver for the mode list. That would cover all bases very simply. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/29/2013 08:02 PM, Alexandre Courbot wrote: > Add support for the Chunghwa CLAA101WA01A display panel. > diff --git a/Documentation/devicetree/bindings/video/display/chunghwa,claa101wa01a.txt b/Documentation/devicetree/bindings/video/display/chunghwa,claa101wa01a.txt > +Chunghwa CLAA101WA01A Display Panel > + > +Required properties: > +- compatible: "chunghwa,claa101wa01a" > +- pnl-supply: regulator controlling power supply to the panel > +- bl-supply: regulator controlling power supply to the backlight > +- pnl-enable-gpios: GPIO that enables the panel > +- bl-enable-gpios: GPIO that enables the backlight So this looks like a reasonable binding to me. The one issue is that it's very generic, and if we go this route, we'll probably end up with tens or hundreds of identical or extremely similar simple bindings, and associated drivers. We can avoid this instead by defining something like a "simple-lcd" binding, and associated driver, that will work for perhaps 90%, 95%, 99%, even 100%(?) of panels. Just like the above, but with: compatible="simple-panel", "chunghwa,claa101wa01a" instead, and the driver binding to "simple-panel" rather than "chunghwa,claa101wa01a". The driver can assume that a specific set of supplies (and perhaps GPIOs) is always present, and that the /sequence/ of manipulating those is fixed. This will avoid the need for anything like the power sequences code. If a particular panel doesn't fit those assumptions, including the exact sequence of manipulations for each state transition (which should be documented in the binding) then it can get a custom driver, this also avoiding having to define custom sequences in DT. Things that might be parameterized/optional: * Perhaps some GPIOs aren't always present. * If some regulators aren't SW-controllable, DT should still provide a fixed/dummy regulator so the driver doesn't care. * Wait times between regulator/GPIO/... manipulation could be specified in DT. * For panels without EDID, CDF DT bindings can provide the list of supported modes, otherwise we assume that the display controller that drives the panel has been told how to access the EDID, just like it would for an "external" display. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/29/2013 08:02 PM, Alexandre Courbot wrote:
> Add support for the Chunghwa CLAA101WA01A display panel.
Since this defines a new DT binding, especially since it's for a new
class of device, this patch should be Cc:
devicetree-discuss@lists.ozlabs.org too, next time you post.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/31/2013 04:19 AM, Stephen Warren wrote: > On 01/30/2013 12:20 AM, Mark Zhang wrote: >> On 01/30/2013 11:02 AM, Alexandre Courbot wrote: >>> Add support for the Chunghwa CLAA101WA01A display panel. > >>> +static int panel_claa101_get_modes(struct display_entity *entity, >>> + const struct videomode **modes) >>> +{ >>> + /* TODO get modes from EDID? */ >> >> Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that >> case, you can get EDID here. I know drm has some helpers to fetch EDID >> but I recall there are some other functions which has no drm >> dependencies which may be suitable for you. > > DDC access is a property of the display controller, not the panel > itself. The panel might be hooked up to a display controller's DDC/I2C > channel as the target, but it isn't the host/controller of the DDC/I2C > channel. As such, placing the nvidia,ddc property into the display > controller node makes sense. > Yes, DC triggers the DDC access and is the host of the DDC/I2C channel. So I think it's reasonable to put nvidia,ddc property into the display controller node. But the video mode info in EDID which be fetched via DDC is the property of the panel, so this info should be provided by panel driver. Actually display controller cares about the video modes, not the way to get these info. So I think it's better to do the whole work like this: 1) display controller relied on CDF to call "display_entity_get_modes" 2) panel driver calls the function which is hooked to display controller to get the video modes via DDC. 3) if the video modes can't be fetched via DDC, panel driver provides the info(either by hard-coded or defined in DT) anyway > Re: the other discussion in this thread: Probably the simplest is if > tegradrm/other-CDF-users do something like: > > 1) If DDC I2C channel available for this display channel, query DDC. Just like I said above, display controller should not query DDC directly. Since CDF already defines these for us, display controller should call CDF's functions, like: display_entity_get_modes, display_entity_get_size... I think this is the key difference I wanna talk about. And also in this way, display controller doesn't need to care about this 2 steps logics, just call "display_entity_get_modes" is OK. Mark > 2) If not, or perhaps also if that fails, query the panel driver for the > mode list. > > That would cover all bases very simply. > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 31, 2013 at 5:27 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > So this looks like a reasonable binding to me. The one issue is that > it's very generic, and if we go this route, we'll probably end up with > tens or hundreds of identical or extremely similar simple bindings, and > associated drivers. > > We can avoid this instead by defining something like a "simple-lcd" > binding, and associated driver, that will work for perhaps 90%, 95%, > 99%, even 100%(?) of panels. That seems totally doable indeed. Actually the right way to do this might be by extending the simple DPI panel driver Laurent included in his patchset. > Just like the above, but with: > > compatible="simple-panel", "chunghwa,claa101wa01a" > > instead, and the driver binding to "simple-panel" rather than > "chunghwa,claa101wa01a". Just out of curiosity, why don't we rather define compatible="chunghwa,claa101wa01a", "simple-panel" in that order? I thought DT compatible strings should go from more to less specific. The device would still bind to "simple-panel" if no more specific driver exists. > The driver can assume that a specific set of supplies (and perhaps > GPIOs) is always present, and that the /sequence/ of manipulating those > is fixed. This will avoid the need for anything like the power sequences > code. If a particular panel doesn't fit those assumptions, including the > exact sequence of manipulations for each state transition (which should > be documented in the binding) then it can get a custom driver, this also > avoiding having to define custom sequences in DT. > > Things that might be parameterized/optional: > > * Perhaps some GPIOs aren't always present. > * If some regulators aren't SW-controllable, DT should still provide a > fixed/dummy regulator so the driver doesn't care. How about making all regulators and GPIO optional in the driver? > * Wait times between regulator/GPIO/... manipulation could be specified > in DT. > * For panels without EDID, CDF DT bindings can provide the list of > supported modes, otherwise we assume that the display controller that > drives the panel has been told how to access the EDID, just like it > would for an "external" display. Excellent. Thanks for the feedback. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 31, 2013 at 12:51 PM, Mark Zhang <nvmarkzhang@gmail.com> wrote: >> DDC access is a property of the display controller, not the panel >> itself. The panel might be hooked up to a display controller's DDC/I2C >> channel as the target, but it isn't the host/controller of the DDC/I2C >> channel. As such, placing the nvidia,ddc property into the display >> controller node makes sense. >> > > Yes, DC triggers the DDC access and is the host of the DDC/I2C channel. > So I think it's reasonable to put nvidia,ddc property into the display > controller node. But the video mode info in EDID which be fetched via > DDC is the property of the panel, so this info should be provided by > panel driver. Actually display controller cares about the video modes, > not the way to get these info. So I think it's better to do the whole > work like this: > > 1) display controller relied on CDF to call "display_entity_get_modes" > 2) panel driver calls the function which is hooked to display controller > to get the video modes via DDC. > 3) if the video modes can't be fetched via DDC, panel driver provides > the info(either by hard-coded or defined in DT) anyway This would require a callback dedicated to this in display_entity and would break its simple design. > Just like I said above, display controller should not query DDC > directly. Since CDF already defines these for us, display controller > should call CDF's functions, like: display_entity_get_modes, > display_entity_get_size... I think this is the key difference I wanna > talk about. And also in this way, display controller doesn't need to > care about this 2 steps logics, just call "display_entity_get_modes" is OK. Well the fact is that it *is* the display controller that queries DDC directly. The panel is just a bus here, and the EDID EEPROM could perfectly work without it. If you model what you described with DT nodes, I'm afraid you will end up with something both complex (with DC node referencing panel node and back again for the EDID bus) and that does not picture reality accurately. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/31/2013 12:24 PM, Alexandre Courbot wrote: > On Thu, Jan 31, 2013 at 12:51 PM, Mark Zhang <nvmarkzhang@gmail.com> wrote: >>> DDC access is a property of the display controller, not the panel >>> itself. The panel might be hooked up to a display controller's DDC/I2C >>> channel as the target, but it isn't the host/controller of the DDC/I2C >>> channel. As such, placing the nvidia,ddc property into the display >>> controller node makes sense. >>> >> >> Yes, DC triggers the DDC access and is the host of the DDC/I2C channel. >> So I think it's reasonable to put nvidia,ddc property into the display >> controller node. But the video mode info in EDID which be fetched via >> DDC is the property of the panel, so this info should be provided by >> panel driver. Actually display controller cares about the video modes, >> not the way to get these info. So I think it's better to do the whole >> work like this: >> >> 1) display controller relied on CDF to call "display_entity_get_modes" >> 2) panel driver calls the function which is hooked to display controller >> to get the video modes via DDC. >> 3) if the video modes can't be fetched via DDC, panel driver provides >> the info(either by hard-coded or defined in DT) anyway > > This would require a callback dedicated to this in display_entity and > would break its simple design. We just need to add a function pointer param which can be used by panel driver in "display_entity_get_modes", don't we? > >> Just like I said above, display controller should not query DDC >> directly. Since CDF already defines these for us, display controller >> should call CDF's functions, like: display_entity_get_modes, >> display_entity_get_size... I think this is the key difference I wanna >> talk about. And also in this way, display controller doesn't need to >> care about this 2 steps logics, just call "display_entity_get_modes" is OK. > > Well the fact is that it *is* the display controller that queries DDC > directly. The panel is just a bus here, and the EDID EEPROM could > perfectly work without it. If you model what you described with DT > nodes, I'm afraid you will end up with something both complex (with DC > node referencing panel node and back again for the EDID bus) and that > does not picture reality accurately. Display controller don't know whether the panel has EDID EEPROM but the panel driver knows. So why we need to make display controller queries EDID blindly? Since panel driver knows about all "video modes/panel size" stuffs, why not we let it handle all these things? > > Alex. > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 31, 2013 at 1:54 PM, Mark Zhang <nvmarkzhang@gmail.com> wrote: > Display controller don't know whether the panel has EDID EEPROM but the > panel driver knows. So why we need to make display controller queries > EDID blindly? Since panel driver knows about all "video modes/panel > size" stuffs, why not we let it handle all these things? The DC actually does know whether the connected panel supports EDID - in this case, the output node will have a property for the DDC bus, e.g. nvidia,ddc-i2c-bus = <&lcd_ddc>; If no DDC bus is specified or if transfer fails, the DC driver can still query the panel driver for hardcoded modes. Also passing a function pointer to get_modes() does not relief the DC driver from probing for the DDC bus. You will still have to handle both conditions (DDC bus present or not), the check will just be moved into your callback function. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/31/2013 02:36 PM, Alexandre Courbot wrote: > On Thu, Jan 31, 2013 at 1:54 PM, Mark Zhang <nvmarkzhang@gmail.com> wrote: >> Display controller don't know whether the panel has EDID EEPROM but the >> panel driver knows. So why we need to make display controller queries >> EDID blindly? Since panel driver knows about all "video modes/panel >> size" stuffs, why not we let it handle all these things? > > The DC actually does know whether the connected panel supports EDID - > in this case, the output node will have a property for the DDC bus, > e.g. > > nvidia,ddc-i2c-bus = <&lcd_ddc>; > > If no DDC bus is specified or if transfer fails, the DC driver can > still query the panel driver for hardcoded modes. > Strictly speaking, according to my understanding, "nvidia,ddc-i2c-bus" means tegra has a hardware ddc channel, this doesn't mean we can get the EDID from this DDC surely. > Also passing a function pointer to get_modes() does not relief the DC > driver from probing for the DDC bus. You will still have to handle > both conditions (DDC bus present or not), the check will just be moved > into your callback function. > Yes. But panel driver decides whether the DDC probing is needed. If the panel has an EEPROM, it can call the function which display controller provides to do a DDC probing, it not, panel driver just ignores this function pointer and return the hardcoded modes directly. > Alex. > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/30/2013 08:51 PM, Mark Zhang wrote: > On 01/31/2013 04:19 AM, Stephen Warren wrote: >> On 01/30/2013 12:20 AM, Mark Zhang wrote: >>> On 01/30/2013 11:02 AM, Alexandre Courbot wrote: >>>> Add support for the Chunghwa CLAA101WA01A display panel. >> >>>> +static int panel_claa101_get_modes(struct display_entity *entity, >>>> + const struct videomode **modes) >>>> +{ >>>> + /* TODO get modes from EDID? */ >>> >>> Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that >>> case, you can get EDID here. I know drm has some helpers to fetch EDID >>> but I recall there are some other functions which has no drm >>> dependencies which may be suitable for you. >> >> DDC access is a property of the display controller, not the panel >> itself. The panel might be hooked up to a display controller's DDC/I2C >> channel as the target, but it isn't the host/controller of the DDC/I2C >> channel. As such, placing the nvidia,ddc property into the display >> controller node makes sense. > > Yes, DC triggers the DDC access and is the host of the DDC/I2C channel. > So I think it's reasonable to put nvidia,ddc property into the display > controller node. But the video mode info in EDID which be fetched via > DDC is the property of the panel, so this info should be provided by > panel driver. No, that makes absolutely no sense at all in the EDID case. By the same argument, we'd need a panel driver for every external monitor which implemented EDID, just to transfer the EDID results from the display controller's DDC channel into the panel driver and back into the display controller code, which wants the mode list. Again, if the mode list is coming from DDC, the display controller should retrieve it in exactly the same way it retrieves it for any external monitor - by direct control of the DDC channel to read the EDID. The only time it makes sense for the panel driver to get involved in supplying the mode list is when there's no EDID, so the list must be hard-coded into the driver. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/30/2013 09:14 PM, Alexandre Courbot wrote: > On Thu, Jan 31, 2013 at 5:27 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> So this looks like a reasonable binding to me. The one issue is that >> it's very generic, and if we go this route, we'll probably end up with >> tens or hundreds of identical or extremely similar simple bindings, and >> associated drivers. >> >> We can avoid this instead by defining something like a "simple-lcd" >> binding, and associated driver, that will work for perhaps 90%, 95%, >> 99%, even 100%(?) of panels. > > That seems totally doable indeed. Actually the right way to do this > might be by extending the simple DPI panel driver Laurent included in > his patchset. > >> Just like the above, but with: >> >> compatible="simple-panel", "chunghwa,claa101wa01a" >> >> instead, and the driver binding to "simple-panel" rather than >> "chunghwa,claa101wa01a". > > Just out of curiosity, why don't we rather define > > compatible="chunghwa,claa101wa01a", "simple-panel" > > in that order? I thought DT compatible strings should go from more to > less specific. The device would still bind to "simple-panel" if no > more specific driver exists. Yes, that's correct; I "typo"d the example I gave. >> The driver can assume that a specific set of supplies (and perhaps >> GPIOs) is always present, and that the /sequence/ of manipulating those >> is fixed. This will avoid the need for anything like the power sequences >> code. If a particular panel doesn't fit those assumptions, including the >> exact sequence of manipulations for each state transition (which should >> be documented in the binding) then it can get a custom driver, this also >> avoiding having to define custom sequences in DT. >> >> Things that might be parameterized/optional: >> >> * Perhaps some GPIOs aren't always present. >> * If some regulators aren't SW-controllable, DT should still provide a >> fixed/dummy regulator so the driver doesn't care. > > How about making all regulators and GPIO optional in the driver? The general feedback I've received is that regulators should always be present, and simply implemented by fixed/dummy regulators if missing in a particular board design, rather than having the driver code handle them being optional. This certainly does simplify the driver, since it can always unconditionally program the regulator irrespective of whether it's fixed/dummy or not. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/31/2013 12:30 AM, Mark Zhang wrote: > On 01/31/2013 02:36 PM, Alexandre Courbot wrote: >> On Thu, Jan 31, 2013 at 1:54 PM, Mark Zhang <nvmarkzhang@gmail.com> wrote: >>> Display controller don't know whether the panel has EDID EEPROM but the >>> panel driver knows. So why we need to make display controller queries >>> EDID blindly? Since panel driver knows about all "video modes/panel >>> size" stuffs, why not we let it handle all these things? >> >> The DC actually does know whether the connected panel supports EDID - >> in this case, the output node will have a property for the DDC bus, >> e.g. >> >> nvidia,ddc-i2c-bus = <&lcd_ddc>; >> >> If no DDC bus is specified or if transfer fails, the DC driver can >> still query the panel driver for hardcoded modes. > > Strictly speaking, according to my understanding, "nvidia,ddc-i2c-bus" > means tegra has a hardware ddc channel, this doesn't mean we can get the > EDID from this DDC surely. Having a DDC bus by definition means that the EDID can be queried through it. If you can't query the EDID, there is no DDC bus, and hence that property is not present in the DT. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/01/2013 01:20 AM, Stephen Warren wrote: > On 01/30/2013 08:51 PM, Mark Zhang wrote: >> On 01/31/2013 04:19 AM, Stephen Warren wrote: >>> On 01/30/2013 12:20 AM, Mark Zhang wrote: >>>> On 01/30/2013 11:02 AM, Alexandre Courbot wrote: >>>>> Add support for the Chunghwa CLAA101WA01A display panel. >>> >>>>> +static int panel_claa101_get_modes(struct display_entity *entity, >>>>> + const struct videomode **modes) >>>>> +{ >>>>> + /* TODO get modes from EDID? */ >>>> >>>> Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that >>>> case, you can get EDID here. I know drm has some helpers to fetch EDID >>>> but I recall there are some other functions which has no drm >>>> dependencies which may be suitable for you. >>> >>> DDC access is a property of the display controller, not the panel >>> itself. The panel might be hooked up to a display controller's DDC/I2C >>> channel as the target, but it isn't the host/controller of the DDC/I2C >>> channel. As such, placing the nvidia,ddc property into the display >>> controller node makes sense. >> >> Yes, DC triggers the DDC access and is the host of the DDC/I2C channel. >> So I think it's reasonable to put nvidia,ddc property into the display >> controller node. But the video mode info in EDID which be fetched via >> DDC is the property of the panel, so this info should be provided by >> panel driver. > > No, that makes absolutely no sense at all in the EDID case. > > By the same argument, we'd need a panel driver for every external > monitor which implemented EDID, just to transfer the EDID results from > the display controller's DDC channel into the panel driver and back into > the display controller code, which wants the mode list. > Ah, yes, this is right. We can't write driver for every external monitor which implements EDID. Although I think it's more reasonable that panel decides whether the DDC probe is necessary. > Again, if the mode list is coming from DDC, the display controller > should retrieve it in exactly the same way it retrieves it for any > external monitor - by direct control of the DDC channel to read the > EDID. The only time it makes sense for the panel driver to get involved > in supplying the mode list is when there's no EDID, so the list must be > hard-coded into the driver. > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" 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/Documentation/devicetree/bindings/video/display/chunghwa,claa101wa01a.txt b/Documentation/devicetree/bindings/video/display/chunghwa,claa101wa01a.txt new file mode 100644 index 0000000..cfdc7fd --- /dev/null +++ b/Documentation/devicetree/bindings/video/display/chunghwa,claa101wa01a.txt @@ -0,0 +1,8 @@ +Chunghwa CLAA101WA01A Display Panel + +Required properties: +- compatible: "chunghwa,claa101wa01a" +- pnl-supply: regulator controlling power supply to the panel +- bl-supply: regulator controlling power supply to the backlight +- pnl-enable-gpios: GPIO that enables the panel +- bl-enable-gpios: GPIO that enables the backlight diff --git a/drivers/video/display/Kconfig b/drivers/video/display/Kconfig index 9ca2e60..6902abb 100644 --- a/drivers/video/display/Kconfig +++ b/drivers/video/display/Kconfig @@ -32,4 +32,12 @@ config DISPLAY_PANEL_R61517 If you are in doubt, say N. +config DISPLAY_PANEL_CLAA101WA01A + tristate "Chunghwa CLAA101WA01A Display Panel" + select BACKLIGHT_PWM + ---help--- + Support for the Chunghwa CLAA101WA01A Display Panel. + + If you are in doubt, say N. + endif # DISPLAY_CORE diff --git a/drivers/video/display/Makefile b/drivers/video/display/Makefile index ec557a1..19084a2 100644 --- a/drivers/video/display/Makefile +++ b/drivers/video/display/Makefile @@ -2,3 +2,4 @@ obj-$(CONFIG_DISPLAY_CORE) += display-core.o obj-$(CONFIG_DISPLAY_PANEL_DPI) += panel-dpi.o obj-$(CONFIG_DISPLAY_PANEL_R61505) += panel-r61505.o obj-$(CONFIG_DISPLAY_PANEL_R61517) += panel-r61517.o +obj-$(CONFIG_DISPLAY_PANEL_CLAA101WA01A) += panel-claa101wa01a.o diff --git a/drivers/video/display/panel-claa101wa01a.c b/drivers/video/display/panel-claa101wa01a.c new file mode 100644 index 0000000..93ae86b --- /dev/null +++ b/drivers/video/display/panel-claa101wa01a.c @@ -0,0 +1,209 @@ +/* + * CLAA101WA01A Display Panel + * + * Copyright (C) 2013 NVIDIA CORPORATION. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/err.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/delay.h> +#include <linux/regulator/consumer.h> +#include <linux/of.h> +#include <linux/of_gpio.h> + +#include <video/display.h> + +#define CLAA101WA01A_WIDTH 223 +#define CLAA101WA01A_HEIGHT 125 + +struct panel_claa101 { + struct display_entity entity; + struct regulator *vdd_pnl; + struct regulator *vdd_bl; + /* Enable GPIOs */ + int pnl_enable; + int bl_enable; +}; + +#define to_panel_claa101(p) container_of(p, struct panel_claa101, entity) + +static int panel_claa101_set_state(struct display_entity *entity, + enum display_entity_state state) +{ + struct panel_claa101 *panel = to_panel_claa101(entity); + + /* OFF and STANDBY are equivalent to us */ + if (state == DISPLAY_ENTITY_STATE_STANDBY) + state = DISPLAY_ENTITY_STATE_OFF; + + switch (state) { + case DISPLAY_ENTITY_STATE_OFF: + case DISPLAY_ENTITY_STATE_STANDBY: + if (entity->source) + display_entity_set_stream(entity->source, + DISPLAY_ENTITY_STREAM_STOPPED); + + /* TODO error checking? */ + gpio_set_value_cansleep(panel->bl_enable, 0); + usleep_range(10000, 10000); + regulator_disable(panel->vdd_bl); + usleep_range(200000, 200000); + gpio_set_value_cansleep(panel->pnl_enable, 0); + regulator_disable(panel->vdd_pnl); + break; + + case DISPLAY_ENTITY_STATE_ON: + regulator_enable(panel->vdd_pnl); + gpio_set_value_cansleep(panel->pnl_enable, 1); + usleep_range(200000, 200000); + regulator_enable(panel->vdd_bl); + usleep_range(10000, 10000); + gpio_set_value_cansleep(panel->bl_enable, 1); + + if (entity->source) + display_entity_set_stream(entity->source, + DISPLAY_ENTITY_STREAM_CONTINUOUS); + break; + } + + return 0; +} + +static int panel_claa101_get_modes(struct display_entity *entity, + const struct videomode **modes) +{ + /* TODO get modes from EDID? */ + return 0; +} + +static int panel_claa101_get_size(struct display_entity *entity, + unsigned int *width, unsigned int *height) +{ + *width = CLAA101WA01A_WIDTH; + *height = CLAA101WA01A_HEIGHT; + + return 0; +} + +static int panel_claa101_get_params(struct display_entity *entity, + struct display_entity_interface_params *params) +{ + return 0; +} + +static const struct display_entity_control_ops panel_claa101_control_ops = { + .set_state = panel_claa101_set_state, + .get_modes = panel_claa101_get_modes, + .get_size = panel_claa101_get_size, + .get_params = panel_claa101_get_params, +}; + +static int __init panel_claa101_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct panel_claa101 *panel; + int err; + + panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL); + if (!panel) + return -ENOMEM; + + panel->vdd_pnl = devm_regulator_get(dev, "pnl"); + if (IS_ERR(panel->vdd_pnl)) { + dev_err(dev, "cannot get vdd regulator\n"); + return PTR_ERR(panel->vdd_pnl); + } + + panel->vdd_bl = devm_regulator_get(dev, "bl"); + if (IS_ERR(panel->vdd_bl)) { + dev_err(dev, "cannot get bl regulator\n"); + return PTR_ERR(panel->vdd_bl); + } + + err = of_get_named_gpio(dev->of_node, "pnl-enable-gpios", 0); + if (err < 0) { + dev_err(dev, "cannot find panel enable GPIO!\n"); + return err; + } + panel->pnl_enable = err; + err = devm_gpio_request_one(dev, panel->pnl_enable, + GPIOF_DIR_OUT | GPIOF_INIT_LOW, "panel"); + if (err < 0) { + dev_err(dev, "cannot acquire panel enable GPIO!\n"); + return err; + } + + err = of_get_named_gpio(dev->of_node, "bl-enable-gpios", 0); + if (err < 0) { + dev_err(dev, "cannot find backlight enable GPIO!\n"); + return err; + } + panel->bl_enable = err; + err = devm_gpio_request_one(dev, panel->bl_enable, + GPIOF_DIR_OUT | GPIOF_INIT_LOW, "backlight"); + if (err < 0) { + dev_err(dev, "cannot acquire backlight enable GPIO!\n"); + return err; + } + + panel->entity.dev = dev; + panel->entity.ops.ctrl = &panel_claa101_control_ops; + err = display_entity_register(&panel->entity); + if (err < 0) + return err; + + platform_set_drvdata(pdev, panel); + + dev_info(dev, "%s successful\n", __func__); + + return 0; +} + +static int __exit panel_claa101_remove(struct platform_device *pdev) +{ + struct panel_claa101 *panel = platform_get_drvdata(pdev); + + display_entity_unregister(&panel->entity); + + return 0; +} + +#ifdef CONFIG_OF +static struct of_device_id panel_claa101_of_match[] = { + { .compatible = "chunghwa,claa101wa01a", }, + { }, +}; +MODULE_DEVICE_TABLE(of, pwm_backlight_of_match); +#else +#endif + +static const struct dev_pm_ops panel_claa101_dev_pm_ops = { +}; + +static struct platform_driver panel_claa101_driver = { + .probe = panel_claa101_probe, + .remove = panel_claa101_remove, + .driver = { + .name = "panel_claa101wa01a", + .owner = THIS_MODULE, +#ifdef CONFIG_PM + .pm = &panel_claa101_dev_pm_ops, +#endif +#ifdef CONFIG_OF + .of_match_table = of_match_ptr(panel_claa101_of_match), +#endif + }, +}; + +module_platform_driver(panel_claa101_driver); + +MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>"); +MODULE_DESCRIPTION("Chunghwa CLAA101WA01A Display Panel"); +MODULE_LICENSE("GPL");
Add support for the Chunghwa CLAA101WA01A display panel. Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- .../video/display/chunghwa,claa101wa01a.txt | 8 + drivers/video/display/Kconfig | 8 + drivers/video/display/Makefile | 1 + drivers/video/display/panel-claa101wa01a.c | 209 +++++++++++++++++++++ 4 files changed, 226 insertions(+) create mode 100644 Documentation/devicetree/bindings/video/display/chunghwa,claa101wa01a.txt create mode 100644 drivers/video/display/panel-claa101wa01a.c