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

  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 | 80 ++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)
  

On Sun, 15 Jun 2014, 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 | 80 ++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
> 
> diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c
> index d4df305..e48d821 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,9 +1651,74 @@ static struct soc_camera_host_ops pxa_soc_camera_host_ops = {
>  	.set_bus_param	= pxa_camera_set_bus_param,
>  };
>  
> +static const struct of_device_id pxacamera_dt_ids[] = {
> +	{ .compatible = "mrvl,pxa_camera", },

as Documentation/devicetree/bindings/vendor-prefixes.txt defines, it 
should be "marvell."

> +	{ }
> +};
> +
> +static int pxa_camera_pdata_from_dt(struct device *dev,
> +				    struct pxacamera_platform_data *pdata)
> +{
> +	int err = 0;
> +	struct device_node *np = dev->of_node;
> +	struct v4l2_of_endpoint ep;
> +
> +	dev_info(dev, "RJK: %s()\n", __func__);

I have nothing against attributing work to respective authors, but I don't 
think this makes a lot of sense in the long run in the above form :) Once 
you've verified that your binding is working and this function is working, 
either remove this or make it more informative - maybe at the end of this 
function, also listing a couple of important parameters, that you obtained 
from DT.

> +	err = of_property_read_u32(np, "mclk_10khz",
> +				   (u32 *)&pdata->mclk_10khz);

I think we'll be frowned upon for this :) PXA270 doesn't support CCF, does 
it? Even if it doesn't we probably should use the standard 
"clock-frequency" property in any case. Actually, I missed to mention on 
this in my comments to your bindings documentation.

> +	if (!err)
> +		pdata->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:
> +		pdata->flags |= PXA_CAMERA_DATAWIDTH_4;
> +		break;
> +	case 5:
> +		pdata->flags |= PXA_CAMERA_DATAWIDTH_5;
> +		break;
> +	case 8:
> +		pdata->flags |= PXA_CAMERA_DATAWIDTH_8;
> +		break;
> +	case 9:
> +		pdata->flags |= PXA_CAMERA_DATAWIDTH_9;
> +		break;
> +	case 10:
> +		pdata->flags |= PXA_CAMERA_DATAWIDTH_10;
> +		break;
> +	default:
> +		break;
> +	};
> +
> +	if (ep.bus.parallel.flags & V4L2_MBUS_MASTER)
> +		pdata->flags |= PXA_CAMERA_MASTER;
> +	if (ep.bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> +		pdata->flags |= PXA_CAMERA_HSP;
> +	if (ep.bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> +		pdata->flags |= PXA_CAMERA_VSP;
> +	if (ep.bus.parallel.flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> +		pdata->flags |= PXA_CAMERA_PCLK_EN | PXA_CAMERA_PCP;
> +	if (ep.bus.parallel.flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> +		pdata->flags |= PXA_CAMERA_PCLK_EN;
> +
> +	return 0;
> +}
> +
>  static int pxa_camera_probe(struct platform_device *pdev)
>  {
>  	struct pxa_camera_dev *pcdev;
> +	struct pxacamera_platform_data pdata_dt;
>  	struct resource *res;
>  	void __iomem *base;
>  	int irq;
> @@ -1676,6 +1742,13 @@ static int pxa_camera_probe(struct platform_device *pdev)
>  	pcdev->res = res;
>  
>  	pcdev->pdata = pdev->dev.platform_data;
> +	if (&pdev->dev.of_node && !pcdev->pdata)
> +		err = pxa_camera_pdata_from_dt(&pdev->dev, &pdata_dt);
> +	if (err < 0)
> +		return err;
> +	else
> +		pcdev->pdata = &pdata_dt;

This will Oops, if someone decides to dereference pcdev->pdata outside of 
this function. pdata_dt is on stack and you store a pointer to it in your 
device data... But since ->pdata doesn't seem to be used anywhere else in 
this driver, maybe remove that struct member completely?

> +
>  	pcdev->platform_flags = pcdev->pdata->flags;
>  	if (!(pcdev->platform_flags & (PXA_CAMERA_DATAWIDTH_8 |
>  			PXA_CAMERA_DATAWIDTH_9 | PXA_CAMERA_DATAWIDTH_10))) {
> @@ -1799,10 +1872,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 = "mrvl,pxa_camera", },

Another thing I failed to comment upon: I think DT should contain only 
hardware descriptions, nothing driver specific, and "pxa_camera" is more a 
name of the driver, than the hardware? Maybe something like 
"marvell,pxa27x-cif" would be suitable?

Thanks
Guennadi

> +	{},
> +};
> +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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> On Sun, 15 Jun 2014, Robert Jarzmik wrote:
>> +static const struct of_device_id pxacamera_dt_ids[] = {
>> +	{ .compatible = "mrvl,pxa_camera", },
>
> as Documentation/devicetree/bindings/vendor-prefixes.txt defines, it 
> should be "marvell."
OK, I'll ask for confirmation to Haojian and Grant, as marvell and mrvl are both
used, and I have many pending patches. So I'd like to be sure, and then amend
all my patches at once.

>> +	dev_info(dev, "RJK: %s()\n", __func__);
>
> I have nothing against attributing work to respective authors, but I don't 
> think this makes a lot of sense in the long run in the above form :) Once 
> you've verified that your binding is working and this function is working, 
> either remove this or make it more informative - maybe at the end of this 
> function, also listing a couple of important parameters, that you obtained 
> from DT.
Ah, debug leftover. RJK is my "special mark" for "remove me before
submitting". See how well it worked :)


>
>> +	err = of_property_read_u32(np, "mclk_10khz",
>> +				   (u32 *)&pdata->mclk_10khz);
>
> I think we'll be frowned upon for this :) PXA270 doesn't support CCF, does 
> it? Even if it doesn't we probably should use the standard 
> "clock-frequency" property in any case. Actually, I missed to mention on 
> this in my comments to your bindings documentation.

You're right. This should be the normal Hz clock stuff. For V2.
>>  	pcdev->pdata = pdev->dev.platform_data;
>> +	if (&pdev->dev.of_node && !pcdev->pdata)
>> +		err = pxa_camera_pdata_from_dt(&pdev->dev, &pdata_dt);
>> +	if (err < 0)
>> +		return err;
>> +	else
>> +		pcdev->pdata = &pdata_dt;
>
> This will Oops, if someone decides to dereference pcdev->pdata outside of 
> this function. pdata_dt is on stack and you store a pointer to it in your 
> device data... But since ->pdata doesn't seem to be used anywhere else in 
> this driver, maybe remove that struct member completely?
Yep, obliteration, good for me.
There is indeed no purpose in keeping this pdata in pcdev, its only purpose was
to get mclk and platform_flags. For V2 also.

>> +
>>  	pcdev->platform_flags = pcdev->pdata->flags;
>>  	if (!(pcdev->platform_flags & (PXA_CAMERA_DATAWIDTH_8 |
>>  			PXA_CAMERA_DATAWIDTH_9 | PXA_CAMERA_DATAWIDTH_10))) {
>> @@ -1799,10 +1872,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 = "mrvl,pxa_camera", },
>
> Another thing I failed to comment upon: I think DT should contain only 
> hardware descriptions, nothing driver specific, and "pxa_camera" is more a 
> name of the driver, than the hardware? Maybe something like 
> "marvell,pxa27x-cif" would be suitable?
Yeah, sure, whatever you like.

Thanks for the review.

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html