Patchwork [3/4] OMAPDSS: panel-sharp-ls037v7dw01: add device tree support

login
register
mail settings
Submitter Tony Lindgren
Date May 8, 2014, 11:33 p.m.
Message ID <20140508233300.GI2198@atomide.com>
Download mbox | patch
Permalink /patch/347278/
State New
Headers show

Comments

Tony Lindgren - May 8, 2014, 11:33 p.m.
* Tony Lindgren <tony@atomide.com> [140507 11:00]:
> * Tomi Valkeinen <tomi.valkeinen@ti.com> [140507 09:03]:
> > On 07/05/14 18:03, Tony Lindgren wrote:
> > > 
> > > BTW, I'm also personally fine with all five gpios showing in a single
> > > gpios property, I'm not too exited about naming anything in DT..
> > 
> > I don't have a strong opinion here. I don't have much experience with
> > DT, especially with making bindings compatible with other ones.
> > 
> > I'd just forget the simple-panel, and have single gpio array.
> 
> Well if it's a don't care flag for both of us, let's try to use
> the existing standard for simple-panel.txt and add mode-gpios
> property. I'll post a patch for that.

Here's an updated version using enable-gpios, reset-gpios and
mode-gpios. So it follows simple-panel.txt and adds mode-gpios
that's currently specific to this panel only.

Also updated for -EPROBE_DEFER handling, tested that by changing
one of the GPIOs to be a twl4030 GPIO.

Regards,

Tony

8<------------------
From: Tony Lindgren <tony@atomide.com>
Date: Mon, 28 Apr 2014 20:22:21 -0700
Subject: [PATCH] OMAPDSS: panel-sharp-ls037v7dw01: add device tree support

We can pass the GPIO configuration for ls037v7dw01 in a standard
gpios property.

Signed-off-by: Tony Lindgren <tony@atomide.com>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen - May 9, 2014, 7:24 a.m.
On 09/05/14 02:33, Tony Lindgren wrote:

> We can pass the GPIO configuration for ls037v7dw01 in a standard
> gpios property.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt
> @@ -0,0 +1,56 @@
> +SHARP LS037V7DW01 TFT-LCD panel
> +
> +Required properties:
> +- compatible: should be "sharp,ls037v7dw01"
> +
> +Optional properties:
> +- enable-gpios: a GPIO spec for the optional enable pin
> +  this pin is the INI pin as specified in the LS037V7DW01.pdf file.
> +- reset-gpios: a GPIO spec for the optional reset pin
> +  this pin is the RESB pin as specified in the LS037V7DW01.pdf file.
> +- mode-gpios: a GPIO 
> +  ordered MO, LR, and UD as specified in the LS037V7DW01.pdf file.
> +
> +This binding is compatible with the simple-panel binding, which is specified
> +in simple-panel.txt in this directory.

The video port data is required also. See for example
Documentation/devicetree/bindings/video/panel-dsi-cm.txt or
Documentation/devicetree/bindings/video/hdmi-connector.txt.

Also, all the bindings I've made are in
Documentation/devicetree/bindings/video/, and I'd rather keep the video
bindings OMAP uses in the same place.

> +This panel can have zero to five GPIOs to configure
> +to change configuration between QVGA and VGA mode
> +and the scan direction. As these pins can be also
> +configured with external pulls, all the GPIOs are
> +considered optional with holes in the array.
> +
> +Example when connected to a omap2+ based device:
> +
> +	lcd0: display {
> +		compatible = "sharp,ls037v7dw01";
> +		power-supply = <&lcd_3v3>;
> +		enable-gpios = <&gpio5 24 GPIO_ACTIVE_HIGH>;	/* gpio152, lcd INI */
> +		reset-gpios = <&gpio5 27 GPIO_ACTIVE_HIGH>;	/* gpio155, lcd RESB */
> +		mode-gpios = <&gpio5 26 GPIO_ACTIVE_HIGH	/* gpio154, lcd MO */
> +			      &gpio1 2 GPIO_ACTIVE_HIGH		/* gpio2, lcd LR */
> +			      &gpio1 3 GPIO_ACTIVE_HIGH>;	/* gpio3, lcd UD */
> +
> +		panel-timing {
> +			clock-frequency = <19200000>;
> +			hback-porch = <28>;
> +			hactive = <480>;
> +			hfront-porch = <1>;
> +			hsync-len = <2>;
> +			vback-porch = <1>;
> +			vactive = <640>;
> +			vfront-porch = <1>;
> +			vsync-len = <1>;
> +			hsync-active = <0>;
> +			vsync-active = <0>;
> +			de-active = <1>;
> +			pixelclk-active = <1>;
> +		};

I don't think we should define panel-timing here. We know it's
sharp,ls037v7dw01, so the driver knows the video timings. Also, if we
would extend the driver to support both resolution modes, it needs to
support two different timings, so the above doesn't work in that case
either.

Although if the MO gpio is not controlled by the driver, we should tell
the driver whether that gpio is high or low.

> +static int sharp_ls_probe_of(struct platform_device *pdev)
> +{
> +	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
> +	struct device_node *node = pdev->dev.of_node;
> +	struct omap_dss_device *in;
> +	struct display_timing timing;
> +	struct videomode vm;
> +	int r;
> +
> +	ddata->vcc = devm_regulator_get(&pdev->dev, "envdd");
> +	if (IS_ERR(ddata->vcc)) {
> +		dev_err(&pdev->dev, "failed to get regulator\n");
> +		return PTR_ERR(ddata->vcc);
> +	}
> +
> +	r = regulator_enable(ddata->vcc);
> +	if (r != 0) {
> +		dev_err(&pdev->dev, "failed to enable regulator\n");
> +		return r;
> +	}

The regulator should be enabled when the panel is enabled, not at probe
time.

> +static const struct of_device_id sharp_ls_of_match[] = {
> +	{ .compatible = "sharp,ls037v7dw01", },

At the moment our display drivers are OMAP specific, and for that reason
we should prefix the compatible strings with "omapdss,". For example,
drivers/video/fbdev/omap2/displays-new/panel-dsi-cm.c:

	{ .compatible = "omapdss,panel-dsi-cm", },

But we should still have the right compatible string in the .dts, so we
convert the compatible name in arch/arm/mach-omap2/display.c, with
'dss_compat_conv_list' array, to which this panel's name should be added.

 Tomi
Tony Lindgren - May 9, 2014, 3:55 p.m.
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140509 00:24]:
> On 09/05/14 02:33, Tony Lindgren wrote:
> > +Example when connected to a omap2+ based device:
> > +
> > +	lcd0: display {
> > +		compatible = "sharp,ls037v7dw01";
> > +		power-supply = <&lcd_3v3>;
> > +		enable-gpios = <&gpio5 24 GPIO_ACTIVE_HIGH>;	/* gpio152, lcd INI */
> > +		reset-gpios = <&gpio5 27 GPIO_ACTIVE_HIGH>;	/* gpio155, lcd RESB */
> > +		mode-gpios = <&gpio5 26 GPIO_ACTIVE_HIGH	/* gpio154, lcd MO */
> > +			      &gpio1 2 GPIO_ACTIVE_HIGH		/* gpio2, lcd LR */
> > +			      &gpio1 3 GPIO_ACTIVE_HIGH>;	/* gpio3, lcd UD */
> > +
> > +		panel-timing {
> > +			clock-frequency = <19200000>;
> > +			hback-porch = <28>;
> > +			hactive = <480>;
> > +			hfront-porch = <1>;
> > +			hsync-len = <2>;
> > +			vback-porch = <1>;
> > +			vactive = <640>;
> > +			vfront-porch = <1>;
> > +			vsync-len = <1>;
> > +			hsync-active = <0>;
> > +			vsync-active = <0>;
> > +			de-active = <1>;
> > +			pixelclk-active = <1>;
> > +		};
> 
> I don't think we should define panel-timing here. We know it's
> sharp,ls037v7dw01, so the driver knows the video timings. Also, if we
> would extend the driver to support both resolution modes, it needs to
> support two different timings, so the above doesn't work in that case
> either.

OK. It seems we can have at least two different timings for this panel,
the VGA timing above and the QVGA timings that LDP uses that are listed
in the .dts changes.
 
> Although if the MO gpio is not controlled by the driver, we should tell
> the driver whether that gpio is high or low.

What do you have in mind for telling that? We should also tell the
orientation of the panel:

EVM	VGA	omapfb.rotate=3
LDP	QVGA	omapfb.rotate=0

Do you have something in mind for that?
 
> At the moment our display drivers are OMAP specific, and for that reason
> we should prefix the compatible strings with "omapdss,". For example,
> drivers/video/fbdev/omap2/displays-new/panel-dsi-cm.c:
> 
> 	{ .compatible = "omapdss,panel-dsi-cm", },
> 
> But we should still have the right compatible string in the .dts, so we
> convert the compatible name in arch/arm/mach-omap2/display.c, with
> 'dss_compat_conv_list' array, to which this panel's name should be added.

Oh so what do you want to have in the .dts file then?

Regards,

Tony 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen - May 12, 2014, 7:38 a.m.
On 09/05/14 18:55, Tony Lindgren wrote:

>> Although if the MO gpio is not controlled by the driver, we should tell
>> the driver whether that gpio is high or low.
> 
> What do you have in mind for telling that? We should also tell the
> orientation of the panel:
> 
> EVM	VGA	omapfb.rotate=3
> LDP	QVGA	omapfb.rotate=0
> 
> Do you have something in mind for that?

Hmm, right. I guess all we can do is have boolean flags in the .dts for
MO, LR and UD, which tells if the respective pins are hard-wires high or
low. And say in the documentation that you must either have a proper
GPIO, or use the flag, but not both.

The panel mounting orientation is a different matter. But I think it is
also something we should specify in the .dts. However, we don't have any
SW support to handle it, and it's a bit unclear to me how it should be
handled, so I think that has to be left for later.

>> At the moment our display drivers are OMAP specific, and for that reason
>> we should prefix the compatible strings with "omapdss,". For example,
>> drivers/video/fbdev/omap2/displays-new/panel-dsi-cm.c:
>>
>> 	{ .compatible = "omapdss,panel-dsi-cm", },
>>
>> But we should still have the right compatible string in the .dts, so we
>> convert the compatible name in arch/arm/mach-omap2/display.c, with
>> 'dss_compat_conv_list' array, to which this panel's name should be added.
> 
> Oh so what do you want to have in the .dts file then?

The .dts should have the proper names. The idea of this hackery is that
in the .dts we can have the proper compatible string. So in the .dts, we
have, say:

"sony,panel-foobar"

Then, at boot time, omap's platform code changes that to:

"omapdss,sony,panel-foobar".

And our (omap specific) panel-foobar driver use that
"omapdss,sony,panel-foobar" string.

This way some other platform could do the same, and have their platform
specific drivers handle the panel.

At some point in the future we hopefully will have common panel drivers,
and at that point we can remove that hackery. The .dts files will
already be correct.

 Tomi
Javier Martinez Canillas - May 12, 2014, 9:34 a.m.
Hello Tomi,

On Mon, May 12, 2014 at 9:38 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 09/05/14 18:55, Tony Lindgren wrote:
>
>>> Although if the MO gpio is not controlled by the driver, we should tell
>>> the driver whether that gpio is high or low.
>>
>> What do you have in mind for telling that? We should also tell the
>> orientation of the panel:
>>
>> EVM   VGA     omapfb.rotate=3
>> LDP   QVGA    omapfb.rotate=0
>>
>> Do you have something in mind for that?
>
> Hmm, right. I guess all we can do is have boolean flags in the .dts for
> MO, LR and UD, which tells if the respective pins are hard-wires high or
> low. And say in the documentation that you must either have a proper
> GPIO, or use the flag, but not both.
>
> The panel mounting orientation is a different matter. But I think it is
> also something we should specify in the .dts. However, we don't have any
> SW support to handle it, and it's a bit unclear to me how it should be
> handled, so I think that has to be left for later.
>
>>> At the moment our display drivers are OMAP specific, and for that reason
>>> we should prefix the compatible strings with "omapdss,". For example,
>>> drivers/video/fbdev/omap2/displays-new/panel-dsi-cm.c:
>>>
>>>      { .compatible = "omapdss,panel-dsi-cm", },
>>>
>>> But we should still have the right compatible string in the .dts, so we
>>> convert the compatible name in arch/arm/mach-omap2/display.c, with
>>> 'dss_compat_conv_list' array, to which this panel's name should be added.
>>
>> Oh so what do you want to have in the .dts file then?
>
> The .dts should have the proper names. The idea of this hackery is that
> in the .dts we can have the proper compatible string. So in the .dts, we
> have, say:
>
> "sony,panel-foobar"
>
> Then, at boot time, omap's platform code changes that to:
>
> "omapdss,sony,panel-foobar".
>
> And our (omap specific) panel-foobar driver use that
> "omapdss,sony,panel-foobar" string.
>
> This way some other platform could do the same, and have their platform
> specific drivers handle the panel.
>
> At some point in the future we hopefully will have common panel drivers,
> and at that point we can remove that hackery. The .dts files will
> already be correct.
>

Maybe we can remove this hackery by relying on the fact that a
compatible string can have a set of values that goes from more
specific to more general. So you can have something like:

compatible = "sony,panel-foobar", "omapdss,panel-foobar"

So right now only "omapdss,panel-foobar" will be matched and later
when we have common panel drivers then that driver could handle the
"sony,panel-foobar" compatible string.

Other platforms could do something similar and have

compatible = "sony,panel-foobar", "baz,panel-foobar"

That way you won't break DT backward compatible but still not require
hacks on arch/arm/mach-omap2/display.c.

We do the same for OMAP boards, we now have the following compatible string:

compatible = "isee,omap3-igep0020", "ti,omap36xx", "ti,omap3";

There isn't a struct machine_desc that matches "isee,omap3-igep0020"
but later if we find that we need some board specific initialization
we could add one without breaking existing DTS. In fact it used to be
a single machine_desc that matched "ti,omap3" for both omap36xx and
omap34xx but later when some DT clocks changes were introduced we
needed to split both SoC families.

>  Tomi
>
>

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen - May 12, 2014, 9:40 a.m.
On 12/05/14 12:34, Javier Martinez Canillas wrote:

> Maybe we can remove this hackery by relying on the fact that a
> compatible string can have a set of values that goes from more
> specific to more general. So you can have something like:
> 
> compatible = "sony,panel-foobar", "omapdss,panel-foobar"
> 
> So right now only "omapdss,panel-foobar" will be matched and later
> when we have common panel drivers then that driver could handle the
> "sony,panel-foobar" compatible string.
> 
> Other platforms could do something similar and have
> 
> compatible = "sony,panel-foobar", "baz,panel-foobar"
> 
> That way you won't break DT backward compatible but still not require
> hacks on arch/arm/mach-omap2/display.c.
> 
> We do the same for OMAP boards, we now have the following compatible string:
> 
> compatible = "isee,omap3-igep0020", "ti,omap36xx", "ti,omap3";
> 
> There isn't a struct machine_desc that matches "isee,omap3-igep0020"
> but later if we find that we need some board specific initialization
> we could add one without breaking existing DTS. In fact it used to be
> a single machine_desc that matched "ti,omap3" for both omap36xx and
> omap34xx but later when some DT clocks changes were introduced we
> needed to split both SoC families.

I think that's a different thing. "isee,omap3-igep0020" is a proper
compatible string, if the board is "isee,...".  No matter what software
you run, that's correct and fine.

The omapdss case is different, there the "omapdss" points to a sw thing,
it does not describe the hardware. It's only needed as we don't have
proper sw drivers for the devices.

That said, I think what you describe would work. But I would rather keep
the .dts files clean and correct, and keep that hacks hidden inside the
kernel.

 Tomi
Javier Martinez Canillas - May 12, 2014, 10 a.m.
Hello Tomi,

On Mon, May 12, 2014 at 11:40 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 12/05/14 12:34, Javier Martinez Canillas wrote:
>
>> Maybe we can remove this hackery by relying on the fact that a
>> compatible string can have a set of values that goes from more
>> specific to more general. So you can have something like:
>>
>> compatible = "sony,panel-foobar", "omapdss,panel-foobar"
>>
>> So right now only "omapdss,panel-foobar" will be matched and later
>> when we have common panel drivers then that driver could handle the
>> "sony,panel-foobar" compatible string.
>>
>> Other platforms could do something similar and have
>>
>> compatible = "sony,panel-foobar", "baz,panel-foobar"
>>
>> That way you won't break DT backward compatible but still not require
>> hacks on arch/arm/mach-omap2/display.c.
>>
>> We do the same for OMAP boards, we now have the following compatible string:
>>
>> compatible = "isee,omap3-igep0020", "ti,omap36xx", "ti,omap3";
>>
>> There isn't a struct machine_desc that matches "isee,omap3-igep0020"
>> but later if we find that we need some board specific initialization
>> we could add one without breaking existing DTS. In fact it used to be
>> a single machine_desc that matched "ti,omap3" for both omap36xx and
>> omap34xx but later when some DT clocks changes were introduced we
>> needed to split both SoC families.
>
> I think that's a different thing. "isee,omap3-igep0020" is a proper
> compatible string, if the board is "isee,...".  No matter what software
> you run, that's correct and fine.
>
> The omapdss case is different, there the "omapdss" points to a sw thing,
> it does not describe the hardware. It's only needed as we don't have
> proper sw drivers for the devices.
>
> That said, I think what you describe would work. But I would rather keep
> the .dts files clean and correct, and keep that hacks hidden inside the
> kernel.
>

Thanks for the explanation. Since DT are meant to describe hardware
and not software I agree with you that we shouldn't leak an
implementation detail to the DeviceTrees.

And after all fortunately we don't have a stable API in the kernel
like the one that is enforced in the DT so we can fix it later ;-)

>  Tomi
>
>

Bets regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren - May 12, 2014, 2:26 p.m.
* Javier Martinez Canillas <javier@dowhile0.org> [140512 03:01]:
> Hello Tomi,
> 
> On Mon, May 12, 2014 at 11:40 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On 12/05/14 12:34, Javier Martinez Canillas wrote:
> >
> >> Maybe we can remove this hackery by relying on the fact that a
> >> compatible string can have a set of values that goes from more
> >> specific to more general. So you can have something like:
> >>
> >> compatible = "sony,panel-foobar", "omapdss,panel-foobar"
> >>
> >> So right now only "omapdss,panel-foobar" will be matched and later
> >> when we have common panel drivers then that driver could handle the
> >> "sony,panel-foobar" compatible string.
> >>
> >> Other platforms could do something similar and have
> >>
> >> compatible = "sony,panel-foobar", "baz,panel-foobar"
> >>
> >> That way you won't break DT backward compatible but still not require
> >> hacks on arch/arm/mach-omap2/display.c.
> >>
> >> We do the same for OMAP boards, we now have the following compatible string:
> >>
> >> compatible = "isee,omap3-igep0020", "ti,omap36xx", "ti,omap3";
> >>
> >> There isn't a struct machine_desc that matches "isee,omap3-igep0020"
> >> but later if we find that we need some board specific initialization
> >> we could add one without breaking existing DTS. In fact it used to be
> >> a single machine_desc that matched "ti,omap3" for both omap36xx and
> >> omap34xx but later when some DT clocks changes were introduced we
> >> needed to split both SoC families.
> >
> > I think that's a different thing. "isee,omap3-igep0020" is a proper
> > compatible string, if the board is "isee,...".  No matter what software
> > you run, that's correct and fine.
> >
> > The omapdss case is different, there the "omapdss" points to a sw thing,
> > it does not describe the hardware. It's only needed as we don't have
> > proper sw drivers for the devices.
> >
> > That said, I think what you describe would work. But I would rather keep
> > the .dts files clean and correct, and keep that hacks hidden inside the
> > kernel.
> >
> 
> Thanks for the explanation. Since DT are meant to describe hardware
> and not software I agree with you that we shouldn't leak an
> implementation detail to the DeviceTrees.
> 
> And after all fortunately we don't have a stable API in the kernel
> like the one that is enforced in the DT so we can fix it later ;-)

This remapping of compatible flags sounds smelly to me, please discuss
it first with the device tree people.

I think we're better off just using the compatible properties limited
to the simple-panel.txt for now and actually describe the real
hardware. The fact that the panel is connected to DSS should be possible
to find out based on the phandles.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen - May 12, 2014, 2:55 p.m.
On 12/05/14 17:26, Tony Lindgren wrote:

>>> The omapdss case is different, there the "omapdss" points to a sw thing,
>>> it does not describe the hardware. It's only needed as we don't have
>>> proper sw drivers for the devices.
>>>
>>> That said, I think what you describe would work. But I would rather keep
>>> the .dts files clean and correct, and keep that hacks hidden inside the
>>> kernel.
>>>
>>
>> Thanks for the explanation. Since DT are meant to describe hardware
>> and not software I agree with you that we shouldn't leak an
>> implementation detail to the DeviceTrees.
>>
>> And after all fortunately we don't have a stable API in the kernel
>> like the one that is enforced in the DT so we can fix it later ;-)
> 
> This remapping of compatible flags sounds smelly to me, please discuss
> it first with the device tree people.

It's already in v3.15.

I agree it's smelly, but it smelly only inside the kernel, not visible
anywhere, and we can remove it when we have common panel drivers,
without any modifications to .dts files.

> I think we're better off just using the compatible properties limited
> to the simple-panel.txt for now and actually describe the real
> hardware. The fact that the panel is connected to DSS should be possible
> to find out based on the phandles.

I'm not sure what you mean. We do describe only the real hardware. The
compatible string in the .dts file is "sharp,ls037v7dw01".

Prepending the "omapdss" to the compatible string is required for the
device/driver matching to work, because we can't use the
"sharp,ls037v7dw01" compatible string in our omap specific panel driver.

I'm not sure what it would give us to try to be compatible with
simple-panel.txt. The simple-panel bindings won't probably be compatible
with the future common display drivers in any case, as the simple-panel
binding is just too limited and doesn't describe the hardware fully.

 Tomi
Tony Lindgren - May 12, 2014, 3:51 p.m.
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140512 07:55]:
> On 12/05/14 17:26, Tony Lindgren wrote:
> 
> >>> The omapdss case is different, there the "omapdss" points to a sw thing,
> >>> it does not describe the hardware. It's only needed as we don't have
> >>> proper sw drivers for the devices.
> >>>
> >>> That said, I think what you describe would work. But I would rather keep
> >>> the .dts files clean and correct, and keep that hacks hidden inside the
> >>> kernel.
> >>>
> >>
> >> Thanks for the explanation. Since DT are meant to describe hardware
> >> and not software I agree with you that we shouldn't leak an
> >> implementation detail to the DeviceTrees.
> >>
> >> And after all fortunately we don't have a stable API in the kernel
> >> like the one that is enforced in the DT so we can fix it later ;-)
> > 
> > This remapping of compatible flags sounds smelly to me, please discuss
> > it first with the device tree people.
> 
> It's already in v3.15.

Oh great.. And you dumped it into arch/arm/mach-omap2 too and I somehow
even acked it :( Looks like there's also the custom mux hacks. And
custom hwmod hacks. And ongoing soc_is_xxx tinkering. Now let's not add
_any_ new crap into arch/arm/mach-omap2/display.c.

So please consider this a huge eternal NAK to add any more crap to
arch/arm/mach-omap2/display.c from me. No more new soc_is beyond
the soc_is_am43xx() you have in linux next. No more adding 
of_find_compatible_node() beyond ti,omap5-dss you have in linux next.

And no more new panel remapping in this file as soon as you have found
a better solution. Preferrably by v3.17 merge window. This file just
needs to disappear. Yuk.

> I agree it's smelly, but it smelly only inside the kernel, not visible
> anywhere, and we can remove it when we have common panel drivers,
> without any modifications to .dts files.
> 
> > I think we're better off just using the compatible properties limited
> > to the simple-panel.txt for now and actually describe the real
> > hardware. The fact that the panel is connected to DSS should be possible
> > to find out based on the phandles.
> 
> I'm not sure what you mean. We do describe only the real hardware. The
> compatible string in the .dts file is "sharp,ls037v7dw01".
> 
> Prepending the "omapdss" to the compatible string is required for the
> device/driver matching to work, because we can't use the
> "sharp,ls037v7dw01" compatible string in our omap specific panel driver.
> 
> I'm not sure what it would give us to try to be compatible with
> simple-panel.txt. The simple-panel bindings won't probably be compatible
> with the future common display drivers in any case, as the simple-panel
> binding is just too limited and doesn't describe the hardware fully.

Hmm what else does a panel need where the existing binding cannot be
extended? 

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen - May 13, 2014, 10:51 a.m.
On 12/05/14 18:51, Tony Lindgren wrote:

>> It's already in v3.15.
> 
> Oh great.. And you dumped it into arch/arm/mach-omap2 too and I somehow
> even acked it :( Looks like there's also the custom mux hacks. And
> custom hwmod hacks. And ongoing soc_is_xxx tinkering. Now let's not add

The omap2_dss_hwmod_data, create_dss_pdev and create_simple_dss_pdev,
omap_display_init stuff can be removed when the DT conversion has been done.

For the omap4_dsi_mux_pads (or the omap5 dsi muxing that was recently
discussed) I still have no solution, but I haven't spent time on it. I
have dropped the omap5 dsi muxing from the latest series for that reason.

dispc_disable_outputs and omap_dss_reset are needed because omap_device
doesn't handle resetting DSS properly, as special steps need to be done
for that. omap_dss_reset is called from the hwmod/omap_device code, so I
don't think this code can be anywhere else.

For the omap_display_get_version() I have no good solution. Making
separate .dts versions for all the needed OMAP ES versions seems like a
huge hassle to me, but that is one solution.

I think that would mean having separate .dtsi files for omapdss for
omap3_es1, omap3_es3plus, omap4_es1, omap4_es2, omap4_es3plus, and then
having separate omapxxxx.dtsi files for all of those, and then separate
board .dts files for the ES versions used on each board.

Or is there some sane way to define such things in dts?

> _any_ new crap into arch/arm/mach-omap2/display.c.
> 
> So please consider this a huge eternal NAK to add any more crap to
> arch/arm/mach-omap2/display.c from me. No more new soc_is beyond
> the soc_is_am43xx() you have in linux next. No more adding 
> of_find_compatible_node() beyond ti,omap5-dss you have in linux next.
> 
> And no more new panel remapping in this file as soon as you have found
> a better solution. Preferrably by v3.17 merge window. This file just
> needs to disappear. Yuk.

Do you object to the compatible string remapping as such, or just that
it's in arch/arm/mach-omap2?

I guess nothing prevents me from moving it to drivers/, and having some
early-ish initcall doing the job.

If the remapping as such is horrible, I'm all ears for better ideas. I
spent quite a lot of time on it, and that's the best I could come up with.

It's rather simple prefixing of the compatible strings for selected
devices. The purpose of that is to have proper data in the .dts files
(which I think is very important) with the cost of having some rather
simple internal hacks in the kernel, which can be removed immediately
when we have a common display driver framework.

>> I'm not sure what it would give us to try to be compatible with
>> simple-panel.txt. The simple-panel bindings won't probably be compatible
>> with the future common display drivers in any case, as the simple-panel
>> binding is just too limited and doesn't describe the hardware fully.
> 
> Hmm what else does a panel need where the existing binding cannot be
> extended? 

The existing simple-panel binding doesn't describe the connections of
the panel, i.e. its video input. I guess it can be extended, but I don't
see what the benefit is of trying to create new panel bindings
compatible with the simple-panel bindings. As I see, the simple-panel
bindings work only with very limited use cases, where the drivers make
assumptions. Simple panel bindings cannot be used with omapdss, nor can
it be used with the future common display framework.

But I'm not really familiar with using extending current bindings, and
making new bindings compatible with old ones. Can you explain why it'd
be good to have the sharp panel bindings compatible with simple-panel?
In what kind of scenario would it be used?

 Tomi
Javier Martinez Canillas - May 13, 2014, 11:39 a.m.
On Tue, May 13, 2014 at 12:51 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 12/05/14 18:51, Tony Lindgren wrote:
>
>>> It's already in v3.15.
>>
>> Oh great.. And you dumped it into arch/arm/mach-omap2 too and I somehow
>> even acked it :( Looks like there's also the custom mux hacks. And
>> custom hwmod hacks. And ongoing soc_is_xxx tinkering. Now let's not add
>
> The omap2_dss_hwmod_data, create_dss_pdev and create_simple_dss_pdev,
> omap_display_init stuff can be removed when the DT conversion has been done.
>
> For the omap4_dsi_mux_pads (or the omap5 dsi muxing that was recently
> discussed) I still have no solution, but I haven't spent time on it. I
> have dropped the omap5 dsi muxing from the latest series for that reason.
>
> dispc_disable_outputs and omap_dss_reset are needed because omap_device
> doesn't handle resetting DSS properly, as special steps need to be done
> for that. omap_dss_reset is called from the hwmod/omap_device code, so I
> don't think this code can be anywhere else.
>
> For the omap_display_get_version() I have no good solution. Making
> separate .dts versions for all the needed OMAP ES versions seems like a
> huge hassle to me, but that is one solution.
>
> I think that would mean having separate .dtsi files for omapdss for
> omap3_es1, omap3_es3plus, omap4_es1, omap4_es2, omap4_es3plus, and then
> having separate omapxxxx.dtsi files for all of those, and then separate
> board .dts files for the ES versions used on each board.
>
> Or is there some sane way to define such things in dts?
>

Unfortunately there isn't.

I have a similar problem with the IGEP boards since there are just too
many variations of them (e.g: DM3730 SoC vs OMAP3530 SoC, NAND vs
OneNAND, 256MB vs 512MB flash memory sizes, with and without wifi
chip, etc).

Since dts are supposed to describe the hardware, each revision with a
slighter variation forces to have a different dts. My solution was to
not try to have a dts for every single variation in mainline but only
one for the most common revision and that could be used as a reference
to modify and ship on each device. Unfortunately that is not possible
to you since each DSS IP block is used by many boards.

>> _any_ new crap into arch/arm/mach-omap2/display.c.
>>
>> So please consider this a huge eternal NAK to add any more crap to
>> arch/arm/mach-omap2/display.c from me. No more new soc_is beyond
>> the soc_is_am43xx() you have in linux next. No more adding
>> of_find_compatible_node() beyond ti,omap5-dss you have in linux next.
>>
>> And no more new panel remapping in this file as soon as you have found
>> a better solution. Preferrably by v3.17 merge window. This file just
>> needs to disappear. Yuk.
>
> Do you object to the compatible string remapping as such, or just that
> it's in arch/arm/mach-omap2?
>
> I guess nothing prevents me from moving it to drivers/, and having some
> early-ish initcall doing the job.
>
> If the remapping as such is horrible, I'm all ears for better ideas. I
> spent quite a lot of time on it, and that's the best I could come up with.
>
> It's rather simple prefixing of the compatible strings for selected
> devices. The purpose of that is to have proper data in the .dts files
> (which I think is very important) with the cost of having some rather
> simple internal hacks in the kernel, which can be removed immediately
> when we have a common display driver framework.
>

Even though the compatible trick that I said before could be an
alternative, it has the problem that does not describes the hardware
as you said. The restriction of the DT being a stable API and get it
right from the beginning forces us to make these kind of technical
decisions.

So, since we can change the kernel later but not the DTS, I agree with
you that the remapping is the least bad of our options.

>>> I'm not sure what it would give us to try to be compatible with
>>> simple-panel.txt. The simple-panel bindings won't probably be compatible
>>> with the future common display drivers in any case, as the simple-panel
>>> binding is just too limited and doesn't describe the hardware fully.
>>
>> Hmm what else does a panel need where the existing binding cannot be
>> extended?
>
> The existing simple-panel binding doesn't describe the connections of
> the panel, i.e. its video input. I guess it can be extended, but I don't
> see what the benefit is of trying to create new panel bindings
> compatible with the simple-panel bindings. As I see, the simple-panel
> bindings work only with very limited use cases, where the drivers make
> assumptions. Simple panel bindings cannot be used with omapdss, nor can
> it be used with the future common display framework.
>
> But I'm not really familiar with using extending current bindings, and
> making new bindings compatible with old ones. Can you explain why it'd
> be good to have the sharp panel bindings compatible with simple-panel?
> In what kind of scenario would it be used?
>
>  Tomi
>
>

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren - May 13, 2014, 3:25 p.m.
* Javier Martinez Canillas <javier@dowhile0.org> [140513 04:40]:
> On Tue, May 13, 2014 at 12:51 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On 12/05/14 18:51, Tony Lindgren wrote:
> >
> >>> It's already in v3.15.
> >>
> >> Oh great.. And you dumped it into arch/arm/mach-omap2 too and I somehow
> >> even acked it :( Looks like there's also the custom mux hacks. And
> >> custom hwmod hacks. And ongoing soc_is_xxx tinkering. Now let's not add
> >
> > The omap2_dss_hwmod_data, create_dss_pdev and create_simple_dss_pdev,
> > omap_display_init stuff can be removed when the DT conversion has been done.

Yes great that helps :)

> > For the omap4_dsi_mux_pads (or the omap5 dsi muxing that was recently
> > discussed) I still have no solution, but I haven't spent time on it. I
> > have dropped the omap5 dsi muxing from the latest series for that reason.

OK

> > dispc_disable_outputs and omap_dss_reset are needed because omap_device
> > doesn't handle resetting DSS properly, as special steps need to be done
> > for that. omap_dss_reset is called from the hwmod/omap_device code, so I
> > don't think this code can be anywhere else.

OK that we should be able to do with drivers/reset eventually. The reset
and idle of drivers is also needed for unused devices too, so we can't
have drivers do it in those cases.

> > For the omap_display_get_version() I have no good solution. Making
> > separate .dts versions for all the needed OMAP ES versions seems like a
> > huge hassle to me, but that is one solution.
> >
> > I think that would mean having separate .dtsi files for omapdss for
> > omap3_es1, omap3_es3plus, omap4_es1, omap4_es2, omap4_es3plus, and then
> > having separate omapxxxx.dtsi files for all of those, and then separate
> > board .dts files for the ES versions used on each board.
> >
> > Or is there some sane way to define such things in dts?
> >
> 
> Unfortunately there isn't.
> 
> I have a similar problem with the IGEP boards since there are just too
> many variations of them (e.g: DM3730 SoC vs OMAP3530 SoC, NAND vs
> OneNAND, 256MB vs 512MB flash memory sizes, with and without wifi
> chip, etc).
> 
> Since dts are supposed to describe the hardware, each revision with a
> slighter variation forces to have a different dts. My solution was to
> not try to have a dts for every single variation in mainline but only
> one for the most common revision and that could be used as a reference
> to modify and ship on each device. Unfortunately that is not possible
> to you since each DSS IP block is used by many boards.

Well ideally the revision info for a device would come from device
revision registers rather using the SoC revision. In the DSS case when
the SoC revision is needed by a device it maybe it can be deciphered
from a combination of compatible flag and the clock rate for example?

But yeah I agree we still need revision detection in the kernel
rather than try to create separate .dtsi files for sub-revisions.
 
> >> _any_ new crap into arch/arm/mach-omap2/display.c.
> >>
> >> So please consider this a huge eternal NAK to add any more crap to
> >> arch/arm/mach-omap2/display.c from me. No more new soc_is beyond
> >> the soc_is_am43xx() you have in linux next. No more adding
> >> of_find_compatible_node() beyond ti,omap5-dss you have in linux next.
> >>
> >> And no more new panel remapping in this file as soon as you have found
> >> a better solution. Preferrably by v3.17 merge window. This file just
> >> needs to disappear. Yuk.
> >
> > Do you object to the compatible string remapping as such, or just that
> > it's in arch/arm/mach-omap2?

It's something I'd rather not have under mach-omap2 as that means that
I may need to deal with it too to some extent. And I don't think we
need to do such remapping, we should be able to use the panel compatible
strings as they are just fine. It should be possible to figure out from
the device tree properties what controller the panel belongs to. Or
for now, use the panel registration to figure out what display controller
it belongs to.

> > I guess nothing prevents me from moving it to drivers/, and having some
> > early-ish initcall doing the job.

/me likes this idea if you need it at all. Stuff like this tends to stay
and spread, so I'd rather not do the remapping of compatible strings at
all.

> > If the remapping as such is horrible, I'm all ears for better ideas. I
> > spent quite a lot of time on it, and that's the best I could come up with.
> >
> > It's rather simple prefixing of the compatible strings for selected
> > devices. The purpose of that is to have proper data in the .dts files
> > (which I think is very important) with the cost of having some rather
> > simple internal hacks in the kernel, which can be removed immediately
> > when we have a common display driver framework.
> >
> 
> Even though the compatible trick that I said before could be an
> alternative, it has the problem that does not describes the hardware
> as you said. The restriction of the DT being a stable API and get it
> right from the beginning forces us to make these kind of technical
> decisions.
> 
> So, since we can change the kernel later but not the DTS, I agree with
> you that the remapping is the least bad of our options.

Yes the binding for the panel should just describe the panel so it can be
used with whatever display controller. But we do have quite a few buses
probing devices. How about set up the panel probing the same way?

For the panels on display controller, just do the usual
for_each_child_of_node(pdev->dev.of_node, child) and probe them?

It seems the remapping of compatible strings is not needed in this
as we're only picking up panels that are children of the display
controller.
 
> >>> I'm not sure what it would give us to try to be compatible with
> >>> simple-panel.txt. The simple-panel bindings won't probably be compatible
> >>> with the future common display drivers in any case, as the simple-panel
> >>> binding is just too limited and doesn't describe the hardware fully.
> >>
> >> Hmm what else does a panel need where the existing binding cannot be
> >> extended?
> >
> > The existing simple-panel binding doesn't describe the connections of
> > the panel, i.e. its video input. I guess it can be extended, but I don't
> > see what the benefit is of trying to create new panel bindings
> > compatible with the simple-panel bindings. As I see, the simple-panel
> > bindings work only with very limited use cases, where the drivers make
> > assumptions. Simple panel bindings cannot be used with omapdss, nor can
> > it be used with the future common display framework.

Well it seems at least the reset and enable pin standard from that
binding can be kept.

> > But I'm not really familiar with using extending current bindings, and
> > making new bindings compatible with old ones. Can you explain why it'd
> > be good to have the sharp panel bindings compatible with simple-panel?
> > In what kind of scenario would it be used?

Ideally the panel binding just describes the panel and it should not
matter which display controller it is a child of.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen - May 14, 2014, 6:19 a.m.
On 13/05/14 18:25, Tony Lindgren wrote:

> Well ideally the revision info for a device would come from device
> revision registers rather using the SoC revision. In the DSS case when
> the SoC revision is needed by a device it maybe it can be deciphered
> from a combination of compatible flag and the clock rate for example?

I've been trying that. The HW guys didn't bother to update the DSS
revision registers, so they are useless. And, for example, the OMAP3 ES
difference is only about bitfield widths in two registers.

I tried writing "too long" value to the register on the earlier ES
version, hoping that the extra bits would be kept at zero, but that
wasn't the case. So I just don't see a way to detect this from the DSS's
point of view.

>>> Do you object to the compatible string remapping as such, or just that
>>> it's in arch/arm/mach-omap2?
> 
> It's something I'd rather not have under mach-omap2 as that means that
> I may need to deal with it too to some extent. And I don't think we
> need to do such remapping, we should be able to use the panel compatible
> strings as they are just fine. It should be possible to figure out from
> the device tree properties what controller the panel belongs to. Or
> for now, use the panel registration to figure out what display controller
> it belongs to.
> 
>>> I guess nothing prevents me from moving it to drivers/, and having some
>>> early-ish initcall doing the job.
> 
> /me likes this idea if you need it at all. Stuff like this tends to stay
> and spread, so I'd rather not do the remapping of compatible strings at
> all.

Yep. I'll look to this. Thinking about it now, it kind of makes more
sense to have it in the omapdss's directory.

>> So, since we can change the kernel later but not the DTS, I agree with
>> you that the remapping is the least bad of our options.
> 
> Yes the binding for the panel should just describe the panel so it can be
> used with whatever display controller. But we do have quite a few buses
> probing devices. How about set up the panel probing the same way?

> For the panels on display controller, just do the usual
> for_each_child_of_node(pdev->dev.of_node, child) and probe them?
>
> It seems the remapping of compatible strings is not needed in this
> as we're only picking up panels that are children of the display
> controller.

The panels (or display encoders) are not (usually) children of the
display controller. They are children of their respective control bus.
Say, an i2c panel is a child of i2c bus. If there's no control bus, like
is the case with the sharp panel, it's a platform device.

The video paths of the panels and encoders are connected using the v4l2
style ports/endpoints. We can use those to see what display controller a
panel is connected to, but only after the panel driver has already
probed. We don't have control for the actual probing, as that happens
with whatever the control bus is for the display component.

>>>>> I'm not sure what it would give us to try to be compatible with
>>>>> simple-panel.txt. The simple-panel bindings won't probably be compatible
>>>>> with the future common display drivers in any case, as the simple-panel
>>>>> binding is just too limited and doesn't describe the hardware fully.
>>>>
>>>> Hmm what else does a panel need where the existing binding cannot be
>>>> extended?
>>>
>>> The existing simple-panel binding doesn't describe the connections of
>>> the panel, i.e. its video input. I guess it can be extended, but I don't
>>> see what the benefit is of trying to create new panel bindings
>>> compatible with the simple-panel bindings. As I see, the simple-panel
>>> bindings work only with very limited use cases, where the drivers make
>>> assumptions. Simple panel bindings cannot be used with omapdss, nor can
>>> it be used with the future common display framework.
> 
> Well it seems at least the reset and enable pin standard from that
> binding can be kept.

Only enable gpio there. But even that's vague. Do you turn on the power
before or after setting the enable gpio? How long delay should be
between the power and the gpio? Different panels have different rules
for the power-up.

>>> But I'm not really familiar with using extending current bindings, and
>>> making new bindings compatible with old ones. Can you explain why it'd
>>> be good to have the sharp panel bindings compatible with simple-panel?
>>> In what kind of scenario would it be used?
> 
> Ideally the panel binding just describes the panel and it should not
> matter which display controller it is a child of.

Yes, but that means the panel bindings need to have enough information
so that all display controllers can use it. Simple-panel bindings do not
have enough information. The simple-panel bindings do not have
information about the video bus input, and it doesn't even have
information about the resolution or bitdepth of the panel.

So I'm still asking, if we create sharp bindings that use the same
properties as the simple-panel bindings, and define that sharp panel is
compatible with simple-panel, what kind of scenario in practice would it
be used in?

Would the scenario be some other OS, that doesn't have a driver for the
sharp panel, but has a driver for the simple-panel? That would only work
if the sharp panel hardware is setup so that only the enable gpio is
needed, so that quite a narrow definition of "compatible".

Or is there some other scenario in which it could be used?

 Tomi
Tony Lindgren - May 14, 2014, 4:02 p.m.
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140513 23:20]:
> On 13/05/14 18:25, Tony Lindgren wrote:
> 
> > Well ideally the revision info for a device would come from device
> > revision registers rather using the SoC revision. In the DSS case when
> > the SoC revision is needed by a device it maybe it can be deciphered
> > from a combination of compatible flag and the clock rate for example?
> 
> I've been trying that. The HW guys didn't bother to update the DSS
> revision registers, so they are useless. And, for example, the OMAP3 ES
> difference is only about bitfield widths in two registers.
> 
> I tried writing "too long" value to the register on the earlier ES
> version, hoping that the extra bits would be kept at zero, but that
> wasn't the case. So I just don't see a way to detect this from the DSS's
> point of view.
> 
> >>> Do you object to the compatible string remapping as such, or just that
> >>> it's in arch/arm/mach-omap2?
> > 
> > It's something I'd rather not have under mach-omap2 as that means that
> > I may need to deal with it too to some extent. And I don't think we
> > need to do such remapping, we should be able to use the panel compatible
> > strings as they are just fine. It should be possible to figure out from
> > the device tree properties what controller the panel belongs to. Or
> > for now, use the panel registration to figure out what display controller
> > it belongs to.
> > 
> >>> I guess nothing prevents me from moving it to drivers/, and having some
> >>> early-ish initcall doing the job.
> > 
> > /me likes this idea if you need it at all. Stuff like this tends to stay
> > and spread, so I'd rather not do the remapping of compatible strings at
> > all.
> 
> Yep. I'll look to this. Thinking about it now, it kind of makes more
> sense to have it in the omapdss's directory.

OK thanks.
 
> >> So, since we can change the kernel later but not the DTS, I agree with
> >> you that the remapping is the least bad of our options.
> > 
> > Yes the binding for the panel should just describe the panel so it can be
> > used with whatever display controller. But we do have quite a few buses
> > probing devices. How about set up the panel probing the same way?
> 
> > For the panels on display controller, just do the usual
> > for_each_child_of_node(pdev->dev.of_node, child) and probe them?
> >
> > It seems the remapping of compatible strings is not needed in this
> > as we're only picking up panels that are children of the display
> > controller.
> 
> The panels (or display encoders) are not (usually) children of the
> display controller. They are children of their respective control bus.
> Say, an i2c panel is a child of i2c bus. If there's no control bus, like
> is the case with the sharp panel, it's a platform device.

OK
 
> The video paths of the panels and encoders are connected using the v4l2
> style ports/endpoints. We can use those to see what display controller a
> panel is connected to, but only after the panel driver has already
> probed. We don't have control for the actual probing, as that happens
> with whatever the control bus is for the display component.

OK. So with generic panels, you can just let the panels probe with
the right compatible flag then and let the ports/endpoints registration
to figure out if the panel is usable for the display controller in
question.

> >>>>> I'm not sure what it would give us to try to be compatible with
> >>>>> simple-panel.txt. The simple-panel bindings won't probably be compatible
> >>>>> with the future common display drivers in any case, as the simple-panel
> >>>>> binding is just too limited and doesn't describe the hardware fully.
> >>>>
> >>>> Hmm what else does a panel need where the existing binding cannot be
> >>>> extended?
> >>>
> >>> The existing simple-panel binding doesn't describe the connections of
> >>> the panel, i.e. its video input. I guess it can be extended, but I don't
> >>> see what the benefit is of trying to create new panel bindings
> >>> compatible with the simple-panel bindings. As I see, the simple-panel
> >>> bindings work only with very limited use cases, where the drivers make
> >>> assumptions. Simple panel bindings cannot be used with omapdss, nor can
> >>> it be used with the future common display framework.
> > 
> > Well it seems at least the reset and enable pin standard from that
> > binding can be kept.
> 
> Only enable gpio there. But even that's vague. Do you turn on the power
> before or after setting the enable gpio? How long delay should be
> between the power and the gpio? Different panels have different rules
> for the power-up.

Sure, it's a complex problem. But for the enable gpio..

Maybe the enable GPIO should be treated as a regulator? That would allow
specifying first the source regulator startup delay, and then the
panel has it's own startup delay.
 
> >>> But I'm not really familiar with using extending current bindings, and
> >>> making new bindings compatible with old ones. Can you explain why it'd
> >>> be good to have the sharp panel bindings compatible with simple-panel?
> >>> In what kind of scenario would it be used?
> > 
> > Ideally the panel binding just describes the panel and it should not
> > matter which display controller it is a child of.
> 
> Yes, but that means the panel bindings need to have enough information
> so that all display controllers can use it. Simple-panel bindings do not
> have enough information. The simple-panel bindings do not have
> information about the video bus input, and it doesn't even have
> information about the resolution or bitdepth of the panel.

Some of that you can hide into the panel driver based on the compatible
flag. So why not already do something like this in the .dts files
instead of the remapping:

compatible = "sharp,ls037v7dw01-omap-dss", "sharp,ls037v7dw01"; 

And drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c
would only claim to be compatible with "sharp,ls037v7dw01-omap-dss".

Then when the common panel framework is available, you can stop
parsing sharp,ls037v7dw01-omap-dss but the .dts files don't need
to be changed and it's fine to keep "sharp,ls037v7dw01-omap-dss"
in the .dts files.
 
> So I'm still asking, if we create sharp bindings that use the same
> properties as the simple-panel bindings, and define that sharp panel is
> compatible with simple-panel, what kind of scenario in practice would it
> be used in?

Well with the above example, just by dss with "sharp,ls037v7dw01-omap-dss"
until some other SoC notices it can use the GPIO parts of the panel
code at least :)
 
> Would the scenario be some other OS, that doesn't have a driver for the
> sharp panel, but has a driver for the simple-panel? That would only work
> if the sharp panel hardware is setup so that only the enable gpio is
> needed, so that quite a narrow definition of "compatible".

That's where we can use the compatible flags and just avoid parsing
the generic compatible flag unless some common framework is available.
 
> Or is there some other scenario in which it could be used?

I guess other OS or other SoC with a different display controller
but the same panel.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen - May 15, 2014, 9:23 a.m.
On 14/05/14 19:02, Tony Lindgren wrote:

>> The video paths of the panels and encoders are connected using the v4l2
>> style ports/endpoints. We can use those to see what display controller a
>> panel is connected to, but only after the panel driver has already
>> probed. We don't have control for the actual probing, as that happens
>> with whatever the control bus is for the display component.
> 
> OK. So with generic panels, you can just let the panels probe with
> the right compatible flag then and let the ports/endpoints registration
> to figure out if the panel is usable for the display controller in
> question.

I'm not sure what you mean here.

Do you mean with the future common display framework? There's no need to
figure out anything there, as supposedly the .dts has been written
correctly and the panel and the display controller work together.

If you mean with the current kernel, there's still nothing to figure
out. We can have only single driver with a particular compatible string.
And as our current drivers are omap specific, it makes sense to have
their compatible string be something omap-ish. And if the .dts file
connects the display controller and the panel, then they must be usable
together.

>>> Well it seems at least the reset and enable pin standard from that
>>> binding can be kept.
>>
>> Only enable gpio there. But even that's vague. Do you turn on the power
>> before or after setting the enable gpio? How long delay should be
>> between the power and the gpio? Different panels have different rules
>> for the power-up.
> 
> Sure, it's a complex problem. But for the enable gpio..
> 
> Maybe the enable GPIO should be treated as a regulator? That would allow
> specifying first the source regulator startup delay, and then the
> panel has it's own startup delay.

Well... I don't know. Sounds rather hacky to me. We have the option to
have a specific driver for this panel, and that driver can handle all
the delays and power-up sequences just right in a clean manner.

>>>>> But I'm not really familiar with using extending current bindings, and
>>>>> making new bindings compatible with old ones. Can you explain why it'd
>>>>> be good to have the sharp panel bindings compatible with simple-panel?
>>>>> In what kind of scenario would it be used?
>>>
>>> Ideally the panel binding just describes the panel and it should not
>>> matter which display controller it is a child of.
>>
>> Yes, but that means the panel bindings need to have enough information
>> so that all display controllers can use it. Simple-panel bindings do not
>> have enough information. The simple-panel bindings do not have
>> information about the video bus input, and it doesn't even have
>> information about the resolution or bitdepth of the panel.
> 
> Some of that you can hide into the panel driver based on the compatible
> flag. So why not already do something like this in the .dts files
> instead of the remapping:
> 
> compatible = "sharp,ls037v7dw01-omap-dss", "sharp,ls037v7dw01"; 
> 
> And drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c
> would only claim to be compatible with "sharp,ls037v7dw01-omap-dss".
> 
> Then when the common panel framework is available, you can stop
> parsing sharp,ls037v7dw01-omap-dss but the .dts files don't need
> to be changed and it's fine to keep "sharp,ls037v7dw01-omap-dss"
> in the .dts files.

Hmm, I don't see how this relates to the simple-panel bindings.

But you're right, having "sharp,ls037v7dw01-omap-dss" in the .dts is an
alternative for the compatible-string conversion we do now. I guess it's
a matter of taste, but I rather hide it inside the kernel, in an
internal omapdss file, than pollute the .dts files with those compatible
strings.

>> So I'm still asking, if we create sharp bindings that use the same
>> properties as the simple-panel bindings, and define that sharp panel is
>> compatible with simple-panel, what kind of scenario in practice would it
>> be used in?
> 
> Well with the above example, just by dss with "sharp,ls037v7dw01-omap-dss"
> until some other SoC notices it can use the GPIO parts of the panel
> code at least :)
>  
>> Would the scenario be some other OS, that doesn't have a driver for the
>> sharp panel, but has a driver for the simple-panel? That would only work
>> if the sharp panel hardware is setup so that only the enable gpio is
>> needed, so that quite a narrow definition of "compatible".
> 
> That's where we can use the compatible flags and just avoid parsing
> the generic compatible flag unless some common framework is available.

Hmm, sorry, I don't follow. My question was about the simple-panel
bindings, not common display framework.

You were saying that the sharp bindings should be compatible with
simple-panel bindings. I'm still trying to understand what benefit does
that give us.

As I see it, the sharp panel could be used with the simple-panel
bindings only in certain special case, where all the mode pins and the
reset are hardwired in the board hardware, and they are hardwired in a
certain state (all hardwired low, probably), which matches what the
simple-panel driver expects.

 Tomi
Tony Lindgren - May 15, 2014, 6:21 p.m.
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140515 02:24]:
> On 14/05/14 19:02, Tony Lindgren wrote:
> 
> >> The video paths of the panels and encoders are connected using the v4l2
> >> style ports/endpoints. We can use those to see what display controller a
> >> panel is connected to, but only after the panel driver has already
> >> probed. We don't have control for the actual probing, as that happens
> >> with whatever the control bus is for the display component.
> > 
> > OK. So with generic panels, you can just let the panels probe with
> > the right compatible flag then and let the ports/endpoints registration
> > to figure out if the panel is usable for the display controller in
> > question.
> 
> I'm not sure what you mean here.
> 
> Do you mean with the future common display framework? There's no need to
> figure out anything there, as supposedly the .dts has been written
> correctly and the panel and the display controller work together.

OK. Yes I meant for future use.
 
> If you mean with the current kernel, there's still nothing to figure
> out. We can have only single driver with a particular compatible string.
> And as our current drivers are omap specific, it makes sense to have
> their compatible string be something omap-ish. And if the .dts file
> connects the display controller and the panel, then they must be usable
> together.

Yup.
 
> >>> Well it seems at least the reset and enable pin standard from that
> >>> binding can be kept.
> >>
> >> Only enable gpio there. But even that's vague. Do you turn on the power
> >> before or after setting the enable gpio? How long delay should be
> >> between the power and the gpio? Different panels have different rules
> >> for the power-up.
> > 
> > Sure, it's a complex problem. But for the enable gpio..
> > 
> > Maybe the enable GPIO should be treated as a regulator? That would allow
> > specifying first the source regulator startup delay, and then the
> > panel has it's own startup delay.
> 
> Well... I don't know. Sounds rather hacky to me. We have the option to
> have a specific driver for this panel, and that driver can handle all
> the delays and power-up sequences just right in a clean manner.

I guess we could have a generic startup-latency property for the GPIOs.
 
> >>>>> But I'm not really familiar with using extending current bindings, and
> >>>>> making new bindings compatible with old ones. Can you explain why it'd
> >>>>> be good to have the sharp panel bindings compatible with simple-panel?
> >>>>> In what kind of scenario would it be used?
> >>>
> >>> Ideally the panel binding just describes the panel and it should not
> >>> matter which display controller it is a child of.
> >>
> >> Yes, but that means the panel bindings need to have enough information
> >> so that all display controllers can use it. Simple-panel bindings do not
> >> have enough information. The simple-panel bindings do not have
> >> information about the video bus input, and it doesn't even have
> >> information about the resolution or bitdepth of the panel.
> > 
> > Some of that you can hide into the panel driver based on the compatible
> > flag. So why not already do something like this in the .dts files
> > instead of the remapping:
> > 
> > compatible = "sharp,ls037v7dw01-omap-dss", "sharp,ls037v7dw01"; 
> > 
> > And drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c
> > would only claim to be compatible with "sharp,ls037v7dw01-omap-dss".
> > 
> > Then when the common panel framework is available, you can stop
> > parsing sharp,ls037v7dw01-omap-dss but the .dts files don't need
> > to be changed and it's fine to keep "sharp,ls037v7dw01-omap-dss"
> > in the .dts files.
> 
> Hmm, I don't see how this relates to the simple-panel bindings.
> 
> But you're right, having "sharp,ls037v7dw01-omap-dss" in the .dts is an
> alternative for the compatible-string conversion we do now. I guess it's
> a matter of taste, but I rather hide it inside the kernel, in an
> internal omapdss file, than pollute the .dts files with those compatible
> strings.

Well it avoid you parsing through all the nodes during booting
and leaves out the function to do remapping. And removes the need
for maintaining a custom display mapping table. I'd say that's a
pretty good list of advantages right there :)
 
> >> So I'm still asking, if we create sharp bindings that use the same
> >> properties as the simple-panel bindings, and define that sharp panel is
> >> compatible with simple-panel, what kind of scenario in practice would it
> >> be used in?
> > 
> > Well with the above example, just by dss with "sharp,ls037v7dw01-omap-dss"
> > until some other SoC notices it can use the GPIO parts of the panel
> > code at least :)
> >  
> >> Would the scenario be some other OS, that doesn't have a driver for the
> >> sharp panel, but has a driver for the simple-panel? That would only work
> >> if the sharp panel hardware is setup so that only the enable gpio is
> >> needed, so that quite a narrow definition of "compatible".
> > 
> > That's where we can use the compatible flags and just avoid parsing
> > the generic compatible flag unless some common framework is available.
> 
> Hmm, sorry, I don't follow. My question was about the simple-panel
> bindings, not common display framework.
> 
> You were saying that the sharp bindings should be compatible with
> simple-panel bindings. I'm still trying to understand what benefit does
> that give us.

Oh sorry, for that part I don't really have an opinion. If you think
the simple-panel binding is not going to be usable in the long
run, then I'm fine with whatever binding you display guys come up
with and prefer to use.
 
> As I see it, the sharp panel could be used with the simple-panel
> bindings only in certain special case, where all the mode pins and the
> reset are hardwired in the board hardware, and they are hardwired in a
> certain state (all hardwired low, probably), which matches what the
> simple-panel driver expects.

OK. Maybe take a stab at updating the binding for this as I don't
know what you want to do with it?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen - May 16, 2014, 5:56 a.m.
On 15/05/14 21:21, Tony Lindgren wrote:

>> But you're right, having "sharp,ls037v7dw01-omap-dss" in the .dts is an
>> alternative for the compatible-string conversion we do now. I guess it's
>> a matter of taste, but I rather hide it inside the kernel, in an
>> internal omapdss file, than pollute the .dts files with those compatible
>> strings.
> 
> Well it avoid you parsing through all the nodes during booting
> and leaves out the function to do remapping. And removes the need
> for maintaining a custom display mapping table. I'd say that's a
> pretty good list of advantages right there :)

Yep... I don't know. Maybe I'm being too careful about doing wrong
things with .dts. I just like it more if any hacks are in kernel code,
which I can remove without anyone noticing.

Anyway, we already have board.dts files using the non-omapified
compatible strings in the mainline, so if I would now add the omapified
compatible strings to .dts files, those old board.dts files would break.

So I guess the choice has already been made.

 Tomi
Tony Lindgren - May 16, 2014, 4:07 p.m.
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140515 22:57]:
> On 15/05/14 21:21, Tony Lindgren wrote:
> 
> >> But you're right, having "sharp,ls037v7dw01-omap-dss" in the .dts is an
> >> alternative for the compatible-string conversion we do now. I guess it's
> >> a matter of taste, but I rather hide it inside the kernel, in an
> >> internal omapdss file, than pollute the .dts files with those compatible
> >> strings.
> > 
> > Well it avoid you parsing through all the nodes during booting
> > and leaves out the function to do remapping. And removes the need
> > for maintaining a custom display mapping table. I'd say that's a
> > pretty good list of advantages right there :)
> 
> Yep... I don't know. Maybe I'm being too careful about doing wrong
> things with .dts. I just like it more if any hacks are in kernel code,
> which I can remove without anyone noticing.
> 
> Anyway, we already have board.dts files using the non-omapified
> compatible strings in the mainline, so if I would now add the omapified
> compatible strings to .dts files, those old board.dts files would break.
> 
> So I guess the choice has already been made.

I really think you should remove this misuse of device tree code ASAP.
It's better to fix it up now than carry the hack for next two years
and keep on adding to it.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Reichel - May 16, 2014, 5:41 p.m.
On Fri, May 16, 2014 at 09:07:17AM -0700, Tony Lindgren wrote:
> * Tomi Valkeinen <tomi.valkeinen@ti.com> [140515 22:57]:
> > On 15/05/14 21:21, Tony Lindgren wrote:
> > >> But you're right, having "sharp,ls037v7dw01-omap-dss" in the .dts is an
> > >> alternative for the compatible-string conversion we do now. I guess it's
> > >> a matter of taste, but I rather hide it inside the kernel, in an
> > >> internal omapdss file, than pollute the .dts files with those compatible
> > >> strings.
> > > 
> > > Well it avoid you parsing through all the nodes during booting
> > > and leaves out the function to do remapping. And removes the need
> > > for maintaining a custom display mapping table. I'd say that's a
> > > pretty good list of advantages right there :)
> > 
> > Yep... I don't know. Maybe I'm being too careful about doing wrong
> > things with .dts. I just like it more if any hacks are in kernel code,
> > which I can remove without anyone noticing.
> > 
> > Anyway, we already have board.dts files using the non-omapified
> > compatible strings in the mainline, so if I would now add the omapified
> > compatible strings to .dts files, those old board.dts files would break.
> > 
> > So I guess the choice has already been made.
> 
> I really think you should remove this misuse of device tree code ASAP.
> It's better to fix it up now than carry the hack for next two years
> and keep on adding to it.

IMHO appending -omap-dss to a random device is an even bigger hack,
since its adding lots of bloat to the API. Let's assume there is
another OS using DT for ARM, but has no proper API for SPI
controllers and it introduces your hack to SPI devices. That would
mean each SPI device has -omap-spi appended (or -exynos-spi,
-foo-spi, ...). At least I would blame them for creating a huge
unmaintainable mess.

I think Tomi's workaround is a much better hack, since it keeps the
API clean. If the code simply prefixes "omapdss," to all
"child"-devices of omapdss it can be left mostly untouched, too.

-- Sebastian
Tony Lindgren - May 16, 2014, 6:01 p.m.
* Sebastian Reichel <sre@ring0.de> [140516 10:42]:
> On Fri, May 16, 2014 at 09:07:17AM -0700, Tony Lindgren wrote:
> > * Tomi Valkeinen <tomi.valkeinen@ti.com> [140515 22:57]:
> > > On 15/05/14 21:21, Tony Lindgren wrote:
> > > >> But you're right, having "sharp,ls037v7dw01-omap-dss" in the .dts is an
> > > >> alternative for the compatible-string conversion we do now. I guess it's
> > > >> a matter of taste, but I rather hide it inside the kernel, in an
> > > >> internal omapdss file, than pollute the .dts files with those compatible
> > > >> strings.
> > > > 
> > > > Well it avoid you parsing through all the nodes during booting
> > > > and leaves out the function to do remapping. And removes the need
> > > > for maintaining a custom display mapping table. I'd say that's a
> > > > pretty good list of advantages right there :)
> > > 
> > > Yep... I don't know. Maybe I'm being too careful about doing wrong
> > > things with .dts. I just like it more if any hacks are in kernel code,
> > > which I can remove without anyone noticing.
> > > 
> > > Anyway, we already have board.dts files using the non-omapified
> > > compatible strings in the mainline, so if I would now add the omapified
> > > compatible strings to .dts files, those old board.dts files would break.
> > > 
> > > So I guess the choice has already been made.
> > 
> > I really think you should remove this misuse of device tree code ASAP.
> > It's better to fix it up now than carry the hack for next two years
> > and keep on adding to it.
> 
> IMHO appending -omap-dss to a random device is an even bigger hack,
> since its adding lots of bloat to the API. Let's assume there is
> another OS using DT for ARM, but has no proper API for SPI
> controllers and it introduces your hack to SPI devices. That would
> mean each SPI device has -omap-spi appended (or -exynos-spi,
> -foo-spi, ...). At least I would blame them for creating a huge
> unmaintainable mess.

I think you're misunderstanding. I do not want the naming to
be Linux specific. The naming should naturally be as hardware
specific as possible. In this case something like:

compatible = "sharp,ls037v7dw01-dss", "sharp,ls037v7dw01";

Or we should probably use:

compatible = "sharp,ls037v7dw01-dpi", "sharp,ls037v7dw01";

As dpi here reflects the hardware it's connected to. The dss
is probably a Linux name.

Not use what you're after with the SPI example though, but sounds
like that's something different.

> I think Tomi's workaround is a much better hack, since it keeps the
> API clean. If the code simply prefixes "omapdss," to all
> "child"-devices of omapdss it can be left mostly untouched, too.

AFAIK that remapping not needed at all. All that is already
available with the compatible flags.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren - May 16, 2014, 9:39 p.m.
* Tony Lindgren <tony@atomide.com> [140516 11:02]:
> * Sebastian Reichel <sre@ring0.de> [140516 10:42]:
> > On Fri, May 16, 2014 at 09:07:17AM -0700, Tony Lindgren wrote:
> > > * Tomi Valkeinen <tomi.valkeinen@ti.com> [140515 22:57]:
> > > > On 15/05/14 21:21, Tony Lindgren wrote:
> > > > >> But you're right, having "sharp,ls037v7dw01-omap-dss" in the .dts is an
> > > > >> alternative for the compatible-string conversion we do now. I guess it's
> > > > >> a matter of taste, but I rather hide it inside the kernel, in an
> > > > >> internal omapdss file, than pollute the .dts files with those compatible
> > > > >> strings.
> > > > > 
> > > > > Well it avoid you parsing through all the nodes during booting
> > > > > and leaves out the function to do remapping. And removes the need
> > > > > for maintaining a custom display mapping table. I'd say that's a
> > > > > pretty good list of advantages right there :)
> > > > 
> > > > Yep... I don't know. Maybe I'm being too careful about doing wrong
> > > > things with .dts. I just like it more if any hacks are in kernel code,
> > > > which I can remove without anyone noticing.
> > > > 
> > > > Anyway, we already have board.dts files using the non-omapified
> > > > compatible strings in the mainline, so if I would now add the omapified
> > > > compatible strings to .dts files, those old board.dts files would break.
> > > > 
> > > > So I guess the choice has already been made.
> > > 
> > > I really think you should remove this misuse of device tree code ASAP.
> > > It's better to fix it up now than carry the hack for next two years
> > > and keep on adding to it.
> > 
> > IMHO appending -omap-dss to a random device is an even bigger hack,
> > since its adding lots of bloat to the API. Let's assume there is
> > another OS using DT for ARM, but has no proper API for SPI
> > controllers and it introduces your hack to SPI devices. That would
> > mean each SPI device has -omap-spi appended (or -exynos-spi,
> > -foo-spi, ...). At least I would blame them for creating a huge
> > unmaintainable mess.
> 
> I think you're misunderstanding. I do not want the naming to
> be Linux specific. The naming should naturally be as hardware
> specific as possible. In this case something like:
> 
> compatible = "sharp,ls037v7dw01-dss", "sharp,ls037v7dw01";
> 
> Or we should probably use:
> 
> compatible = "sharp,ls037v7dw01-dpi", "sharp,ls037v7dw01";
> 
> As dpi here reflects the hardware it's connected to. The dss
> is probably a Linux name.
> 
> Not use what you're after with the SPI example though, but sounds
> like that's something different.
> 
> > I think Tomi's workaround is a much better hack, since it keeps the
> > API clean. If the code simply prefixes "omapdss," to all
> > "child"-devices of omapdss it can be left mostly untouched, too.
> 
> AFAIK that remapping not needed at all. All that is already
> available with the compatible flags.

And alternatively we can also just use the sharp,ls037v7dw01
in the panel driver(s). We can have the panels bail out in driver
probe based on of_machine_is_compatible if nothing else is available.

Also, currently the remapping code also probably keeps prepending
more and more omapdss,omapdss,omapdss,... on each kexec reboot? And
it would be quite silly for each display controller to have to do
their own remapping for shared panels?

If we still still despite all these reasons decide to go with
the remapping of the compatible strings, it should really be a
Linux generic (or drivers/video generic function), not implemented
for each display controller.

Cheers,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen - May 19, 2014, 9:21 a.m.
On 16/05/14 21:01, Tony Lindgren wrote:

>> IMHO appending -omap-dss to a random device is an even bigger hack,
>> since its adding lots of bloat to the API. Let's assume there is
>> another OS using DT for ARM, but has no proper API for SPI
>> controllers and it introduces your hack to SPI devices. That would
>> mean each SPI device has -omap-spi appended (or -exynos-spi,
>> -foo-spi, ...). At least I would blame them for creating a huge
>> unmaintainable mess.
> 
> I think you're misunderstanding. I do not want the naming to
> be Linux specific. The naming should naturally be as hardware
> specific as possible. In this case something like:
> 
> compatible = "sharp,ls037v7dw01-dss", "sharp,ls037v7dw01";
> 
> Or we should probably use:
> 
> compatible = "sharp,ls037v7dw01-dpi", "sharp,ls037v7dw01";
> 
> As dpi here reflects the hardware it's connected to. The dss
> is probably a Linux name.

Well, "dss" or "omapdss" is as much a hardware term as "dpi". And "dpi"
wouldn't really be a good extension, as what we want is an omapdss
driver specific compatible string. So I think "-omapdss" is the best
extension if that method is used.

But I don't think that's really the point. The point is that the panel's
compatible string should be "sharp,ls037v7dw01", nothing else.

All the variations of "sharp,ls037v7dw01-dss" are not correct, and are
only made for Linux SW reasons. So I would say they are Linux specific
SW names, even if the words themselves are also HW terms.

> Not use what you're after with the SPI example though, but sounds
> like that's something different.

I think Sebastien's example is just like the issue here.

 Tomi
Tomi Valkeinen - May 19, 2014, 9:48 a.m.
On 17/05/14 00:39, Tony Lindgren wrote:

>> AFAIK that remapping not needed at all. All that is already
>> available with the compatible flags.
> 
> And alternatively we can also just use the sharp,ls037v7dw01
> in the panel driver(s). We can have the panels bail out in driver
> probe based on of_machine_is_compatible if nothing else is available.

I don't think that would work. Then we could have two panel drivers,
both loaded into the kernel and both having the same driver name and the
same compatible-string.

I'm not sure if it's even possible to load both of those drivers at the
same time, but if it was, and the first one would get probe() called for
a device, and the driver would return an error, I'm quite sure the
driver framework won't continue trying with the other driver, as the
first driver already matched for the device.

> Also, currently the remapping code also probably keeps prepending
> more and more omapdss,omapdss,omapdss,... on each kexec reboot? And

I don't think so. The code looks for, say, "ti,tfp410", and it wouldn't
match for "omapdss,ti,tfp410".

> it would be quite silly for each display controller to have to do
> their own remapping for shared panels?

It would, but wouldn't it be just as silly (well, even more silly in my
opinion) to have each display controller require driver specific
compatible strings for the panels used?

Until we have a common display framework, we need _some_ way to handle
this problem. Either we mess up the .dts files as you suggest, or we
have a driver internal hack.

> If we still still despite all these reasons decide to go with
> the remapping of the compatible strings, it should really be a
> Linux generic (or drivers/video generic function), not implemented
> for each display controller.

Well, I sure hope nobody else has to implement similar thing.

The reason why we have this code, and others do not, is mainly that
omapdss is maybe the first driver to implement the display components
properly, both in the SW side and in the .dts side, and that I wanted
omapdss to behave correctly even if there are other display
subsystems/panels compiled into the same kernel.

To open that up a bit, traditionally other display driver architectures
have not had drivers for each display "entity", like the panels. So you
would really just have the display subsystem in the .dts, and some raw
data there (panel timings) to get the panel running.

Now, with omapdss, we have separate drivers for each display entity.
Unfortunately those drivers are all tied to the omapdss driver's API
(and cannot thus be used on anything else than OMAP), as there's not yet
a driver framework for display entities (DRM is a bit different thing,
in my opinion).

Everything with that was simple with platform data, as the omap board
files created the panel devices, and there were no name clashing
problems with other platforms.

With DT that changed, as the bindings are common, and thus the
compatible strings are common also. But we still have omapdss specific
panel drivers. This is the reason we do the compatible-string conversion
hack. As soon as we can create platform agnostic panel drivers, we can
remove the hack.

And, I want to remind, it's an omapdss internal hack (after the patch I
sent), not visible to anyone else. The .dts files, the arch/arm files,
they are all not touched. So I don't quite understand why you see it as
such a bad thing. It's ugly, sure, but what harm does it do (except some
maintainer burden, for me)?

 Tomi
Tony Lindgren - May 19, 2014, 3:57 p.m.
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140519 02:49]:
> On 17/05/14 00:39, Tony Lindgren wrote:
> 
> >> AFAIK that remapping not needed at all. All that is already
> >> available with the compatible flags.
> > 
> > And alternatively we can also just use the sharp,ls037v7dw01
> > in the panel driver(s). We can have the panels bail out in driver
> > probe based on of_machine_is_compatible if nothing else is available.
> 
> I don't think that would work. Then we could have two panel drivers,
> both loaded into the kernel and both having the same driver name and the
> same compatible-string.
> 
> I'm not sure if it's even possible to load both of those drivers at the
> same time, but if it was, and the first one would get probe() called for
> a device, and the driver would return an error, I'm quite sure the
> driver framework won't continue trying with the other driver, as the
> first driver already matched for the device.

No need to load multiple drivers at this point. That's why I'm saying
you can bail out on the undesired display controller probes using
of_machine_is_compatible test. It's not that uncommon for drivers to do:

$ git grep of_machine_is_compatible drivers/ | wc -l
116
 
> > Also, currently the remapping code also probably keeps prepending
> > more and more omapdss,omapdss,omapdss,... on each kexec reboot? And
> 
> I don't think so. The code looks for, say, "ti,tfp410", and it wouldn't
> match for "omapdss,ti,tfp410".

OK
 
> > it would be quite silly for each display controller to have to do
> > their own remapping for shared panels?
> 
> It would, but wouldn't it be just as silly (well, even more silly in my
> opinion) to have each display controller require driver specific
> compatible strings for the panels used?

Not driver specific, but hardware package specific. That's being done
all the time. For example, compatible strings like "nvidia,tegra124",
"ti,omap5" describe a package of an ARM core and various devices.

But by using of_machine_is_compatible in the display drivers, you
can use the right compatible flags from the start if you want to go
that way.

> Until we have a common display framework, we need _some_ way to handle
> this problem. Either we mess up the .dts files as you suggest, or we
> have a driver internal hack.

It seems that there's no need to mess up the .dts files here by
using the of_machine_is_compatible check in the drivers.
 
> > If we still still despite all these reasons decide to go with
> > the remapping of the compatible strings, it should really be a
> > Linux generic (or drivers/video generic function), not implemented
> > for each display controller.
> 
> Well, I sure hope nobody else has to implement similar thing.

Suuuure.. I've heard that about 20 times before and guess what
happenened? Things never got fixed until some poor maintainer
had to start fixing it all over the place about three years later.
 
> The reason why we have this code, and others do not, is mainly that
> omapdss is maybe the first driver to implement the display components
> properly, both in the SW side and in the .dts side, and that I wanted
> omapdss to behave correctly even if there are other display
> subsystems/panels compiled into the same kernel.
> 
> To open that up a bit, traditionally other display driver architectures
> have not had drivers for each display "entity", like the panels. So you
> would really just have the display subsystem in the .dts, and some raw
> data there (panel timings) to get the panel running.

Right, I believe that is the way to go, no doubt about it.
 
> Now, with omapdss, we have separate drivers for each display entity.
> Unfortunately those drivers are all tied to the omapdss driver's API
> (and cannot thus be used on anything else than OMAP), as there's not yet
> a driver framework for display entities (DRM is a bit different thing,
> in my opinion).
> 
> Everything with that was simple with platform data, as the omap board
> files created the panel devices, and there were no name clashing
> problems with other platforms.
> 
> With DT that changed, as the bindings are common, and thus the
> compatible strings are common also. But we still have omapdss specific
> panel drivers. This is the reason we do the compatible-string conversion
> hack. As soon as we can create platform agnostic panel drivers, we can
> remove the hack.
> 
> And, I want to remind, it's an omapdss internal hack (after the patch I
> sent), not visible to anyone else. The .dts files, the arch/arm files,
> they are all not touched. So I don't quite understand why you see it as
> such a bad thing. It's ugly, sure, but what harm does it do (except some
> maintainer burden, for me)?

Here's a list of things that bothers me:

1. Searching through the dtb from a driver instead of relying on the
   driver probe mechanism for the components in question. If the parsing
   of the dtb needs to be done it should be done in a Linux generic way
   from some framework instead.

2. Having to do this all early before we even have any debug console
   available. We are trying to move everything to happen later on to avoid
   all the issues related to initializing things early.

3. Having to maintain a database in kernel of display mappings separately
   in addition to the .dts files.

4. Having this hack limited to device tree based booting while we are
   trying to unify the functions for drivers to use to get their
   resources.

5. Seeing the possibility of this spreading to other drivers.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren - May 19, 2014, 4:04 p.m.
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140519 02:22]:
> On 16/05/14 21:01, Tony Lindgren wrote:
> 
> >> IMHO appending -omap-dss to a random device is an even bigger hack,
> >> since its adding lots of bloat to the API. Let's assume there is
> >> another OS using DT for ARM, but has no proper API for SPI
> >> controllers and it introduces your hack to SPI devices. That would
> >> mean each SPI device has -omap-spi appended (or -exynos-spi,
> >> -foo-spi, ...). At least I would blame them for creating a huge
> >> unmaintainable mess.
> > 
> > I think you're misunderstanding. I do not want the naming to
> > be Linux specific. The naming should naturally be as hardware
> > specific as possible. In this case something like:
> > 
> > compatible = "sharp,ls037v7dw01-dss", "sharp,ls037v7dw01";
> > 
> > Or we should probably use:
> > 
> > compatible = "sharp,ls037v7dw01-dpi", "sharp,ls037v7dw01";
> > 
> > As dpi here reflects the hardware it's connected to. The dss
> > is probably a Linux name.
> 
> Well, "dss" or "omapdss" is as much a hardware term as "dpi". And "dpi"
> wouldn't really be a good extension, as what we want is an omapdss
> driver specific compatible string. So I think "-omapdss" is the best
> extension if that method is used.
> 
> But I don't think that's really the point. The point is that the panel's
> compatible string should be "sharp,ls037v7dw01", nothing else.
> 
> All the variations of "sharp,ls037v7dw01-dss" are not correct, and are
> only made for Linux SW reasons. So I would say they are Linux specific
> SW names, even if the words themselves are also HW terms.

If you really want to use only "sharp,ls037v7dw01" as the compatible
string, then you can still do that and bail out from the undesired
drivers by using of_machine_is_compatible check.

In many cases however we do have multiple compatible strings that
describe how the device is wired. See drivers/tty/serial/of_serial.c
for example. It has "ns16550" but then it also has additional
"nvidia,tegra20-uart", "nxp,lpc3220-uart" and "ibm,qpace-nwp-serial".
 
> > Not use what you're after with the SPI example though, but sounds
> > like that's something different.
> 
> I think Sebastien's example is just like the issue here.

Hmm is there some existing example in the kernel like that?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - May 19, 2014, 4:43 p.m.
On Monday 19 May 2014 08:57:33 Tony Lindgren wrote:
> No need to load multiple drivers at this point. That's why I'm saying
> you can bail out on the undesired display controller probes using
> of_machine_is_compatible test. It's not that uncommon for drivers to do:
> 
> $ git grep of_machine_is_compatible drivers/ | wc -l
> 116

I think this is widely seen as bad style, and most of the examples are
testing for Power Mac specific workarounds for stuff that Apple didn't
put in DT elsewhere.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen - May 19, 2014, 6:57 p.m.
On 19/05/14 18:57, Tony Lindgren wrote:

>> I'm not sure if it's even possible to load both of those drivers at the
>> same time, but if it was, and the first one would get probe() called for
>> a device, and the driver would return an error, I'm quite sure the
>> driver framework won't continue trying with the other driver, as the
>> first driver already matched for the device.
> 
> No need to load multiple drivers at this point. That's why I'm saying
> you can bail out on the undesired display controller probes using
> of_machine_is_compatible test. It's not that uncommon for drivers to do:

Hmm, I don't follow. If both, say, omap and exynos have a panel driver
for the same sharp panel, both need a driver for it (presuming exynos
has a proper display component driver system, which may not be true).

Both drivers would be loaded at boot time for the probe to be even to be
possible. Even if the other one bails out from a device probe using
of_machine_is_compatible, afaik the driver framework will stop probing
at that point, as that driver reports being compatible with the device
in question.

...

Ah, ok, now I see. I think you mean module init, not probe? For some
reason I was just thinking about the probe stage.

Maybe that would work. I need to test tomorrow.

>>> it would be quite silly for each display controller to have to do
>>> their own remapping for shared panels?
>>
>> It would, but wouldn't it be just as silly (well, even more silly in my
>> opinion) to have each display controller require driver specific
>> compatible strings for the panels used?
> 
> Not driver specific, but hardware package specific. That's being done
> all the time. For example, compatible strings like "nvidia,tegra124",
> "ti,omap5" describe a package of an ARM core and various devices.

It feels to me rather wrong if I'd specify the compatible string for an
external piece of hardware based on the SoC it's connected to.

> But by using of_machine_is_compatible in the display drivers, you
> can use the right compatible flags from the start if you want to go
> that way.

Yes, with that we could have right compatible flags in the .dts.

>> Well, I sure hope nobody else has to implement similar thing.
> 
> Suuuure.. I've heard that about 20 times before and guess what
> happenened? Things never got fixed until some poor maintainer
> had to start fixing it all over the place about three years later.

The poor maintainer here being me =). And it's still a case of
pick-your-poison. We need to hack around the issue, in some form or
another. So in any case the poor maintainer has to fix it at some point
in the future.

But all this is fixed when we have a common display framework. And
that's something that is really badly needed in some form or another,
the sooner the better.

So the "fix" here is not just some internal thing that could be left at
that and forgotten.

>> To open that up a bit, traditionally other display driver architectures
>> have not had drivers for each display "entity", like the panels. So you
>> would really just have the display subsystem in the .dts, and some raw
>> data there (panel timings) to get the panel running.
> 
> Right, I believe that is the way to go, no doubt about it.

No, that's exactly _not_ the way to go =). We need proper drivers for
display components, instead of trying to avoid that and embed panel data
into the display controller's data.

> Here's a list of things that bothers me:
> 
> 1. Searching through the dtb from a driver instead of relying on the
>    driver probe mechanism for the components in question. If the parsing
>    of the dtb needs to be done it should be done in a Linux generic way
>    from some framework instead.

The reason I haven't made the conversion code available for others to
use is that there are no other users at the moment. And I hope there
will be no other user until we get the common display framework. If
there is, we'll need to move the code to a generic place.

> 2. Having to do this all early before we even have any debug console
>    available. We are trying to move everything to happen later on to avoid
>    all the issues related to initializing things early.

It worked fine for me when doing it as subsys_init level. Why do you say
it'd be early, before debug console?

All the drivers with converted compatible strings are normal drivers, so
afaik things work fine if we just convert the strings before the module
init level.

> 3. Having to maintain a database in kernel of display mappings separately
>    in addition to the .dts files.

Yes, that's slightly annoying. Not really a big burden, though, as it's
quite rare to get a new encoder or panel driver.

> 4. Having this hack limited to device tree based booting while we are
>    trying to unify the functions for drivers to use to get their
>    resources.

I don't understand this one. With non-DT boot we don't have the issue at
all, there's no need for hacks.

> 5. Seeing the possibility of this spreading to other drivers.

Well, until we have a common display framework, one of the hacky options
we've discussed will spread to other drivers if they need to have their
own drivers for the same hardware devices.

Which is not nice, but not something we can escape. And using the
of_machine_is_compatible or having the compatible strings in .dts
appended with "dss" or such doesn't change that, it just changes which
hack may spread.

 Tomi
Tomi Valkeinen - May 19, 2014, 7:05 p.m.
On 19/05/14 19:04, Tony Lindgren wrote:

> In many cases however we do have multiple compatible strings that
> describe how the device is wired. See drivers/tty/serial/of_serial.c
> for example. It has "ns16550" but then it also has additional
> "nvidia,tegra20-uart", "nxp,lpc3220-uart" and "ibm,qpace-nwp-serial".

All those sound like SoC components. In that case it sounds fine to have
the device compatible contain the SoC name. We're talking here about
external, detachable devices.

>>> Not use what you're after with the SPI example though, but sounds
>>> like that's something different.
>>
>> I think Sebastien's example is just like the issue here.
> 
> Hmm is there some existing example in the kernel like that?

No, Sebastien's example was just a hypothetical case. Here, using your
way of having SoC specific data in the .dts, we would have
"sharp,ls037v7dw01-omap-dss", and in Sebastien's example with a touch
sensor we'd have, say, "synaptics,xyz123-omap-spi".

 Tomi
Tony Lindgren - May 19, 2014, 7:51 p.m.
* Tomi Valkeinen <tomi.valkeinen@ti.com> [140519 11:58]:
> On 19/05/14 18:57, Tony Lindgren wrote:
> 
> >> I'm not sure if it's even possible to load both of those drivers at the
> >> same time, but if it was, and the first one would get probe() called for
> >> a device, and the driver would return an error, I'm quite sure the
> >> driver framework won't continue trying with the other driver, as the
> >> first driver already matched for the device.
> > 
> > No need to load multiple drivers at this point. That's why I'm saying
> > you can bail out on the undesired display controller probes using
> > of_machine_is_compatible test. It's not that uncommon for drivers to do:
> 
> Hmm, I don't follow. If both, say, omap and exynos have a panel driver
> for the same sharp panel, both need a driver for it (presuming exynos
> has a proper display component driver system, which may not be true).
> 
> Both drivers would be loaded at boot time for the probe to be even to be
> possible. Even if the other one bails out from a device probe using
> of_machine_is_compatible, afaik the driver framework will stop probing
> at that point, as that driver reports being compatible with the device
> in question.
> 
> ...
> 
> Ah, ok, now I see. I think you mean module init, not probe? For some
> reason I was just thinking about the probe stage.
> 
> Maybe that would work. I need to test tomorrow.

Ah right probe is too late as the match has been made already by then :)
 
> >>> it would be quite silly for each display controller to have to do
> >>> their own remapping for shared panels?
> >>
> >> It would, but wouldn't it be just as silly (well, even more silly in my
> >> opinion) to have each display controller require driver specific
> >> compatible strings for the panels used?
> > 
> > Not driver specific, but hardware package specific. That's being done
> > all the time. For example, compatible strings like "nvidia,tegra124",
> > "ti,omap5" describe a package of an ARM core and various devices.
> 
> It feels to me rather wrong if I'd specify the compatible string for an
> external piece of hardware based on the SoC it's connected to.
> 
> > But by using of_machine_is_compatible in the display drivers, you
> > can use the right compatible flags from the start if you want to go
> > that way.
> 
> Yes, with that we could have right compatible flags in the .dts.
> 
> >> Well, I sure hope nobody else has to implement similar thing.
> > 
> > Suuuure.. I've heard that about 20 times before and guess what
> > happenened? Things never got fixed until some poor maintainer
> > had to start fixing it all over the place about three years later.
> 
> The poor maintainer here being me =). And it's still a case of
> pick-your-poison. We need to hack around the issue, in some form or
> another. So in any case the poor maintainer has to fix it at some point
> in the future.

Yeah I agree none of the options so far have been very nice.
 
> But all this is fixed when we have a common display framework. And
> that's something that is really badly needed in some form or another,
> the sooner the better.
> 
> So the "fix" here is not just some internal thing that could be left at
> that and forgotten.

Yes..
 
> >> To open that up a bit, traditionally other display driver architectures
> >> have not had drivers for each display "entity", like the panels. So you
> >> would really just have the display subsystem in the .dts, and some raw
> >> data there (panel timings) to get the panel running.
> > 
> > Right, I believe that is the way to go, no doubt about it.
> 
> No, that's exactly _not_ the way to go =). We need proper drivers for
> display components, instead of trying to avoid that and embed panel data
> into the display controller's data.

Right, sorry I misread it :)
 
> > Here's a list of things that bothers me:
> > 
> > 1. Searching through the dtb from a driver instead of relying on the
> >    driver probe mechanism for the components in question. If the parsing
> >    of the dtb needs to be done it should be done in a Linux generic way
> >    from some framework instead.
> 
> The reason I haven't made the conversion code available for others to
> use is that there are no other users at the moment. And I hope there
> will be no other user until we get the common display framework. If
> there is, we'll need to move the code to a generic place.
> 
> > 2. Having to do this all early before we even have any debug console
> >    available. We are trying to move everything to happen later on to avoid
> >    all the issues related to initializing things early.
> 
> It worked fine for me when doing it as subsys_init level. Why do you say
> it'd be early, before debug console?

The debug console should not be needed for debugging drivers.. Just
the serial console. This is because it's hard for users to produce
decent error information when the system does not boot if DEBUG_LL
and earlyprintk need to be enabled.
 
> All the drivers with converted compatible strings are normal drivers, so
> afaik things work fine if we just convert the strings before the module
> init level.

Yes that seems like the right place to do the workarounds.
 
> > 3. Having to maintain a database in kernel of display mappings separately
> >    in addition to the .dts files.
> 
> Yes, that's slightly annoying. Not really a big burden, though, as it's
> quite rare to get a new encoder or panel driver.
> 
> > 4. Having this hack limited to device tree based booting while we are
> >    trying to unify the functions for drivers to use to get their
> >    resources.
> 
> I don't understand this one. With non-DT boot we don't have the issue at
> all, there's no need for hacks.

Hmm can't we still have multiarch booting happening with the same
panel name being used by two different display drivers?
 
> > 5. Seeing the possibility of this spreading to other drivers.
> 
> Well, until we have a common display framework, one of the hacky options
> we've discussed will spread to other drivers if they need to have their
> own drivers for the same hardware devices.
> 
> Which is not nice, but not something we can escape. And using the
> of_machine_is_compatible or having the compatible strings in .dts
> appended with "dss" or such doesn't change that, it just changes which
> hack may spread.

Yes I agree they are all hacks, but your version seems to carry some
extra early init baggage with it as I noted above :)

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Reichel - May 20, 2014, 4:57 a.m.
Hi,

On Mon, May 19, 2014 at 09:57:18PM +0300, Tomi Valkeinen wrote:
> > 3. Having to maintain a database in kernel of display mappings separately
> >    in addition to the .dts files.
> 
> Yes, that's slightly annoying. Not really a big burden, though, as it's
> quite rare to get a new encoder or panel driver.

I wonder if the "omapdss," prefix could be added to *all* remote
endpoints seen by omapdss. Pseudocode:

for each port in dss {
    for each endpoint in port {
        panel = get_phandle(endpoint, "remote-endpoint");

        /* only add prefix if it's not already there (kexec) */
        if (panel.compatible != "omapdss,.*")
            panel.compatible = "omapdss," + panel.compatible;
    }
}

-- Sebastian
Sebastian Reichel - May 20, 2014, 5:12 a.m.
On Mon, May 19, 2014 at 10:05:15PM +0300, Tomi Valkeinen wrote:
> On 19/05/14 19:04, Tony Lindgren wrote:
> 
> > In many cases however we do have multiple compatible strings that
> > describe how the device is wired. See drivers/tty/serial/of_serial.c
> > for example. It has "ns16550" but then it also has additional
> > "nvidia,tegra20-uart", "nxp,lpc3220-uart" and "ibm,qpace-nwp-serial".
> 
> All those sound like SoC components. In that case it sounds fine to have
> the device compatible contain the SoC name. We're talking here about
> external, detachable devices.
>
> >>> Not use what you're after with the SPI example though, but sounds
> >>> like that's something different.
> >>
> >> I think Sebastien's example is just like the issue here.
> > 
> > Hmm is there some existing example in the kernel like that?
> 
> No, Sebastien's example was just a hypothetical case. Here, using your
> way of having SoC specific data in the .dts, we would have
> "sharp,ls037v7dw01-omap-dss", and in Sebastien's example with a touch
> sensor we'd have, say, "synaptics,xyz123-omap-spi".

Yes, that's what I wanted to say :)

My name is spelled Sebastian, though. Sebastien is the French
variant as far as i know.

-- Sebastian
Tomi Valkeinen - May 20, 2014, 5:21 a.m.
On 20/05/14 07:57, Sebastian Reichel wrote:
> Hi,
> 
> On Mon, May 19, 2014 at 09:57:18PM +0300, Tomi Valkeinen wrote:
>>> 3. Having to maintain a database in kernel of display mappings separately
>>>    in addition to the .dts files.
>>
>> Yes, that's slightly annoying. Not really a big burden, though, as it's
>> quite rare to get a new encoder or panel driver.
> 
> I wonder if the "omapdss," prefix could be added to *all* remote
> endpoints seen by omapdss. Pseudocode:
> 
> for each port in dss {
>     for each endpoint in port {
>         panel = get_phandle(endpoint, "remote-endpoint");
> 
>         /* only add prefix if it's not already there (kexec) */
>         if (panel.compatible != "omapdss,.*")
>             panel.compatible = "omapdss," + panel.compatible;
>     }
> }

Hmm, yes, I think that would work. It would remove the need for the
database, but the code would be slightly more complex as we'd need to
travel through the graph, not just the nodes that are directly connected
to omapdss. I need to try this approach also.

 Tomi

Ps. Sorry for misspelling your name, twice. If I recall right, I even
checked if your name is written with an 'a' or an 'e', but somehow still
managed to get it wrong.
Tony Lindgren - May 20, 2014, 5:48 a.m.
* Sebastian Reichel <sre@kernel.org> [140519 22:13]:
> On Mon, May 19, 2014 at 10:05:15PM +0300, Tomi Valkeinen wrote:
> > On 19/05/14 19:04, Tony Lindgren wrote:
> > 
> > > In many cases however we do have multiple compatible strings that
> > > describe how the device is wired. See drivers/tty/serial/of_serial.c
> > > for example. It has "ns16550" but then it also has additional
> > > "nvidia,tegra20-uart", "nxp,lpc3220-uart" and "ibm,qpace-nwp-serial".
> > 
> > All those sound like SoC components. In that case it sounds fine to have
> > the device compatible contain the SoC name. We're talking here about
> > external, detachable devices.

Yes I agree it's also hackish. But playing with the compatible flags
avoids adding other custom workarounds.

> > >>> Not use what you're after with the SPI example though, but sounds
> > >>> like that's something different.
> > >>
> > >> I think Sebastien's example is just like the issue here.
> > > 
> > > Hmm is there some existing example in the kernel like that?
> > 
> > No, Sebastien's example was just a hypothetical case. Here, using your
> > way of having SoC specific data in the .dts, we would have
> > "sharp,ls037v7dw01-omap-dss", and in Sebastien's example with a touch
> > sensor we'd have, say, "synaptics,xyz123-omap-spi".
> 
> Yes, that's what I wanted to say :)

Hmm I think we need to add few things to the hypothetical SPI case
to get it even close to the panel example. Like assume that no SPI bus
exists, and that potentially multiple different Synaptics drivers are
trying to share the same compatible flag for a non-existing generic
binding.. I think then it's getting close to the panel example :)

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Reichel - May 20, 2014, 9:10 p.m.
On Mon, May 19, 2014 at 10:48:52PM -0700, Tony Lindgren wrote:
> * Sebastian Reichel <sre@kernel.org> [140519 22:13]:
> > On Mon, May 19, 2014 at 10:05:15PM +0300, Tomi Valkeinen wrote:
> > > On 19/05/14 19:04, Tony Lindgren wrote:
> > > 
> > > > In many cases however we do have multiple compatible strings that
> > > > describe how the device is wired. See drivers/tty/serial/of_serial.c
> > > > for example. It has "ns16550" but then it also has additional
> > > > "nvidia,tegra20-uart", "nxp,lpc3220-uart" and "ibm,qpace-nwp-serial".
> > > 
> > > All those sound like SoC components. In that case it sounds fine to have
> > > the device compatible contain the SoC name. We're talking here about
> > > external, detachable devices.
> 
> Yes I agree it's also hackish. But playing with the compatible flags
> avoids adding other custom workarounds.
> 
> > > >>> Not use what you're after with the SPI example though, but sounds
> > > >>> like that's something different.
> > > >>
> > > >> I think Sebastien's example is just like the issue here.
> > > > 
> > > > Hmm is there some existing example in the kernel like that?
> > > 
> > > No, Sebastien's example was just a hypothetical case. Here, using your
> > > way of having SoC specific data in the .dts, we would have
> > > "sharp,ls037v7dw01-omap-dss", and in Sebastien's example with a touch
> > > sensor we'd have, say, "synaptics,xyz123-omap-spi".
> > 
> > Yes, that's what I wanted to say :)
> 
> Hmm I think we need to add few things to the hypothetical SPI case
> to get it even close to the panel example. Like assume that no SPI bus
> exists, [...]

The important thing is not, that there is an SPI bus (hardware), but
that there is a common SPI client framework (software). That's one
of the problem's I see with the modified compatible value.

Basically I see the following problem with the appended -omapdss in
the compatible string:

 * You can read the -omapdss as "to be used with omapdss driver". In
   this case the compatible string is just needed for driver loading,
   which is frowned upon DT binding maintainers AFAIK.
 * Alternatively one can read it as "connected to omapdss". In this
   case we add unneeded redundancy to the DT. This does not belong
   into a compatible string, since its encoded in the DT hierarchy.

And btw. Tomi's rewrite hack basically works exactly like this, just
without adding the redundancy explicitly into the DT. Instead of
prefixing the comaptible string he could also add a -omapdss sufix.
That's basically a proof, that the suffix adds useless redudancy.

> [...] and that potentially multiple different Synaptics drivers are
> trying to share the same compatible flag for a non-existing generic
> binding.

If there was no common SPI client framework you would need two
different SPI client drivers for two different SPI host controllers.

Obviously its a bad idea to have no SPI framework, but that's not
important for the example (except that there should obviously be a
common panel framework) :)

> I think then it's getting close to the panel example :)

-- Sebastian
Tomi Valkeinen - May 21, 2014, 1:05 p.m.
On 19/05/14 22:51, Tony Lindgren wrote:

>>> 4. Having this hack limited to device tree based booting while we are
>>>    trying to unify the functions for drivers to use to get their
>>>    resources.
>>
>> I don't understand this one. With non-DT boot we don't have the issue at
>> all, there's no need for hacks.
> 
> Hmm can't we still have multiarch booting happening with the same
> panel name being used by two different display drivers?

Yes, but in that case the driver names are internal to kernel. You can
append "omapdss" or such to the driver name, and have that name used in
the board file and in the driver. It's not visible outside the kernel,
and when there's a common display framework, it can be changed without
anyone knowing.

With DT we have the old, stable .dts files that need to be supported...

>>> 5. Seeing the possibility of this spreading to other drivers.
>>
>> Well, until we have a common display framework, one of the hacky options
>> we've discussed will spread to other drivers if they need to have their
>> own drivers for the same hardware devices.
>>
>> Which is not nice, but not something we can escape. And using the
>> of_machine_is_compatible or having the compatible strings in .dts
>> appended with "dss" or such doesn't change that, it just changes which
>> hack may spread.
> 
> Yes I agree they are all hacks, but your version seems to carry some
> extra early init baggage with it as I noted above :)

True, but it doesn't feel very big baggage to me. We can bail out
immediately if the machine is not omap, so for non-omap's the effect
should be negligible.

For omap there is some extra stuff being done. I don't really know heavy
it is, performance wise, but the operation itself is quite small.

I'll try and see how the other options work, which are:

- Bailing out from module_init in each display driver. The reason I
don't like this (although I haven't tried it) is that all the display
drivers need the modification, and because I need to catch the
module_init, I cannot use the helpers like module_platform_driver(), so
it adds multiple lines to every driver.

- Traveling the video graph, starting from omapdss. This one is possibly
better performance-wise than my original version, as we only need to
search for the omapdss node and can then follow the links. But the code
will be more complex.

 Tomi
Sebastian Reichel - May 21, 2014, 2:24 p.m.
Hi,

On Wed, May 21, 2014 at 04:05:56PM +0300, Tomi Valkeinen wrote:
> I'll try and see how the other options work, which are:
> 
> - Bailing out from module_init in each display driver. The reason I
> don't like this (although I haven't tried it) is that all the display
> drivers need the modification, and because I need to catch the
> module_init, I cannot use the helpers like module_platform_driver(), so
> it adds multiple lines to every driver.

You could create your own omapdss_driver() define for that.
This can be replaced more simply once no longer needed and
creates less bloat.

I see one more disadvantage for this approach:

assumed situation (some future 3.16+x kernel):
 * The kernel has an common display framework (CDF)
 * The kernel has an panel driver using CDF
 * There also exists omapdss driver for the same panel
 * omapdss does not (yet) adopt CDF
 * CDF and omapdss are enabled in .config
 * Kernel is booted from an OMAP system

This would result in both drivers being loaded using the same
DT compatible string. The same scenario works using the rewriting
method, since the common display framework would only see the
rewritten compatible string.

Of course this scenario only happens when omapdss is not adopted
to the common display framework from the start on.

> - Traveling the video graph, starting from omapdss. This one is possibly
> better performance-wise than my original version, as we only need to
> search for the omapdss node and can then follow the links. But the code
> will be more complex.

I think the main advantage is the missing lookup table/whitelist,
which adds redundancy. Not having it has some advantages:

* new drivers don't need to touch any existing files (making
  rebasing etc. easier)
* table can get out of sync (though that shouldn't happen anyway,
  since the binding is supposed to be stable)

-- Sebastian

Patch

--- /dev/null
+++ b/Documentation/devicetree/bindings/panel/sharp,ls037v7dw01.txt
@@ -0,0 +1,56 @@ 
+SHARP LS037V7DW01 TFT-LCD panel
+
+Required properties:
+- compatible: should be "sharp,ls037v7dw01"
+
+Optional properties:
+- enable-gpios: a GPIO spec for the optional enable pin
+  this pin is the INI pin as specified in the LS037V7DW01.pdf file.
+- reset-gpios: a GPIO spec for the optional reset pin
+  this pin is the RESB pin as specified in the LS037V7DW01.pdf file.
+- mode-gpios: a GPIO 
+  ordered MO, LR, and UD as specified in the LS037V7DW01.pdf file.
+
+This binding is compatible with the simple-panel binding, which is specified
+in simple-panel.txt in this directory.
+
+This panel can have zero to five GPIOs to configure
+to change configuration between QVGA and VGA mode
+and the scan direction. As these pins can be also
+configured with external pulls, all the GPIOs are
+considered optional with holes in the array.
+
+Example when connected to a omap2+ based device:
+
+	lcd0: display {
+		compatible = "sharp,ls037v7dw01";
+		power-supply = <&lcd_3v3>;
+		enable-gpios = <&gpio5 24 GPIO_ACTIVE_HIGH>;	/* gpio152, lcd INI */
+		reset-gpios = <&gpio5 27 GPIO_ACTIVE_HIGH>;	/* gpio155, lcd RESB */
+		mode-gpios = <&gpio5 26 GPIO_ACTIVE_HIGH	/* gpio154, lcd MO */
+			      &gpio1 2 GPIO_ACTIVE_HIGH		/* gpio2, lcd LR */
+			      &gpio1 3 GPIO_ACTIVE_HIGH>;	/* gpio3, lcd UD */
+
+		panel-timing {
+			clock-frequency = <19200000>;
+			hback-porch = <28>;
+			hactive = <480>;
+			hfront-porch = <1>;
+			hsync-len = <2>;
+			vback-porch = <1>;
+			vactive = <640>;
+			vfront-porch = <1>;
+			vsync-len = <1>;
+			hsync-active = <0>;
+			vsync-active = <0>;
+			de-active = <1>;
+			pixelclk-active = <1>;
+		};
+
+		port {
+			lcd_in: endpoint {
+				remote-endpoint = <&dpi_out>;
+			};
+		};
+	};
+
--- a/drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c
+++ b/drivers/video/fbdev/omap2/displays-new/panel-sharp-ls037v7dw01.c
@@ -12,15 +12,19 @@ 
 #include <linux/delay.h>
 #include <linux/gpio.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
-
+#include <linux/regulator/consumer.h>
+#include <video/of_display_timing.h>
 #include <video/omapdss.h>
 #include <video/omap-panel-data.h>
 
 struct panel_drv_data {
 	struct omap_dss_device dssdev;
 	struct omap_dss_device *in;
+	struct regulator *vcc;
 
 	int data_lines;
 
@@ -95,7 +99,8 @@  static int sharp_ls_enable(struct omap_dss_device *dssdev)
 	if (omapdss_device_is_enabled(dssdev))
 		return 0;
 
-	in->ops.dpi->set_data_lines(in, ddata->data_lines);
+	if (ddata->data_lines)
+		in->ops.dpi->set_data_lines(in, ddata->data_lines);
 	in->ops.dpi->set_timings(in, &ddata->videomode);
 
 	r = in->ops.dpi->enable(in);
@@ -240,6 +245,88 @@  static int sharp_ls_probe_pdata(struct platform_device *pdev)
 	if (PTR_ERR(ddata->ini_gpio) == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
 
+	ddata->videomode = sharp_ls_timings;
+
+	return 0;
+}
+
+static struct gpio_desc *
+sharp_ls_get_gpio_of(struct device *dev, int index, int val, char *desc)
+{
+	struct gpio_desc *gpio;
+
+	gpio = devm_gpiod_get_index(dev, desc, index);
+	if (IS_ERR(gpio))
+		return gpio;
+
+	gpiod_direction_output(gpio, val);
+
+	return gpio;
+}
+
+static int sharp_ls_probe_of(struct platform_device *pdev)
+{
+	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
+	struct device_node *node = pdev->dev.of_node;
+	struct omap_dss_device *in;
+	struct display_timing timing;
+	struct videomode vm;
+	int r;
+
+	ddata->vcc = devm_regulator_get(&pdev->dev, "envdd");
+	if (IS_ERR(ddata->vcc)) {
+		dev_err(&pdev->dev, "failed to get regulator\n");
+		return PTR_ERR(ddata->vcc);
+	}
+
+	r = regulator_enable(ddata->vcc);
+	if (r != 0) {
+		dev_err(&pdev->dev, "failed to enable regulator\n");
+		return r;
+	}
+
+	/* lcd INI */
+	ddata->ini_gpio = sharp_ls_get_gpio_of(&pdev->dev, 3, 0, "enable");
+	if (PTR_ERR(ddata->ini_gpio) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	/* lcd RESB */
+	ddata->resb_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 0, "reset");
+	if (PTR_ERR(ddata->resb_gpio) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	/* lcd MO */
+	ddata->mo_gpio = sharp_ls_get_gpio_of(&pdev->dev, 0, 0, "mode");
+	if (PTR_ERR(ddata->mo_gpio) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	/* lcd LR */
+	ddata->lr_gpio = sharp_ls_get_gpio_of(&pdev->dev, 1, 1, "mode");
+	if (PTR_ERR(ddata->lr_gpio) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	/* lcd UD */
+	ddata->ud_gpio = sharp_ls_get_gpio_of(&pdev->dev, 2, 1, "mode");
+	if (PTR_ERR(ddata->ud_gpio) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
+	r = of_get_display_timing(node, "panel-timing", &timing);
+	if (r) {
+		dev_err(&pdev->dev, "failed to get video timing\n");
+		return r;
+	}
+
+	videomode_from_timing(&timing, &vm);
+	videomode_to_omap_video_timings(&vm, &ddata->videomode);
+
+	in = omapdss_of_find_source_for_first_ep(node);
+	if (IS_ERR(in)) {
+		dev_err(&pdev->dev, "failed to find video source\n");
+		return PTR_ERR(in);
+	}
+
+	ddata->in = in;
+
 	return 0;
 }
 
@@ -259,12 +346,14 @@  static int sharp_ls_probe(struct platform_device *pdev)
 		r = sharp_ls_probe_pdata(pdev);
 		if (r)
 			return r;
+	} else if (pdev->dev.of_node) {
+		r = sharp_ls_probe_of(pdev);
+		if (r)
+			return r;
 	} else {
 		return -ENODEV;
 	}
 
-	ddata->videomode = sharp_ls_timings;
-
 	dssdev = &ddata->dssdev;
 	dssdev->dev = &pdev->dev;
 	dssdev->driver = &sharp_ls_ops;
@@ -302,12 +391,20 @@  static int __exit sharp_ls_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id sharp_ls_of_match[] = {
+	{ .compatible = "sharp,ls037v7dw01", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, sharp_ls_of_match);
+
 static struct platform_driver sharp_ls_driver = {
 	.probe = sharp_ls_probe,
 	.remove = __exit_p(sharp_ls_remove),
 	.driver = {
 		.name = "panel-sharp-ls037v7dw01",
 		.owner = THIS_MODULE,
+		.of_match_table = sharp_ls_of_match,
 	},
 };