Guennadi Liakhovetski <g.liakhovetski@xxxxxx> 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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
