Re: [PATCH v2 3/3] [media] atmel-isi: add primary DT support

[Josh Wu <josh.wu@atmel.com>]

Hi, Guennadi

On 5/19/2014 5:51 AM, Guennadi Liakhovetski wrote:
> On Tue, 25 Mar 2014, Josh Wu wrote:
>
>> This patch add the DT support for Atmel ISI driver.
>> It use the same v4l2 DT interface that defined in video-interfaces.txt.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> Cc: devicetree@vger.kernel.org
>> ---
>> v1 --> v2:
>>   refine the binding document.
>>   add port node description.
>>   removed the optional property.
>>
>>   .../devicetree/bindings/media/atmel-isi.txt        |   50 ++++++++++++++++++++
>>   drivers/media/platform/soc_camera/atmel-isi.c      |   31 +++++++++++-
>>   2 files changed, 79 insertions(+), 2 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/media/atmel-isi.txt
> [snip]
>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
>> index f4add0a..d6a1f7b 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> [snip]
>
>> @@ -885,6 +887,20 @@ static int atmel_isi_remove(struct platform_device *pdev)
>>   	return 0;
>>   }
>>   
>> +static int atmel_isi_probe_dt(struct atmel_isi *isi,
>> +			struct platform_device *pdev)
>> +{
>> +	struct device_node *node = pdev->dev.of_node;
>> +
>> +	/* Default settings for ISI */
>> +	isi->pdata.full_mode = 1;
>> +	isi->pdata.mck_hz = ISI_DEFAULT_MCLK_FREQ;
>> +	isi->pdata.frate = ISI_CFG1_FRATE_CAPTURE_ALL;
> The above flags eventually should probably partially be added as new
> driver-specific DT properties, partially derived from DT clock bindings.
> But I'm ok to have them fixed like this in the initial version.
>
>> +	isi->pdata.data_width_flags = ISI_DATAWIDTH_8 | ISI_DATAWIDTH_10;
> Whereas these flags, I think, should already now be derived from the
> bus-width standard property?

yes. I agree.

> v4l2_of_parse_parallel_bus() will extract
> them for you and I just asked Ben to add a call to
> v4l2_of_parse_endpoint() to his patch.

Is it better to call v4l2_of_parse_endpoint() in the atmel-isi driver? I 
think the dt parsing stuff should be done by host driver and sensor 
driver itself. No need to call v4l2_of_parse_endpoint() in soc-camera.c.

> Consequently you'll have to
> rearrange bus-width interpretation in isi_camera_try_bus_param() a bit and
> use OF parsing results there directly if available? Or maybe you find a
> better way. It would certainly be better to extend your probing code and
> just use OF results to initialise isi->width_flags, but that might be
> impossible, because OF parsing would be performed inside
> soc_camera_host_register() and your isi_camera_try_bus_param() can also be
> called immediately from it if all required I2C devices are already
> available?

I am little bit confuse here. I don't see any issue in above case. Since 
atmel_isi_probe_dt() will always be called earlier then 
soc_camera_host_register().
That means when soc_camera_host_register() called 
isi_camera_try_bus_param(), the isi->width_flags are already initialized 
in a valid value by atmel_isi_probe_dt().
Am I missing anything here?

> If your I2C subdevice drivers defer probing until the host has
> probed, then you could initialise .width_flags after
> soc_camera_host_register(), but you cannot rely on that.

I tested these two cases without any issue:
1. In dtb, the i2c sensor dt node probe earlier than atmel-isi dt node.
     i2c sensor will do a defer probe here as mclk is not found until 
atmel-isi driver probed and call soc_camera_host_register().
2. In dtb, the atmel-isi dt node is probed earlier than i2c sensor.

Best Regards,
Josh Wu

>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html