[PATCH v2 1/2] media: soc_camera: pxa_camera documentation device-tree support

[PATCH v2 1/2] media: soc_camera: pxa_camera documentation device-tree support

[Robert Jarzmik]

Add device-tree bindings documentation for pxa_camera driver.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 .../devicetree/bindings/media/pxa-camera.txt       | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/pxa-camera.txt

diff --git a/Documentation/devicetree/bindings/media/pxa-camera.txt b/Documentation/devicetree/bindings/media/pxa-camera.txt
new file mode 100644
index 0000000..9835aae
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/pxa-camera.txt
@@ -0,0 +1,39 @@
+Marvell PXA camera host interface
+
+Required properties:
+ - compatible: Should be "marvell,pxa27x-qci"
+ - reg: register base and size
+ - interrupts: the interrupt number
+ - any required generic properties defined in video-interfaces.txt
+
+Optional properties:
+ - clock-frequency: host interface is driving MCLK, and MCLK rate is this rate
+
+Example:
+
+	pxa_camera: pxa_camera@50000000 {
+		compatible = "marvell,pxa27x-qci";
+		reg = <0x50000000 0x1000>;
+		interrupts = <33>;
+
+		clocks = <&pxa2xx_clks 24>;
+		clock-names = "camera";
+		status = "okay";
+
+		clock-frequency = <50000000>;
+
+		port {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			/* Parallel bus endpoint */
+			qci: endpoint@0 {
+				reg = <0>;		/* Local endpoint # */
+				remote-endpoint = <&mt9m111_1>;
+				bus-width = <8>;	/* Used data lines */
+				hsync-active = <0>;	/* Active low */
+				vsync-active = <0>;	/* Active low */
+				pclk-sample = <1>;	/* Rising */
+			};
+		};
+	};
-- 
2.0.0.rc2

[PATCH v2 2/2] media: soc_camera: pxa_camera device-tree support

[Robert Jarzmik]

Add device-tree support to pxa_camera host driver.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/media/platform/soc_camera/pxa_camera.c | 77 +++++++++++++++++++++++++-
 1 file changed, 75 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c
index d4df305..8c9de9e 100644
--- a/drivers/media/platform/soc_camera/pxa_camera.c
+++ b/drivers/media/platform/soc_camera/pxa_camera.c
@@ -34,6 +34,7 @@
 #include <media/videobuf-dma-sg.h>
 #include <media/soc_camera.h>
 #include <media/soc_mediabus.h>
+#include <media/v4l2-of.h>
 
 #include <linux/videodev2.h>
 
@@ -1650,6 +1651,64 @@ static struct soc_camera_host_ops pxa_soc_camera_host_ops = {
 	.set_bus_param	= pxa_camera_set_bus_param,
 };
 
+static int pxa_camera_pdata_from_dt(struct device *dev,
+				    struct pxa_camera_dev *pcdev)
+{
+	int err = 0;
+	struct device_node *np = dev->of_node;
+	struct v4l2_of_endpoint ep;
+
+	err = of_property_read_u32(np, "clock-frequency",
+				   (u32 *)&pcdev->mclk);
+	if (!err)
+		pcdev->platform_flags |= PXA_CAMERA_MCLK_EN;
+
+	np = of_graph_get_next_endpoint(np, NULL);
+	if (!np) {
+		dev_err(dev, "could not find endpoint\n");
+		return -EINVAL;
+	}
+
+	err = v4l2_of_parse_endpoint(np, &ep);
+	if (err) {
+		dev_err(dev, "could not parse endpoint\n");
+		return err;
+	}
+
+	switch (ep.bus.parallel.bus_width) {
+	case 4:
+		pcdev->platform_flags |= PXA_CAMERA_DATAWIDTH_4;
+		break;
+	case 5:
+		pcdev->platform_flags |= PXA_CAMERA_DATAWIDTH_5;
+		break;
+	case 8:
+		pcdev->platform_flags |= PXA_CAMERA_DATAWIDTH_8;
+		break;
+	case 9:
+		pcdev->platform_flags |= PXA_CAMERA_DATAWIDTH_9;
+		break;
+	case 10:
+		pcdev->platform_flags |= PXA_CAMERA_DATAWIDTH_10;
+		break;
+	default:
+		break;
+	};
+
+	if (ep.bus.parallel.flags & V4L2_MBUS_MASTER)
+		pcdev->platform_flags |= PXA_CAMERA_MASTER;
+	if (ep.bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
+		pcdev->platform_flags |= PXA_CAMERA_HSP;
+	if (ep.bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
+		pcdev->platform_flags |= PXA_CAMERA_VSP;
+	if (ep.bus.parallel.flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
+		pcdev->platform_flags |= PXA_CAMERA_PCLK_EN | PXA_CAMERA_PCP;
+	if (ep.bus.parallel.flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
+		pcdev->platform_flags |= PXA_CAMERA_PCLK_EN;
+
+	return 0;
+}
+
 static int pxa_camera_probe(struct platform_device *pdev)
 {
 	struct pxa_camera_dev *pcdev;
@@ -1676,7 +1735,15 @@ static int pxa_camera_probe(struct platform_device *pdev)
 	pcdev->res = res;
 
 	pcdev->pdata = pdev->dev.platform_data;
-	pcdev->platform_flags = pcdev->pdata->flags;
+	if (&pdev->dev.of_node && !pcdev->pdata) {
+		err = pxa_camera_pdata_from_dt(&pdev->dev, pcdev);
+	} else {
+		pcdev->platform_flags = pcdev->pdata->flags;
+		pcdev->mclk = pcdev->pdata->mclk_10khz * 10000;
+	}
+	if (err < 0)
+		return err;
+
 	if (!(pcdev->platform_flags & (PXA_CAMERA_DATAWIDTH_8 |
 			PXA_CAMERA_DATAWIDTH_9 | PXA_CAMERA_DATAWIDTH_10))) {
 		/*
@@ -1693,7 +1760,6 @@ static int pxa_camera_probe(struct platform_device *pdev)
 		pcdev->width_flags |= 1 << 8;
 	if (pcdev->platform_flags & PXA_CAMERA_DATAWIDTH_10)
 		pcdev->width_flags |= 1 << 9;
-	pcdev->mclk = pcdev->pdata->mclk_10khz * 10000;
 	if (!pcdev->mclk) {
 		dev_warn(&pdev->dev,
 			 "mclk == 0! Please, fix your platform data. "
@@ -1799,10 +1865,17 @@ static const struct dev_pm_ops pxa_camera_pm = {
 	.resume		= pxa_camera_resume,
 };
 
+static const struct of_device_id pxa_camera_of_match[] = {
+	{ .compatible = "marvell,pxa27x-qci", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, pxa_camera_of_match);
+
 static struct platform_driver pxa_camera_driver = {
 	.driver		= {
 		.name	= PXA_CAM_DRV_NAME,
 		.pm	= &pxa_camera_pm,
+		.of_match_table = of_match_ptr(pxa_camera_of_match),
 	},
 	.probe		= pxa_camera_probe,
 	.remove		= pxa_camera_remove,
-- 
2.0.0.rc2

Re: [PATCH v2 2/2] media: soc_camera: pxa_camera device-tree support

[Mark Rutland]

On Sat, Jun 21, 2014 at 11:21:47PM +0100, Robert Jarzmik wrote:
> Add device-tree support to pxa_camera host driver.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  drivers/media/platform/soc_camera/pxa_camera.c | 77 +++++++++++++++++++++++++-
>  1 file changed, 75 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c
> index d4df305..8c9de9e 100644
> --- a/drivers/media/platform/soc_camera/pxa_camera.c
> +++ b/drivers/media/platform/soc_camera/pxa_camera.c
> @@ -34,6 +34,7 @@
>  #include <media/videobuf-dma-sg.h>
>  #include <media/soc_camera.h>
>  #include <media/soc_mediabus.h>
> +#include <media/v4l2-of.h>
>  
>  #include <linux/videodev2.h>
>  
> @@ -1650,6 +1651,64 @@ static struct soc_camera_host_ops pxa_soc_camera_host_ops = {
>  	.set_bus_param	= pxa_camera_set_bus_param,
>  };
>  
> +static int pxa_camera_pdata_from_dt(struct device *dev,
> +				    struct pxa_camera_dev *pcdev)
> +{
> +	int err = 0;
> +	struct device_node *np = dev->of_node;
> +	struct v4l2_of_endpoint ep;
> +
> +	err = of_property_read_u32(np, "clock-frequency",
> +				   (u32 *)&pcdev->mclk);

That cast is either unnecessary or this code is broken.

Use a temporary u32 if the types don't match.

Mark.

Re: [PATCH v2 1/2] media: soc_camera: pxa_camera documentation device-tree support

[Mark Rutland]

On Sat, Jun 21, 2014 at 11:21:46PM +0100, Robert Jarzmik wrote:
> Add device-tree bindings documentation for pxa_camera driver.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  .../devicetree/bindings/media/pxa-camera.txt       | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/pxa-camera.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/pxa-camera.txt b/Documentation/devicetree/bindings/media/pxa-camera.txt
> new file mode 100644
> index 0000000..9835aae
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/pxa-camera.txt
> @@ -0,0 +1,39 @@
> +Marvell PXA camera host interface
> +
> +Required properties:
> + - compatible: Should be "marvell,pxa27x-qci"

Is that x a wildcard? Or is 'x' part of the name of a particular unit?

We prefer not to have wildcard compatible strings in DT.

> + - reg: register base and size
> + - interrupts: the interrupt number
> + - any required generic properties defined in video-interfaces.txt
> +
> +Optional properties:
> + - clock-frequency: host interface is driving MCLK, and MCLK rate is this rate

Is MCLK an input or an output of this block?

If the former, why isn't this described as a clock?

> +
> +Example:
> +
> +	pxa_camera: pxa_camera@50000000 {
> +		compatible = "marvell,pxa27x-qci";
> +		reg = <0x50000000 0x1000>;
> +		interrupts = <33>;
> +
> +		clocks = <&pxa2xx_clks 24>;
> +		clock-names = "camera";

These weren't mentioned above. Is the clock input line really called
"camera"?

Mark.

Re: [PATCH v2 2/2] media: soc_camera: pxa_camera device-tree support

[Robert Jarzmik]

Mark Rutland <mark.rutland@arm.com> writes:

> On Sat, Jun 21, 2014 at 11:21:47PM +0100, Robert Jarzmik wrote:
>> @@ -1650,6 +1651,64 @@ static struct soc_camera_host_ops pxa_soc_camera_host_ops = {
>>  	.set_bus_param	= pxa_camera_set_bus_param,
>>  };
>>  
>> +static int pxa_camera_pdata_from_dt(struct device *dev,
>> +				    struct pxa_camera_dev *pcdev)
>> +{
>> +	int err = 0;
>> +	struct device_node *np = dev->of_node;
>> +	struct v4l2_of_endpoint ep;
>> +
>> +	err = of_property_read_u32(np, "clock-frequency",
>> +				   (u32 *)&pcdev->mclk);
>
> That cast is either unnecessary or this code is broken.
Mmm maybe ...
As a clock rate is an unsigned long by design, where is the
of_property_read_ulong() function ?

> Use a temporary u32 if the types don't match.
If there's no of_*() function available, let's do that.

-- 
Robert

Re: [PATCH v2 2/2] media: soc_camera: pxa_camera device-tree support

[Robert Jarzmik]

Mark Rutland <mark.rutland@arm.com> writes:

> On Sat, Jun 21, 2014 at 11:21:47PM +0100, Robert Jarzmik wrote:
>> @@ -1650,6 +1651,64 @@ static struct soc_camera_host_ops pxa_soc_camera_host_ops = {
>>  	.set_bus_param	= pxa_camera_set_bus_param,
>>  };
>>  
>> +static int pxa_camera_pdata_from_dt(struct device *dev,
>> +				    struct pxa_camera_dev *pcdev)
>> +{
>> +	int err = 0;
>> +	struct device_node *np = dev->of_node;
>> +	struct v4l2_of_endpoint ep;
>> +
>> +	err = of_property_read_u32(np, "clock-frequency",
>> +				   (u32 *)&pcdev->mclk);
>
> That cast is either unnecessary or this code is broken.
Mmm maybe ...
As a clock rate is an unsigned long by design, where is the
of_property_read_ulong() function ?

> Use a temporary u32 if the types don't match.
If there's no of_*() function available, let's do that.

-- 
Robert

Re: [PATCH v2 1/2] media: soc_camera: pxa_camera documentation device-tree support

[Robert Jarzmik]

Mark Rutland <mark.rutland@arm.com> writes:

> On Sat, Jun 21, 2014 at 11:21:46PM +0100, Robert Jarzmik wrote:
>> +Required properties:
>> + - compatible: Should be "marvell,pxa27x-qci"
>
> Is that x a wildcard? Or is 'x' part of the name of a particular unit?
It's kind of a wildcard for a group of platforms
It stands for the 3 PXA27x SoCs I'm aware of : PXA270, PXA271, and PXA272. The
difference between them is different core frequency range and embedded RAM.

> We prefer not to have wildcard compatible strings in DT.
OK, then let's go for "marvell,pxa270-qci".


>
>> + - reg: register base and size
>> + - interrupts: the interrupt number
>> + - any required generic properties defined in video-interfaces.txt
>> +
>> +Optional properties:
>> + - clock-frequency: host interface is driving MCLK, and MCLK rate is this rate
>
> Is MCLK an input or an output of this block?
An output clock.

> If the former, why isn't this described as a clock?
It's a good point. I'll try to add that too. The little trouble I have is that
the PXA clocks are not _yet_ in device-tree. Putting a clock description will
make this patch dependant on the clock framework patches [1], right ?

>> 
>> +Example:
>> +
>> +	pxa_camera: pxa_camera@50000000 {
>> +		compatible = "marvell,pxa27x-qci";
>> +		reg = <0x50000000 0x1000>;
>> +		interrupts = <33>;
>> +
>> +		clocks = <&pxa2xx_clks 24>;
>> +		clock-names = "camera";
>
> These weren't mentioned above. Is the clock input line really called
> "camera"?
This is another clock, an input clock, independant of the former one. This is
the clock actually fed to make this IP block work. This is dependant on the
clock framework patches [1].

Cheers.

-- 
Robert

[1] http://www.spinics.net/lists/arm-kernel/msg337521.html

Re: [PATCH v2 1/2] media: soc_camera: pxa_camera documentation device-tree support

[Robert Jarzmik]

Mark Rutland <mark.rutland@arm.com> writes:

> On Sat, Jun 21, 2014 at 11:21:46PM +0100, Robert Jarzmik wrote:
>> +Required properties:
>> + - compatible: Should be "marvell,pxa27x-qci"
>
> Is that x a wildcard? Or is 'x' part of the name of a particular unit?
It's kind of a wildcard for a group of platforms
It stands for the 3 PXA27x SoCs I'm aware of : PXA270, PXA271, and PXA272. The
difference between them is different core frequency range and embedded RAM.

> We prefer not to have wildcard compatible strings in DT.
OK, then let's go for "marvell,pxa270-qci".


>
>> + - reg: register base and size
>> + - interrupts: the interrupt number
>> + - any required generic properties defined in video-interfaces.txt
>> +
>> +Optional properties:
>> + - clock-frequency: host interface is driving MCLK, and MCLK rate is this rate
>
> Is MCLK an input or an output of this block?
An output clock.

> If the former, why isn't this described as a clock?
It's a good point. I'll try to add that too. The little trouble I have is that
the PXA clocks are not _yet_ in device-tree. Putting a clock description will
make this patch dependant on the clock framework patches [1], right ?

>> 
>> +Example:
>> +
>> +	pxa_camera: pxa_camera@50000000 {
>> +		compatible = "marvell,pxa27x-qci";
>> +		reg = <0x50000000 0x1000>;
>> +		interrupts = <33>;
>> +
>> +		clocks = <&pxa2xx_clks 24>;
>> +		clock-names = "camera";
>
> These weren't mentioned above. Is the clock input line really called
> "camera"?
This is another clock, an input clock, independant of the former one. This is
the clock actually fed to make this IP block work. This is dependant on the
clock framework patches [1].

Cheers.

-- 
Robert

[1] http://www.spinics.net/lists/arm-kernel/msg337521.html

Re: [PATCH v2 1/2] media: soc_camera: pxa_camera documentation device-tree support

[Mark Rutland]

On Wed, Jun 25, 2014 at 08:44:31PM +0100, Robert Jarzmik wrote:
> Mark Rutland <mark.rutland@arm.com> writes:
> 
> > On Sat, Jun 21, 2014 at 11:21:46PM +0100, Robert Jarzmik wrote:
> >> +Required properties:
> >> + - compatible: Should be "marvell,pxa27x-qci"
> >
> > Is that x a wildcard? Or is 'x' part of the name of a particular unit?
> It's kind of a wildcard for a group of platforms
> It stands for the 3 PXA27x SoCs I'm aware of : PXA270, PXA271, and PXA272. The
> difference between them is different core frequency range and embedded RAM.
> 
> > We prefer not to have wildcard compatible strings in DT.
> OK, then let's go for "marvell,pxa270-qci".

That sounds fine to me.

> >
> >> + - reg: register base and size
> >> + - interrupts: the interrupt number
> >> + - any required generic properties defined in video-interfaces.txt
> >> +
> >> +Optional properties:
> >> + - clock-frequency: host interface is driving MCLK, and MCLK rate is this rate
> >
> > Is MCLK an input or an output of this block?
> An output clock.
> 
> > If the former, why isn't this described as a clock?
> It's a good point. I'll try to add that too. The little trouble I have is that
> the PXA clocks are not _yet_ in device-tree. Putting a clock description will
> make this patch dependant on the clock framework patches [1], right ?

Yes, this will.

> 
> >> 
> >> +Example:
> >> +
> >> +	pxa_camera: pxa_camera@50000000 {
> >> +		compatible = "marvell,pxa27x-qci";
> >> +		reg = <0x50000000 0x1000>;
> >> +		interrupts = <33>;
> >> +
> >> +		clocks = <&pxa2xx_clks 24>;
> >> +		clock-names = "camera";
> >
> > These weren't mentioned above. Is the clock input line really called
> > "camera"?
> This is another clock, an input clock, independant of the former one. This is
> the clock actually fed to make this IP block work. This is dependant on the
> clock framework patches [1].

Ok. This clock should be mentioned in the properties description above.

Thanks,
Mark.