diff mbox

[v2,4/4] video: mmp: add device tree support

Message ID 1389698184-28761-5-git-send-email-zzhu3@marvell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zhou Zhu Jan. 14, 2014, 11:16 a.m. UTC
add device tree support for mmp fb/controller
the description of DT config is at
Documentation/devicetree/bindings/fb/mmp-disp.txt

Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
---
 Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
 drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
 drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
 3 files changed, 235 insertions(+), 58 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt

Comments

Tomi Valkeinen Feb. 10, 2014, 12:43 p.m. UTC | #1
On 14/01/14 13:16, Zhou Zhu wrote:
> add device tree support for mmp fb/controller
> the description of DT config is at
> Documentation/devicetree/bindings/fb/mmp-disp.txt
> 
> Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
> ---
>  Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
>  drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
>  drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
>  3 files changed, 235 insertions(+), 58 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt
> 
> diff --git a/Documentation/devicetree/bindings/fb/mmp-disp.txt b/Documentation/devicetree/bindings/fb/mmp-disp.txt
> new file mode 100644
> index 0000000..80702f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fb/mmp-disp.txt
> @@ -0,0 +1,60 @@
> +* Marvell MMP Display (MMP_DISP)
> +
> +To config mmp display, 3 parts are required to be set in dts:
> +1. mmp fb
> +Required properties:
> +- compatible: Should be "marvell,<soc>-fb".
> +- marvell,path: Should be the path this fb connecting to.
> +- marvell,overlay-id: Should be the id of overlay this fb is on.
> +- marvell,dmafetch-id: Should be the dma fetch id this fb using.
> +- marvell,default-pixfmt: Should be the default pixel format when this fb is
> +turned on.
> +
> +2. mmp controller
> +Required properties:
> +- compatible: Should be "marvell,<soc>-disp".
> +- reg: Should be address and length of the register set for this controller.
> +- interrupts: Should be interrupt of this controller.
> +
> +Required sub-node:
> +- path:
> +Required properties in this sub-node:
> +-- marvell,overlay_num: Should be number of overlay this path has.

If that one tells how many overlays there are, maybe "num_overlays"
would be better.

> +-- marvell,output-type: Should be output-type settings
> +-- marvell,path-config: Should be path-config settings
> +-- marvell,link-config: Should be link-config settings
> +-- marvell,rbswap: Should be rbswap settings

If these terms (output-type, path-config, ...) are straight from the HW
manual, then fine. But if they are not clear, or are driver specific,
the values these can have should be documented here.

> +
> +3. panel
> +Required properties:
> +- marvell,path: Should be path that this panel connected to.
> +- other properties each panel has.
> +
> +Examples:
> +
> +fb: mmp-fb {
> +	compatible = "marvell,pxa988-fb";
> +	marvell,path = <&path1>;
> +	marvell,overlay-id = <0>;
> +	marvell,dmafetch-id = <1>;
> +	marvell,default-pixfmt = <0x108>;
> +};
> +
> +disp: mmp-disp@d420b000 {
> +	compatible = "marvell,pxa988-disp";
> +	reg = <0xd420b000 0x1fc>;
> +	interrupts = <0 41 0x4>;
> +	path1: mmp-pnpath {
> +		marvell,overlay-num = <2>;
> +		marvell,output-type = <0>;
> +		marvell,path-config = <0x20000000>;
> +		marvell,link-config = <0x60000001>;
> +		marvell,rbswap = <0>;
> +	};
> +};
> +
> +panel: <panel-name> {

How is the panel linked to all this? The nodes above do not refer to the
panel.

> +	...
> +	marvell,path = <&path1>;
> +	...
> +};

It's a bit difficult to say much about this, as I have no knowledge
about mmp.

But I don't quite understand what the pxa988-fb is. Is that some kind of
virtual device, only used to set up fbdev side? And pxa988-disp is the
actual hardware device?

If so, I don't really think pxa988-fb should exist in the DT data at
all, as it's not a hardware device.

Why isn't there just one node for pxa988-disp, which would contain all
the above information?

 Tomi
Zhou Zhu Feb. 11, 2014, 3:27 a.m. UTC | #2
On 02/10/2014 08:43 PM, Tomi Valkeinen wrote:
> On 14/01/14 13:16, Zhou Zhu wrote:
>> add device tree support for mmp fb/controller
>> the description of DT config is at
>> Documentation/devicetree/bindings/fb/mmp-disp.txt
>>
>> Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
>> ---
>>   Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
>>   drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
>>   drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
>>   3 files changed, 235 insertions(+), 58 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt
>>
>> diff --git a/Documentation/devicetree/bindings/fb/mmp-disp.txt b/Documentation/devicetree/bindings/fb/mmp-disp.txt
>> new file mode 100644
>> index 0000000..80702f5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/fb/mmp-disp.txt
>> @@ -0,0 +1,60 @@
>> +* Marvell MMP Display (MMP_DISP)
>> +
>> +To config mmp display, 3 parts are required to be set in dts:
>> +1. mmp fb
>> +Required properties:
>> +- compatible: Should be "marvell,<soc>-fb".
>> +- marvell,path: Should be the path this fb connecting to.
>> +- marvell,overlay-id: Should be the id of overlay this fb is on.
>> +- marvell,dmafetch-id: Should be the dma fetch id this fb using.
>> +- marvell,default-pixfmt: Should be the default pixel format when this fb is
>> +turned on.
>> +
>> +2. mmp controller
>> +Required properties:
>> +- compatible: Should be "marvell,<soc>-disp".
>> +- reg: Should be address and length of the register set for this controller.
>> +- interrupts: Should be interrupt of this controller.
>> +
>> +Required sub-node:
>> +- path:
>> +Required properties in this sub-node:
>> +-- marvell,overlay_num: Should be number of overlay this path has.
>
> If that one tells how many overlays there are, maybe "num_overlays"
> would be better.
I will update it in next version.
>
>> +-- marvell,output-type: Should be output-type settings
>> +-- marvell,path-config: Should be path-config settings
>> +-- marvell,link-config: Should be link-config settings
>> +-- marvell,rbswap: Should be rbswap settings
>
> If these terms (output-type, path-config, ...) are straight from the HW
> manual, then fine. But if they are not clear, or are driver specific,
> the values these can have should be documented here.
Yes, it's straight from HW manual.
>
>> +
>> +3. panel
>> +Required properties:
>> +- marvell,path: Should be path that this panel connected to.
>> +- other properties each panel has.
>> +
>> +Examples:
>> +
>> +fb: mmp-fb {
>> +	compatible = "marvell,pxa988-fb";
>> +	marvell,path = <&path1>;
>> +	marvell,overlay-id = <0>;
>> +	marvell,dmafetch-id = <1>;
>> +	marvell,default-pixfmt = <0x108>;
>> +};
>> +
>> +disp: mmp-disp@d420b000 {
>> +	compatible = "marvell,pxa988-disp";
>> +	reg = <0xd420b000 0x1fc>;
>> +	interrupts = <0 41 0x4>;
>> +	path1: mmp-pnpath {
>> +		marvell,overlay-num = <2>;
>> +		marvell,output-type = <0>;
>> +		marvell,path-config = <0x20000000>;
>> +		marvell,link-config = <0x60000001>;
>> +		marvell,rbswap = <0>;
>> +	};
>> +};
>> +
>> +panel: <panel-name> {
>
> How is the panel linked to all this? The nodes above do not refer to the
> panel.
We are making panel refer to path, so when panel dev probe, it could 
register to related path.
The reason we not link from path to panel is our customer sometimes 
asked us to use same image pack (include dts) for different panel types 
in product. We could only add all these panels in dts and detect panel 
dynamically when boot. So moving panel out and making path not link to 
panel but panel link to path would be more straight forward.
>
>> +	...
>> +	marvell,path = <&path1>;
>> +	...
>> +};
>
> It's a bit difficult to say much about this, as I have no knowledge
> about mmp.
>
> But I don't quite understand what the pxa988-fb is. Is that some kind of
> virtual device, only used to set up fbdev side? And pxa988-disp is the
> actual hardware device?
>
> If so, I don't really think pxa988-fb should exist in the DT data at
> all, as it's not a hardware device.
Yes, fb is a virtual device for fbdev side.
In our platforms we may use more than one fb for different paths/output 
panel or multi overlays on same path for composition. So we need to 
configure fb settings like which path/overlay/dmafetch it connected to 
for one or more fbdev.
Besides, some more configures like yres_virtual size or fixed fb mem 
reserved rather than allocated which depends on platforms are also 
needed although these features are not upstreamed yet.
So we put fb as a dt node here for these configures.
>
> Why isn't there just one node for pxa988-disp, which would contain all
> the above information?
We have moved out fb/panel from path due to several reasons:
1. To simplify the node. If moved these nodes in, it might be:
disp {
	...
	path1 {
		...
		panel-xxx {
		}
		panel-yyy {
		}
		...
		fb0 {
		}
		fb1 {
		}
	}
	path2 {
		...
		panel-zzz {
		}
		fb3 {
		}
	}
}
Also due to child node type might be different, we could even not 
directly check child of node. The code would be complex.
2. the path of node would be too long and not so common.
e.g. the panel node in dts would be /soc/axi/disp/path1/panel-xxx, and 
in sysfs, node would be /sys/devices/platform/soc/axi/path1/panel-xxx. 
It would be complex and not compatible for platforms when our 
bootloader/user app doing some operations to these nodes.
If we moved them out, we could move fb/panel out of soc directory so the 
node would be /panel-xxx or /fb1 and simpler and compatible.
>
>   Tomi
>
>
Tomi Valkeinen Feb. 12, 2014, 10:11 a.m. UTC | #3
On 11/02/14 05:27, Zhou Zhu wrote:
> On 02/10/2014 08:43 PM, Tomi Valkeinen wrote:

>> How is the panel linked to all this? The nodes above do not refer to the
>> panel.
> We are making panel refer to path, so when panel dev probe, it could
> register to related path.
> The reason we not link from path to panel is our customer sometimes
> asked us to use same image pack (include dts) for different panel types
> in product. We could only add all these panels in dts and detect panel
> dynamically when boot. So moving panel out and making path not link to
> panel but panel link to path would be more straight forward.

Ok.

>>
>>> +    ...
>>> +    marvell,path = <&path1>;
>>> +    ...
>>> +};
>>
>> It's a bit difficult to say much about this, as I have no knowledge
>> about mmp.
>>
>> But I don't quite understand what the pxa988-fb is. Is that some kind of
>> virtual device, only used to set up fbdev side? And pxa988-disp is the
>> actual hardware device?
>>
>> If so, I don't really think pxa988-fb should exist in the DT data at
>> all, as it's not a hardware device.
> Yes, fb is a virtual device for fbdev side.
> In our platforms we may use more than one fb for different paths/output
> panel or multi overlays on same path for composition. So we need to
> configure fb settings like which path/overlay/dmafetch it connected to
> for one or more fbdev.
> Besides, some more configures like yres_virtual size or fixed fb mem
> reserved rather than allocated which depends on platforms are also
> needed although these features are not upstreamed yet.
> So we put fb as a dt node here for these configures.

Ok.

The device tree data should reflect the hardware. Not the software. So
when thinking what kind of DT data you should have, you should forget
the drivers, and just think on HW terms.

You might want to have a look at CDF (Common Display Framework)
discussions on linux-fbdev list, and the "[PATCHv3 00/41] OMAPDSS: DT
support v3" series I've posted (mostly the .dts parts).

It sounds to me that you'd benefit greatly from the CDF, and even if CDF
is not yet merged (and will probably still take time), I'd very much
recommend trying to create your DT data in such a manner that it would
be in the same direction as what is currently suggested for CDF (or in
the OMAPDSS series).

>> Why isn't there just one node for pxa988-disp, which would contain all
>> the above information?
> We have moved out fb/panel from path due to several reasons:
> 1. To simplify the node. If moved these nodes in, it might be:
> disp {
>     ...
>     path1 {
>         ...
>         panel-xxx {
>         }
>         panel-yyy {
>         }
>         ...
>         fb0 {
>         }
>         fb1 {
>         }
>     }
>     path2 {
>         ...
>         panel-zzz {
>         }
>         fb3 {
>         }
>     }
> }
> Also due to child node type might be different, we could even not
> directly check child of node. The code would be complex.
> 2. the path of node would be too long and not so common.
> e.g. the panel node in dts would be /soc/axi/disp/path1/panel-xxx, and
> in sysfs, node would be /sys/devices/platform/soc/axi/path1/panel-xxx.
> It would be complex and not compatible for platforms when our
> bootloader/user app doing some operations to these nodes.
> If we moved them out, we could move fb/panel out of soc directory so the
> node would be /panel-xxx or /fb1 and simpler and compatible.

I suggest to first think only about the DT data, and create it
correctly. After that you could think how to create compatibility code
in the driver side, so that the legacy sysfs paths are still usable.

The thing with DT data is that it's quite difficult to make big changes
to it later, without writing possibly complex compatibility
functionality. So in my opinion it's worth it to spend a good deal of
time thinking about good DT bindings from the start.

That said, if the driver and hardware in question is for some old SoC,
that's not going to be used on new boards, and the driver is not going
to be used in any future boards, it might be simpler to just make simple
bindings that work for the known set of boards and displays, and be done
with it.

I don't know if that's the case here or not.

 Tomi
Mark Rutland Feb. 17, 2014, 2:37 p.m. UTC | #4
On Tue, Jan 14, 2014 at 11:16:24AM +0000, Zhou Zhu wrote:
> add device tree support for mmp fb/controller
> the description of DT config is at
> Documentation/devicetree/bindings/fb/mmp-disp.txt
> 
> Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
> ---
>  Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
>  drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
>  drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
>  3 files changed, 235 insertions(+), 58 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt
> 
> diff --git a/Documentation/devicetree/bindings/fb/mmp-disp.txt b/Documentation/devicetree/bindings/fb/mmp-disp.txt
> new file mode 100644
> index 0000000..80702f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fb/mmp-disp.txt
> @@ -0,0 +1,60 @@
> +* Marvell MMP Display (MMP_DISP)
> +
> +To config mmp display, 3 parts are required to be set in dts:
> +1. mmp fb
> +Required properties:
> +- compatible: Should be "marvell,<soc>-fb".

Please list the precise values and when they should be used. It makes
searching for them _far_ easier.

> +- marvell,path: Should be the path this fb connecting to.

What type is this? The example implies a phandle.

What type of node does this point to?

It's not explained at this point and it's really unclear what this is.

> +- marvell,overlay-id: Should be the id of overlay this fb is on.
> +- marvell,dmafetch-id: Should be the dma fetch id this fb using.

Are these hardware properties?

Is there any documentation which could make this clearer?

> +- marvell,default-pixfmt: Should be the default pixel format when this fb is
> +turned on.

What format is this? The example is useless. If this is a standard
binding, please refer to the binding document.

> +
> +2. mmp controller
> +Required properties:
> +- compatible: Should be "marvell,<soc>-disp".

Please list the precise set of values.

> +- reg: Should be address and length of the register set for this controller.
> +- interrupts: Should be interrupt of this controller.

Just to check: the device only has on interrupt?

> +
> +Required sub-node:
> +- path:
> +Required properties in this sub-node:
> +-- marvell,overlay_num: Should be number of overlay this path has.

s/_/-/ in property names please.

num-overlays would be a clearer name.

> +-- marvell,output-type: Should be output-type settings
> +-- marvell,path-config: Should be path-config settings
> +-- marvell,link-config: Should be link-config settings
> +-- marvell,rbswap: Should be rbswap settings

These are completely opaque to me. Please describe what these are either
in place or in reference to standard bindings.

Is rbswap a boolean value?

[...]

> +       if (!path_np || of_property_read_u32(path_np, "marvell,overlay-num",
> +                               &path_info->output_type) ||
> +                       of_property_read_u32(path_np, "marvell,output-type",
> +                               &path_info->overlay_num)) {

These are reading into the wrong variables.

Cheers,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhou Zhu March 4, 2014, 11:28 a.m. UTC | #5
Hi, Tomi and Mark,

On Mon, Feb 17, 2014 at 10:37 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Tue, Jan 14, 2014 at 11:16:24AM +0000, Zhou Zhu wrote:
> > add device tree support for mmp fb/controller
> > the description of DT config is at
> > Documentation/devicetree/bindings/fb/mmp-disp.txt
> >
> > Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
> > ---
> >  Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
> >  drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
> >  drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
> >  3 files changed, 235 insertions(+), 58 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt


Thank you very much for your review! I am trying to update the code
according to your comments.

We have reviewed the dts and removed many software settings and
not-used settings - for example, we unpacked path-config/link-config
and removed some configures that we will never change.
Also we removed fb settings which is considered as software.

Would you please give us some feedbacks if we adjust our dts into follow style?
As there might be big changes on the code structures, I would update
the code after this dts layout is considered as "right".

mmp-disp@d420b000 {
    compatible = "marvell,mmp-disp";
    reg = <0xd420b000 0x1fc>;
    interrupts = <0 41 0x4>;

    internal-connections {
        pipe1: pn-path {
        input = "panel-graphic"; //panel-graphic is the overlay name in spec.
        output = &parallel;
        }
    }
    ports{
        parallel: parallel {
            marvell,rbswap;
            marvell,spi;
        }
    }
    status = "okay";
}

panel-xxx {
    properties;
    connection = &parallel;
}
Tomi Valkeinen March 5, 2014, 9:33 a.m. UTC | #6
On 04/03/14 13:28, Zhou Zhu wrote:
> Hi, Tomi and Mark,
> 
> On Mon, Feb 17, 2014 at 10:37 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>
>> On Tue, Jan 14, 2014 at 11:16:24AM +0000, Zhou Zhu wrote:
>>> add device tree support for mmp fb/controller
>>> the description of DT config is at
>>> Documentation/devicetree/bindings/fb/mmp-disp.txt
>>>
>>> Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
>>> ---
>>>  Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
>>>  drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
>>>  drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
>>>  3 files changed, 235 insertions(+), 58 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt
> 
> 
> Thank you very much for your review! I am trying to update the code
> according to your comments.
> 
> We have reviewed the dts and removed many software settings and
> not-used settings - for example, we unpacked path-config/link-config
> and removed some configures that we will never change.
> Also we removed fb settings which is considered as software.
> 
> Would you please give us some feedbacks if we adjust our dts into follow style?
> As there might be big changes on the code structures, I would update
> the code after this dts layout is considered as "right".
> 
> mmp-disp@d420b000 {
>     compatible = "marvell,mmp-disp";
>     reg = <0xd420b000 0x1fc>;
>     interrupts = <0 41 0x4>;
> 
>     internal-connections {
>         pipe1: pn-path {
>         input = "panel-graphic"; //panel-graphic is the overlay name in spec.
>         output = &parallel;
>         }
>     }
>     ports{
>         parallel: parallel {
>             marvell,rbswap;
>             marvell,spi;
>         }
>     }
>     status = "okay";
> }
> 
> panel-xxx {
>     properties;
>     connection = &parallel;
> }

I would recommend using the same style that is used in the OMAP DSS, imx
drm and exynos patches that are introducing DT support. They use ports
and endpoints as defined in:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/media/video-interfaces.txt

with the addition of a simplified endpoint format, where the 'port' node
is not required.

To give an idea what it could look like, I've modified your example
above. It's just a rough example, you should study and think how it best
fits for mmp.

You could use the same format for the internal connections also, but I
didn't touch that. I think internal connections can as well be
configured separately, in a custom format, because there's no need for
external components to connect to the internal links.

mmp-disp@d420b000 {
    compatible = "marvell,mmp-disp";
    reg = <0xd420b000 0x1fc>;
    interrupts = <0 41 0x4>;

    internal-connections {
        pipe1: pn-path {
        input = "panel-graphic";
        output = &disp_out;
        }
    }

	disp_out: endpoint {
		remote-endpoint = <&panel_in>;
		marvell,rbswap;
		marvell,spi;
	};

    status = "okay";
}

panel-xxx {
	properties;

	panel_in: endpoint {
		remote-endpoint = <&disp_out>;
	};
};

 Tomi
Zhou Zhu March 14, 2014, 10:43 a.m. UTC | #7
Hi, Tomi,

On 03/05/2014 05:33 PM, Tomi Valkeinen wrote:
> I would recommend using the same style that is used in the OMAP DSS, imx
> drm and exynos patches that are introducing DT support. They use ports
> and endpoints as defined in:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/media/video-interfaces.txt
>
> with the addition of a simplified endpoint format, where the 'port' node
> is not required.
>
> To give an idea what it could look like, I've modified your example
> above. It's just a rough example, you should study and think how it best
> fits for mmp.
>
> You could use the same format for the internal connections also, but I
> didn't touch that. I think internal connections can as well be
> configured separately, in a custom format, because there's no need for
> external components to connect to the internal links.
>
> mmp-disp@d420b000 {
>      compatible = "marvell,mmp-disp";
>      reg = <0xd420b000 0x1fc>;
>      interrupts = <0 41 0x4>;
>
>      internal-connections {
>          pipe1: pn-path {
>          input = "panel-graphic";
>          output = &disp_out;
>          }
>      }
>
> 	disp_out: endpoint {
> 		remote-endpoint = <&panel_in>;
> 		marvell,rbswap;
> 		marvell,spi;
> 	};
>
>      status = "okay";
> }
>
> panel-xxx {
> 	properties;
>
> 	panel_in: endpoint {
> 		remote-endpoint = <&disp_out>;
> 	};
> };
>
>   Tomi
>
>
Thank you! I will update patches according to this layout later.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/fb/mmp-disp.txt b/Documentation/devicetree/bindings/fb/mmp-disp.txt
new file mode 100644
index 0000000..80702f5
--- /dev/null
+++ b/Documentation/devicetree/bindings/fb/mmp-disp.txt
@@ -0,0 +1,60 @@ 
+* Marvell MMP Display (MMP_DISP)
+
+To config mmp display, 3 parts are required to be set in dts:
+1. mmp fb
+Required properties:
+- compatible: Should be "marvell,<soc>-fb".
+- marvell,path: Should be the path this fb connecting to.
+- marvell,overlay-id: Should be the id of overlay this fb is on.
+- marvell,dmafetch-id: Should be the dma fetch id this fb using.
+- marvell,default-pixfmt: Should be the default pixel format when this fb is
+turned on.
+
+2. mmp controller
+Required properties:
+- compatible: Should be "marvell,<soc>-disp".
+- reg: Should be address and length of the register set for this controller.
+- interrupts: Should be interrupt of this controller.
+
+Required sub-node:
+- path:
+Required properties in this sub-node:
+-- marvell,overlay_num: Should be number of overlay this path has.
+-- marvell,output-type: Should be output-type settings
+-- marvell,path-config: Should be path-config settings
+-- marvell,link-config: Should be link-config settings
+-- marvell,rbswap: Should be rbswap settings
+
+3. panel
+Required properties:
+- marvell,path: Should be path that this panel connected to.
+- other properties each panel has.
+
+Examples:
+
+fb: mmp-fb {
+	compatible = "marvell,pxa988-fb";
+	marvell,path = <&path1>;
+	marvell,overlay-id = <0>;
+	marvell,dmafetch-id = <1>;
+	marvell,default-pixfmt = <0x108>;
+};
+
+disp: mmp-disp@d420b000 {
+	compatible = "marvell,pxa988-disp";
+	reg = <0xd420b000 0x1fc>;
+	interrupts = <0 41 0x4>;
+	path1: mmp-pnpath {
+		marvell,overlay-num = <2>;
+		marvell,output-type = <0>;
+		marvell,path-config = <0x20000000>;
+		marvell,link-config = <0x60000001>;
+		marvell,rbswap = <0>;
+	};
+};
+
+panel: <panel-name> {
+	...
+	marvell,path = <&path1>;
+	...
+};
diff --git a/drivers/video/mmp/fb/mmpfb.c b/drivers/video/mmp/fb/mmpfb.c
index 7ab31eb..f919d8e 100644
--- a/drivers/video/mmp/fb/mmpfb.c
+++ b/drivers/video/mmp/fb/mmpfb.c
@@ -22,6 +22,7 @@ 
 #include <linux/module.h>
 #include <linux/dma-mapping.h>
 #include <linux/platform_device.h>
+#include <linux/of.h>
 #include "mmpfb.h"
 
 static int var_to_pixfmt(struct fb_var_screeninfo *var)
@@ -551,56 +552,79 @@  static void fb_info_clear(struct fb_info *info)
 	fb_dealloc_cmap(&info->cmap);
 }
 
+static const struct of_device_id mmp_fb_dt_match[] = {
+	{ .compatible = "marvell,mmp-fb" },
+	{ .compatible = "marvell,pxa910-fb" },
+	{ .compatible = "marvell,pxa988-fb" },
+	{},
+};
+
 static int mmpfb_probe(struct platform_device *pdev)
 {
 	struct mmp_buffer_driver_mach_info *mi;
+	struct device_node *np;
 	struct fb_info *info = 0;
 	struct mmpfb_info *fbi = 0;
-	int ret, modes_num;
-
-	mi = pdev->dev.platform_data;
-	if (mi == NULL) {
-		dev_err(&pdev->dev, "no platform data defined\n");
-		return -EINVAL;
-	}
+	int ret = -EINVAL, modes_num;
+	int overlay_id = 0, dmafetch_id = 0;
 
 	/* initialize fb */
 	info = framebuffer_alloc(sizeof(struct mmpfb_info), &pdev->dev);
 	if (info == NULL)
 		return -ENOMEM;
 	fbi = info->par;
-	if (!fbi) {
-		ret = -EINVAL;
+	if (!fbi)
 		goto failed;
+
+	np = pdev->dev.of_node;
+	if (np) {
+		fbi->path = devm_mmp_get_path_by_phandle(&pdev->dev,
+					"marvell,path");
+		if (!fbi->path || of_property_read_u32(np,
+				"marvell,default-pixfmt", &fbi->pix_fmt)) {
+			dev_err(&pdev->dev, "unable to get fb setting from dt\n");
+			goto failed;
+		}
+		/* default setting if not set */
+		of_property_read_u32(np, "marvell,overlay-id", &overlay_id);
+		of_property_read_u32(np, "marvell,dmafetch-id", &dmafetch_id);
+		fbi->name = np->name;
+	} else {
+		mi = pdev->dev.platform_data;
+		if (mi == NULL) {
+			dev_err(&pdev->dev, "no platform data defined\n");
+			goto failed;
+		}
+
+		fbi->path = mmp_get_path(mi->path_name);
+		if (!fbi->path) {
+			dev_err(&pdev->dev, "can't get the path %s\n",
+					mi->path_name);
+			goto failed;
+		}
+
+		fbi->name = mi->name;
+		overlay_id = mi->overlay_id;
+		dmafetch_id = mi->dmafetch_id;
+		fbi->pix_fmt = mi->default_pixfmt;
 	}
 
 	/* init fb */
 	fbi->fb_info = info;
 	platform_set_drvdata(pdev, fbi);
 	fbi->dev = &pdev->dev;
-	fbi->name = mi->name;
-	fbi->pix_fmt = mi->default_pixfmt;
 	pixfmt_to_var(&info->var, fbi->pix_fmt);
 	mutex_init(&fbi->access_ok);
 
-	/* get display path by name */
-	fbi->path = mmp_get_path(mi->path_name);
-	if (!fbi->path) {
-		dev_err(&pdev->dev, "can't get the path %s\n", mi->path_name);
-		ret = -EINVAL;
-		goto failed_destroy_mutex;
-	}
-
 	dev_info(fbi->dev, "path %s get\n", fbi->path->name);
 
 	/* get overlay */
-	fbi->overlay = mmp_path_get_overlay(fbi->path, mi->overlay_id);
-	if (!fbi->overlay) {
-		ret = -EINVAL;
+	fbi->overlay = mmp_path_get_overlay(fbi->path, overlay_id);
+	if (!fbi->overlay)
 		goto failed_destroy_mutex;
-	}
+
 	/* set fetch used */
-	mmp_overlay_set_fetch(fbi->overlay, mi->dmafetch_id);
+	mmp_overlay_set_fetch(fbi->overlay, dmafetch_id);
 
 	modes_num = modes_setup(fbi);
 	if (modes_num < 0) {
@@ -679,6 +703,7 @@  static struct platform_driver mmpfb_driver = {
 	.driver		= {
 		.name	= "mmp-fb",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(mmp_fb_dt_match),
 	},
 	.probe		= mmpfb_probe,
 };
diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c
index b65913e..08b2ee7 100644
--- a/drivers/video/mmp/hw/mmp_ctrl.c
+++ b/drivers/video/mmp/hw/mmp_ctrl.c
@@ -37,6 +37,7 @@ 
 #include <linux/uaccess.h>
 #include <linux/kthread.h>
 #include <linux/io.h>
+#include <linux/of.h>
 
 #include "mmp_ctrl.h"
 
@@ -396,26 +397,43 @@  static void path_set_default(struct mmp_path *path)
 	writel_relaxed(tmp, ctrl_regs(path) + dma_ctrl(0, path->id));
 }
 
-static int path_init(struct mmphw_path_plat *path_plat,
-		struct mmp_mach_path_config *config)
+static int of_path_init(struct mmphw_path_plat *path_plat,
+		struct device_node *path_np)
 {
 	struct mmphw_ctrl *ctrl = path_plat->ctrl;
 	struct mmp_path_info *path_info;
 	struct mmp_path *path = NULL;
 
-	dev_info(ctrl->dev, "%s: %s\n", __func__, config->name);
-
 	/* init driver data */
 	path_info = kzalloc(sizeof(struct mmp_path_info), GFP_KERNEL);
 	if (!path_info) {
-		dev_err(ctrl->dev, "%s: unable to alloc path_info for %s\n",
-				__func__, config->name);
-		return 0;
+		dev_err(ctrl->dev, "%s: unable to alloc path_info\n",
+				__func__);
+		return -ENOMEM;
 	}
-	path_info->name = config->name;
+
+	if (!path_np || of_property_read_u32(path_np, "marvell,overlay-num",
+				&path_info->output_type) ||
+			of_property_read_u32(path_np, "marvell,output-type",
+				&path_info->overlay_num)) {
+		dev_err(ctrl->dev, "%s: unable to get path setting from dt\n",
+			__func__);
+		kfree(path_info);
+		return -EINVAL;
+	}
+	/* allow these settings not set */
+	of_property_read_u32(path_np, "marvell,path-config",
+		&path_plat->path_config);
+	of_property_read_u32(path_np, "marvell,link-config",
+		&path_plat->link_config);
+	of_property_read_u32(path_np, "marvell,rbswap",
+		&path_plat->dsi_rbswap);
+	path_info->name = path_np->name;
+
+	dev_info(ctrl->dev, "%s: %s\n", __func__, path_info->name);
+
 	path_info->id = path_plat->id;
 	path_info->dev = ctrl->dev;
-	path_info->overlay_num = config->overlay_num;
 	path_info->overlay_ops = &mmphw_overlay_ops;
 	path_info->set_mode = path_set_mode;
 	path_info->plat_data = path_plat;
@@ -424,16 +442,56 @@  static int path_init(struct mmphw_path_plat *path_plat,
 	path = mmp_register_path(path_info);
 	if (!path) {
 		kfree(path_info);
-		return 0;
+		return -EINVAL;
 	}
 	path_plat->path = path;
+	path_set_default(path);
+
+	kfree(path_info);
+	return 0;
+}
+
+static int path_init(struct mmphw_path_plat *path_plat,
+		struct mmp_mach_path_config *config)
+{
+	struct mmphw_ctrl *ctrl = path_plat->ctrl;
+	struct mmp_path_info *path_info;
+	struct mmp_path *path = NULL;
+
+	/* init driver data */
+	path_info = kzalloc(sizeof(struct mmp_path_info), GFP_KERNEL);
+	if (!path_info) {
+		dev_err(ctrl->dev, "%s: unable to alloc path_info\n",
+				__func__);
+		return -ENOMEM;
+	}
+
+	path_info->name = config->name;
+	path_info->overlay_num = config->overlay_num;
+	path_info->output_type = config->output_type;
 	path_plat->path_config = config->path_config;
 	path_plat->link_config = config->link_config;
 	path_plat->dsi_rbswap = config->dsi_rbswap;
+
+	dev_info(ctrl->dev, "%s: %s\n", __func__, path_info->name);
+
+	path_info->id = path_plat->id;
+	path_info->dev = ctrl->dev;
+	path_info->overlay_ops = &mmphw_overlay_ops;
+	path_info->set_mode = path_set_mode;
+	path_info->plat_data = path_plat;
+
+	/* create/register platform device */
+	path = mmp_register_path(path_info);
+	if (!path) {
+		kfree(path_info);
+		return -EINVAL;
+	}
+	path_plat->path = path;
 	path_set_default(path);
 
 	kfree(path_info);
-	return 1;
+	return 0;
 }
 
 static void path_deinit(struct mmphw_path_plat *path_plat)
@@ -445,13 +503,22 @@  static void path_deinit(struct mmphw_path_plat *path_plat)
 		mmp_unregister_path(path_plat->path);
 }
 
+static const struct of_device_id mmp_disp_dt_match[] = {
+	{ .compatible = "marvell,mmp-disp" },
+	{ .compatible = "marvell,pxa910-disp" },
+	{ .compatible = "marvell,pxa988-disp" },
+	{},
+};
+
 static int mmphw_probe(struct platform_device *pdev)
 {
-	struct mmp_mach_plat_info *mi;
 	struct resource *res;
-	int ret, i, size, irq;
+	int ret, i, size, irq, path_num;
+	const char *disp_name;
 	struct mmphw_path_plat *path_plat;
 	struct mmphw_ctrl *ctrl = NULL;
+	struct mmp_mach_plat_info *mi = pdev->dev.platform_data;
+	struct device_node *np, *path_np = NULL;
 
 	/* get resources from platform data */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -468,25 +535,38 @@  static int mmphw_probe(struct platform_device *pdev)
 		goto failed;
 	}
 
-	/* get configs from platform data */
-	mi = pdev->dev.platform_data;
-	if (mi == NULL || !mi->path_num || !mi->paths) {
-		dev_err(&pdev->dev, "%s: no platform data defined\n", __func__);
-		ret = -EINVAL;
-		goto failed;
+	np = pdev->dev.of_node;
+	if (np) {
+		path_num = of_get_child_count(np);
+		if (path_num <= 0) {
+			dev_err(&pdev->dev, "%s: failed to get settings from dt\n",
+				__func__);
+			ret = -EINVAL;
+			goto failed;
+		}
+		disp_name = np->name;
+	} else {
+		if (mi == NULL || !mi->path_num || !mi->paths) {
+			dev_err(&pdev->dev, "%s: no platform data defined\n",
+				__func__);
+			ret = -EINVAL;
+			goto failed;
+		}
+
+		disp_name = mi->name;
+		path_num = mi->path_num;
 	}
 
 	/* allocate */
 	size = sizeof(struct mmphw_ctrl) + sizeof(struct mmphw_path_plat) *
-	       mi->path_num;
+	       path_num;
 	ctrl = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
 	if (!ctrl) {
 		ret = -ENOMEM;
 		goto failed;
 	}
-
-	ctrl->name = mi->name;
-	ctrl->path_num = mi->path_num;
+	ctrl->path_num = path_num;
+	ctrl->name = disp_name;
 	ctrl->dev = &pdev->dev;
 	ctrl->irq = irq;
 	platform_set_drvdata(pdev, ctrl);
@@ -532,20 +612,31 @@  static int mmphw_probe(struct platform_device *pdev)
 	/* init global regs */
 	ctrl_set_default(ctrl);
 
-	/* init pathes from machine info and register them */
-	for (i = 0; i < ctrl->path_num; i++) {
-		/* get from config and machine info */
-		path_plat = &ctrl->path_plats[i];
-		path_plat->id = i;
-		path_plat->ctrl = ctrl;
-
-		/* path init */
-		if (!path_init(path_plat, &mi->paths[i])) {
-			ret = -EINVAL;
-			goto failed_path_init;
+	if (np) {
+		i = 0;
+		for_each_child_of_node(np, path_np) {
+			path_plat = &ctrl->path_plats[i];
+			path_plat->id = i;
+			path_plat->ctrl = ctrl;
+			i++;
+
+			ret = of_path_init(path_plat, path_np);
+			if (ret)
+				goto failed_path_init;
+		}
+	} else {
+		for (i = 0; i < ctrl->path_num; i++) {
+			path_plat = &ctrl->path_plats[i];
+			path_plat->id = i;
+			path_plat->ctrl = ctrl;
+
+			ret = path_init(path_plat, &mi->paths[i]);
+			if (ret)
+				goto failed_path_init;
 		}
 	}
 
+
 #ifdef CONFIG_MMP_DISP_SPI
 	ret = lcd_spi_register(ctrl);
 	if (ret < 0)
@@ -573,6 +664,7 @@  static struct platform_driver mmphw_driver = {
 	.driver		= {
 		.name	= "mmp-disp",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(mmp_disp_dt_match),
 	},
 	.probe		= mmphw_probe,
 };