Re: [PATCH 2/4] media: platform: dwc: Support for DW CSI-2 Host

[Hans Verkuil]

Hi Sakari,

Can you comment on this? You are much more a CSI sensor expert than I am.

On 16/05/17 20:18, Ramiro Oliveira wrote:
> Hi Hans,
> 
> Thank you very much for your feedback.
> 
> On 5/8/2017 11:38 AM, Hans Verkuil wrote:
>> Hi Ramiro,
>>
>> My sincere apologies for the long delay in reviewing this. The good news is that
>> I should have more time for reviews going forward, so I hope I'll be a lot quicker
>> in the future.
>>
>> On 03/07/2017 03:37 PM, Ramiro Oliveira wrote:

<snip>

>>> +		if (mf->width == bt->width && mf->height == bt->width) {
>>
>> This is way too generic. There are many preset timings that have the same
>> width and height but different blanking periods.
>>
>> I am really not sure how this is supposed to work. If you want to support
>> HDMI here, then I would expect to see support for the s_dv_timings op and friends
>> in this driver. And I don't see support for that in the host driver either.
>>
>> Is this a generic csi driver, or specific for hdmi? Or supposed to handle both?
> 
> This is a generic CSI driver.
> 
>>
>> Can you give some background and clarification of this?
> 
> This piece of code might seem strange but I'm just using it fill our controller
> timing configuration.
> 
> I don't have any specific requirements, but they should match, more or less, the
> sensor configurations, so I decided to re-use the HDMI blanking values, since,
> usually, they match with the sensor configurations
> 
> So, my intention is to check if there is any HDMI preset that matches the
> required width and height, and then use the blanking values to configure our
> controller. I know this might not be very common, and I'm open to use different
> approaches, but from my perspective it seems to work fine.

Regards,

	Hans