[PATCH v2 18/24] video: da8xx-fb: minimal dt support

[PATCH v2 18/24] video: da8xx-fb: minimal dt support

[Darren Etheridge]

From: Afzal Mohammed <afzal@ti.com>

Driver is provided a means to have the probe triggered by DT.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
Signed-off-by: Darren Etheridge <detheridge@ti.com>
---
 .../devicetree/bindings/video/fb-da8xx.txt         |   16 ++++++++++++++++
 drivers/video/da8xx-fb.c                           |    7 +++++++
 2 files changed, 23 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/fb-da8xx.txt

diff --git a/Documentation/devicetree/bindings/video/fb-da8xx.txt b/Documentation/devicetree/bindings/video/fb-da8xx.txt
new file mode 100644
index 0000000..581e014
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/fb-da8xx.txt
@@ -0,0 +1,16 @@
+TI LCD Controller on DA830/DA850/AM335x SoC's
+
+Required properties:
+- compatible:
+	DA830 - "ti,da830-lcdc"
+	AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
+- reg: Address range of lcdc register set
+- interrupts: lcdc interrupt
+
+Example:
+
+lcdc@4830e000 {
+	compatible = "ti,am3352-lcdc", "ti,da830-lcdc";
+	reg =  <0x4830e000 0x1000>;
+	interrupts = <36>;
+};
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 983176a..97d6b2d 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1596,6 +1596,12 @@ static int fb_resume(struct platform_device *dev)
 #define fb_resume NULL
 #endif
 
+static const struct of_device_id da8xx_fb_of_match[] = {
+	{.compatible = "ti,da830-lcdc", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, da8xx_fb_of_match);
+
 static struct platform_driver da8xx_fb_driver = {
 	.probe = fb_probe,
 	.remove = fb_remove,
@@ -1604,6 +1610,7 @@ static struct platform_driver da8xx_fb_driver = {
 	.driver = {
 		   .name = DRIVER_NAME,
 		   .owner = THIS_MODULE,
+		   .of_match_table = of_match_ptr(da8xx_fb_of_match),
 		   },
 };
 
-- 
1.7.0.4

Re: [PATCH v2 18/24] video: da8xx-fb: minimal dt support

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]

On 30/07/13 21:26, Darren Etheridge wrote:
> From: Afzal Mohammed <afzal@ti.com>
> 
> Driver is provided a means to have the probe triggered by DT.
> 
> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> Signed-off-by: Darren Etheridge <detheridge@ti.com>
> ---
>  .../devicetree/bindings/video/fb-da8xx.txt         |   16 ++++++++++++++++
>  drivers/video/da8xx-fb.c                           |    7 +++++++
>  2 files changed, 23 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/video/fb-da8xx.txt
> 
> diff --git a/Documentation/devicetree/bindings/video/fb-da8xx.txt b/Documentation/devicetree/bindings/video/fb-da8xx.txt
> new file mode 100644
> index 0000000..581e014
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/fb-da8xx.txt
> @@ -0,0 +1,16 @@
> +TI LCD Controller on DA830/DA850/AM335x SoC's
> +
> +Required properties:
> +- compatible:
> +	DA830 - "ti,da830-lcdc"
> +	AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"

I'm not totally sure about this, but how I understand the compatible
property, the above reads as: "this device is ti,am3352-lcdc and it's
fully compatible with ti,da830-lcdc". I.e. if the kernel has a driver
for da830-lcdc, it should work with AM335x also (without any of the new
features in AM335x, obviously). Which I believe is not the case, as the
point of this series is to add the AM335x support.

Or should the current da830-lcdc work with AM335x also, but it just
didn't because there were bugs in da830-lcdc?

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH v2 18/24] video: da8xx-fb: minimal dt support

[Darren Etheridge]

Tomi Valkeinen <tomi.valkeinen@ti.com> wrote on Wed [2013-Jul-31 13:19:01 +0300]:
> > +Required properties:
> > +- compatible:
> > +	DA830 - "ti,da830-lcdc"
> > +	AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
> 
> I'm not totally sure about this, but how I understand the compatible
> property, the above reads as: "this device is ti,am3352-lcdc and it's
> fully compatible with ti,da830-lcdc". I.e. if the kernel has a driver
> for da830-lcdc, it should work with AM335x also (without any of the new
> features in AM335x, obviously). Which I believe is not the case, as the
> point of this series is to add the AM335x support.
> 
> Or should the current da830-lcdc work with AM335x also, but it just
> didn't because there were bugs in da830-lcdc?
> 
>  Tomi
> 
> 
OK I agree there is something wrong here, for one I don't think setting
ti,am3352-lcdc would do anything anyway given the driver only reports
.compatible with ti,da830-lcdc so at the very least the document is
wrong.  I will look into this and decide what is the best way of
resolving this.  I will go ahead and submit the series without the DT
support anyway and then I will look into this.

Darren

Re: [PATCH v2 18/24] video: da8xx-fb: minimal dt support

[Darren Etheridge]

Darren Etheridge <detheridge@ti.com> wrote on Thu [2013-Aug-01 08:36:02 -0500]:
> Tomi Valkeinen <tomi.valkeinen@ti.com> wrote on Wed [2013-Jul-31 13:19:01 +0300]:
> > > +Required properties:
> > > +- compatible:
> > > +	DA830 - "ti,da830-lcdc"
> > > +	AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
> > 
> > I'm not totally sure about this, but how I understand the compatible
> > property, the above reads as: "this device is ti,am3352-lcdc and it's
> > fully compatible with ti,da830-lcdc". I.e. if the kernel has a driver
> > for da830-lcdc, it should work with AM335x also (without any of the new
> > features in AM335x, obviously). Which I believe is not the case, as the
> > point of this series is to add the AM335x support.
> > 
> > Or should the current da830-lcdc work with AM335x also, but it just
> > didn't because there were bugs in da830-lcdc?
> > 
> >  Tomi
> > 
> > 
> OK I agree there is something wrong here, for one I don't think setting
> ti,am3352-lcdc would do anything anyway given the driver only reports
> .compatible with ti,da830-lcdc so at the very least the document is
> wrong.  I will look into this and decide what is the best way of
> resolving this.  I will go ahead and submit the series without the DT
> support anyway and then I will look into this.
> 
Reading the device tree documentation I think what is here in the
patch is actually correct so ignore my first response.

In answer to your original question in theory the da830 driver would
be fully compatible with the am335x - am335x implements a version 2 of
the lcd controller that adds some extra reg bits that support larger
horizontal and vertical resolutions and a sync loss interrupt.  
However the driver takes care of those differences internally.  I
think the main thing that was needed from this patch set to support
am335x were the dt changes, the Kconfig changes and the clock changes
- the rest of it are enhancements and bug fixes that are not strictly
needed for am335x support. 

Perhaps the correct thing to do and maybe what you are getting at is
really am3352 is the superset driver which we have now added complete
support for with these patches. However because we handle the
differences between the two IP's directly in the driver we should have
the driver provide both ti,am3352-lcdc and ti,da830-lcdc as
.compatible entries because really the driver supports both version 1 and
version 2 IP's.

Re: [PATCH v2 18/24] video: da8xx-fb: minimal dt support

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3553 bytes --]

On 01/08/13 23:21, Darren Etheridge wrote:
> Darren Etheridge <detheridge@ti.com> wrote on Thu [2013-Aug-01 08:36:02 -0500]:
>> Tomi Valkeinen <tomi.valkeinen@ti.com> wrote on Wed [2013-Jul-31 13:19:01 +0300]:
>>>> +Required properties:
>>>> +- compatible:
>>>> +	DA830 - "ti,da830-lcdc"
>>>> +	AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
>>>
>>> I'm not totally sure about this, but how I understand the compatible
>>> property, the above reads as: "this device is ti,am3352-lcdc and it's
>>> fully compatible with ti,da830-lcdc". I.e. if the kernel has a driver
>>> for da830-lcdc, it should work with AM335x also (without any of the new
>>> features in AM335x, obviously). Which I believe is not the case, as the
>>> point of this series is to add the AM335x support.
>>>
>>> Or should the current da830-lcdc work with AM335x also, but it just
>>> didn't because there were bugs in da830-lcdc?
>>>
>>>  Tomi
>>>
>>>
>> OK I agree there is something wrong here, for one I don't think setting
>> ti,am3352-lcdc would do anything anyway given the driver only reports
>> .compatible with ti,da830-lcdc so at the very least the document is
>> wrong.  I will look into this and decide what is the best way of
>> resolving this.  I will go ahead and submit the series without the DT
>> support anyway and then I will look into this.
>>
> Reading the device tree documentation I think what is here in the
> patch is actually correct so ignore my first response.
> 
> In answer to your original question in theory the da830 driver would
> be fully compatible with the am335x - am335x implements a version 2 of
> the lcd controller that adds some extra reg bits that support larger
> horizontal and vertical resolutions and a sync loss interrupt.  
> However the driver takes care of those differences internally.  I
> think the main thing that was needed from this patch set to support
> am335x were the dt changes, the Kconfig changes and the clock changes
> - the rest of it are enhancements and bug fixes that are not strictly
> needed for am335x support. 

Ok. And the clock changes are also valid in da8xx context also? The need
for them just didn't realize with our current use of da8xx?

> Perhaps the correct thing to do and maybe what you are getting at is
> really am3352 is the superset driver which we have now added complete
> support for with these patches. However because we handle the
> differences between the two IP's directly in the driver we should have
> the driver provide both ti,am3352-lcdc and ti,da830-lcdc as
> .compatible entries because really the driver supports both version 1 and
> version 2 IP's.

Hmm, right, I believe the driver should provide "ti,am3352-lcdc" also,
because it does support the extra features offered in am3352.

A bit side tracking, but: I've been wondering about the device names we
use. Naming the lcdc driver by the SoC is not quite correct. I don't
know all the IPs TI has, but what I see is that TI has two types of
display controllers: the more basic LCDC, and the bigger DSS.

Could we just call the device lcdc or dss? So, for example, DA830 has
"ti,lcdc1", AM3352 has "ti,lcdc2", OMAP2 has "ti,dss2", OMAP4 has
"ti,dss4", etc.

Then again, I'm not sure if the HW development of LCDC or DSS goes quite
that linearly. There are multiple SoCs that have OMAP3's DSS, but are
they the same DSS, or does each of them have a slightly different
version, without there clearly being a DSS version 3.1, 3.2, 3.3, etc...

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]