| Message ID | 1387205794-32246-24-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tomi, On Mon, Dec 16, 2013 at 04:56:30PM +0200, Tomi Valkeinen wrote: > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/video/omap2/displays-new/connector-dvi.c | 43 ++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/drivers/video/omap2/displays-new/connector-dvi.c b/drivers/video/omap2/displays-new/connector-dvi.c > index b6c50904038e..d1204b1c5182 100644 > > +static const struct of_device_id dvic_of_match[] = { > + { .compatible = "dvi-connector", }, Either the driver is too specific or the binding is too generic, but having such a generic name for an omap specific driver seems wrong. Same for panel-dpi, svideo-connector, composite-video-connector and hdmi-connector, Sascha
On 2013-12-17 09:05, Sascha Hauer wrote: > Hi Tomi, > > On Mon, Dec 16, 2013 at 04:56:30PM +0200, Tomi Valkeinen wrote: >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >> --- >> drivers/video/omap2/displays-new/connector-dvi.c | 43 ++++++++++++++++++++++++ >> 1 file changed, 43 insertions(+) >> >> diff --git a/drivers/video/omap2/displays-new/connector-dvi.c b/drivers/video/omap2/displays-new/connector-dvi.c >> index b6c50904038e..d1204b1c5182 100644 >> >> +static const struct of_device_id dvic_of_match[] = { >> + { .compatible = "dvi-connector", }, > > Either the driver is too specific or the binding is too generic, but > having such a generic name for an omap specific driver seems wrong. Same > for panel-dpi, svideo-connector, composite-video-connector and hdmi-connector, Hmm. Good point. I was thinking that the driver is only used on OMAP, but of course that's not true, the driver is there for all platforms if the kernel just happens to be compiled with the driver. And it's not just about those drivers you mention. The same issue is there for, say, "ti,tpd12s015". I have an omapdss specific driver for that, but if some other platform uses the same chip, they'll have a driver for it also... Sigh. I wonder how this should be handled... The only solution that comes to my mind is to have all the compatible strings as "ti,...". But that's not correct, as they are not TI components, but some are generic ones and some from another vendor. And even "ti,..." is not good, as there are other TI SoCs with other display drivers. So I'd need to prepend the compatible strings with "omapdss,...", making the hardware components driver specific. The long term plan is to make the drivers generic (or implement the same driver for common display framework). But for now I need to have future proof DT bindings with omapdss specific drivers... Tomi
Hi Tomi, On Tuesday 17 December 2013 09:15:58 Tomi Valkeinen wrote: > On 2013-12-17 09:05, Sascha Hauer wrote: > > On Mon, Dec 16, 2013 at 04:56:30PM +0200, Tomi Valkeinen wrote: > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > >> --- > >> > >> drivers/video/omap2/displays-new/connector-dvi.c | 43 ++++++++++++++++++ > >> 1 file changed, 43 insertions(+) > >> > >> diff --git a/drivers/video/omap2/displays-new/connector-dvi.c > >> b/drivers/video/omap2/displays-new/connector-dvi.c index > >> b6c50904038e..d1204b1c5182 100644 > >> > >> +static const struct of_device_id dvic_of_match[] = { > >> + { .compatible = "dvi-connector", }, > > > > Either the driver is too specific or the binding is too generic, but > > having such a generic name for an omap specific driver seems wrong. Same > > for panel-dpi, svideo-connector, composite-video-connector and > > hdmi-connector, > > Hmm. Good point. I was thinking that the driver is only used on OMAP, but of > course that's not true, the driver is there for all platforms if the kernel > just happens to be compiled with the driver. > > And it's not just about those drivers you mention. The same issue is there > for, say, "ti,tpd12s015". I have an omapdss specific driver for that, but if > some other platform uses the same chip, they'll have a driver for it also... > > Sigh. I wonder how this should be handled... The only solution that comes to > my mind is to have all the compatible strings as "ti,...". But that's not > correct, as they are not TI components, but some are generic ones and some > from another vendor. > > And even "ti,..." is not good, as there are other TI SoCs with other display > drivers. So I'd need to prepend the compatible strings with "omapdss,...", > making the hardware components driver specific. > > The long term plan is to make the drivers generic (or implement the same > driver for common display framework). But for now I need to have future > proof DT bindings with omapdss specific drivers... I'll refrain from mentioning that the problem has been identified more than a year ago. Oh, wait, I've metioned it, sorry :-D We really want to make drivers generic enough to be shared by multiple display controllers. I would vote for making the DT bindings generic now already, which would be enough to buy some time to fix the problem properly. If we start prepending driver-specific prefixes such as "omapdss," to compatible string we'll just make the mess even larger and reduce the incentive to fix it. It would be the worst decision we could make.
On 2013-12-17 17:15, Laurent Pinchart wrote: >>> Either the driver is too specific or the binding is too generic, but >>> having such a generic name for an omap specific driver seems wrong. Same >>> for panel-dpi, svideo-connector, composite-video-connector and >>> hdmi-connector, >> >> Hmm. Good point. I was thinking that the driver is only used on OMAP, but of >> course that's not true, the driver is there for all platforms if the kernel >> just happens to be compiled with the driver. >> >> And it's not just about those drivers you mention. The same issue is there >> for, say, "ti,tpd12s015". I have an omapdss specific driver for that, but if >> some other platform uses the same chip, they'll have a driver for it also... >> >> Sigh. I wonder how this should be handled... The only solution that comes to >> my mind is to have all the compatible strings as "ti,...". But that's not >> correct, as they are not TI components, but some are generic ones and some >> from another vendor. >> >> And even "ti,..." is not good, as there are other TI SoCs with other display >> drivers. So I'd need to prepend the compatible strings with "omapdss,...", >> making the hardware components driver specific. >> >> The long term plan is to make the drivers generic (or implement the same >> driver for common display framework). But for now I need to have future >> proof DT bindings with omapdss specific drivers... > > I'll refrain from mentioning that the problem has been identified more than a > year ago. Oh, wait, I've metioned it, sorry :-D > > We really want to make drivers generic enough to be shared by multiple display > controllers. I would vote for making the DT bindings generic now already, > which would be enough to buy some time to fix the problem properly. If we > start prepending driver-specific prefixes such as "omapdss," to compatible > string we'll just make the mess even larger and reduce the incentive to fix > it. It would be the worst decision we could make. Well... I think there are no good options here. I see the following options: 1. Add "omapdss," or similar to compat strings in DT, and the same for the drivers. This solves the issue, but then we'll have bad DT data, although I see similar method already being used in some places. When we have common drivers, we can remove the "omapdss," strings from DT, but we still need to keep them in the drivers to have backward compatibility. 2. Keep the compat strings generic in DT. This way we'll have correct DT data, but if anyone happens to create a new driver with the same compat string, things will break. omapdss can only be compiled for a kernel with OMAP and ARM support, so it narrows the problematic drivers a bit. 3. Have correct DT data, but at init time, in omap arch code, go through the DT data and change the compat strings for the display nodes to include "omapdss,". This way the drivers would only work for omap platforms. Like a combination of 1. and 2. I'm not sure if the DT-code allows this at the moment, though. We could also now select option 2., and go for 3. later if someone else creates a driver with the same compat string. While I'm obviously not very impartial here, I do think that using generic bindings for omapdss is not the worst possible case, as: I'm very much dedicated to get CDF merged at some point, and I already have been working on it for each revision. I also think that even if the omap panel drivers are currently omap specific, they are not very much so. I have been changing them over the years to be more and more generic, and I have used them as a base for CDF panel drivers. If some platform specific driver may have a generic compat string, omap panel drivers are not the worst option. I will look at the option 3., hopefully that will be possible and everybody will be happy. Any other ideas appreciated. Tomi
On 2013-12-18 12:41, Tomi Valkeinen wrote: > 3. Have correct DT data, but at init time, in omap arch code, go through > the DT data and change the compat strings for the display nodes to > include "omapdss,". This way the drivers would only work for omap > platforms. Like a combination of 1. and 2. I'm not sure if the DT-code > allows this at the moment, though. This wasn't actually too hard. It says "hack" all over it, but the code was quite compact. I call the omapdss_early_init_of() as the first thing in omap_generic_init(), before the devices are created: +static const char* const dss_compat_conv_list[] = { + "hdmi-connector", + "dvi-connector", + "ti,tpd12s015", + "panel-dsi-cm", +}; + +static void omapdss_omapify_node(struct device_node *node, const char *compat) +{ + char *new_compat; + struct property *prop; + + new_compat = kasprintf(GFP_KERNEL, "omapdss,%s", compat); + + prop = kzalloc(sizeof(*prop), GFP_KERNEL); + prop->name = "compatible"; + prop->value = new_compat; + prop->length = strlen(new_compat) + 1; + + of_update_property(node, prop); +} + +void __init omapdss_early_init_of(void) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(dss_compat_conv_list); ++i) { + const char *compat = dss_compat_conv_list[i]; + struct device_node *node = NULL; + + while ((node = of_find_compatible_node(node, NULL, compat))) { + if (!of_device_is_available(node)) + continue; + + omapdss_omapify_node(node, compat); + } + } +} The list has just part of the devices, and I've so far only tested on OMAP 4430sdp board, but it seemed to work fine. So with this, I can have "hdmi-connector" in the .dts file, and "omapdss,hdmi-connector" as a compat string in the omap specific driver. Does it make your eyes bleed, or is it maybe something that could be fine for the time being? Tomi
diff --git a/drivers/video/omap2/displays-new/connector-dvi.c b/drivers/video/omap2/displays-new/connector-dvi.c index b6c50904038e..d1204b1c5182 100644 --- a/drivers/video/omap2/displays-new/connector-dvi.c +++ b/drivers/video/omap2/displays-new/connector-dvi.c @@ -277,6 +277,37 @@ static int dvic_probe_pdata(struct platform_device *pdev) return 0; } +static int dvic_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; + struct device_node *adapter_node; + struct i2c_adapter *adapter; + + 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; + + adapter_node = of_parse_phandle(node, "i2c-bus", 0); + if (adapter_node) { + adapter = of_find_i2c_adapter_by_node(adapter_node); + if (adapter == NULL) { + dev_err(&pdev->dev, "failed to parse i2c-bus\n"); + omap_dss_put_device(ddata->in); + return -EPROBE_DEFER; + } + + ddata->i2c_adapter = adapter; + } + + return 0; +} + static int dvic_probe(struct platform_device *pdev) { struct panel_drv_data *ddata; @@ -293,6 +324,10 @@ static int dvic_probe(struct platform_device *pdev) r = dvic_probe_pdata(pdev); if (r) return r; + } else if (pdev->dev.of_node) { + r = dvic_probe_of(pdev); + if (r) + return r; } else { return -ENODEV; } @@ -342,12 +377,20 @@ static int __exit dvic_remove(struct platform_device *pdev) return 0; } +static const struct of_device_id dvic_of_match[] = { + { .compatible = "dvi-connector", }, + {}, +}; + +MODULE_DEVICE_TABLE(of, dvic_of_match); + static struct platform_driver dvi_connector_driver = { .probe = dvic_probe, .remove = __exit_p(dvic_remove), .driver = { .name = "connector-dvi", .owner = THIS_MODULE, + .of_match_table = dvic_of_match, }, };
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/video/omap2/displays-new/connector-dvi.c | 43 ++++++++++++++++++++++++ 1 file changed, 43 insertions(+)