Re: [PATCH v9 2/4] media: i2c: Add MAX9286 driver

[Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>]

Hi Sakari,

On 19/05/2020 09:10, Sakari Ailus wrote:
> Hi Kieran,
> 
> On Mon, May 18, 2020 at 12:45:18PM +0100, Kieran Bingham wrote:
>> Hi Sakari,
>>
>> There are only fairly minor comments here, fix ups will be included in a
>> v10.
>>
>> Is there anything major blocking integration?
> 
> Not that I can see. But please see my comments below.

Thanks,

We might have some more work tidying up the DT validation anyway which
has come too late, and perhaps is going to bump this to v5.9 now anyway.

I can still try but ... ;-S

At least hopefully now we /can/ see a path to integration though.

I probably don't care if it's 5.8 or 5.9 as long as it's not 8.5 ;-)

>>
>> Regards
>>
>> Kieran
>>
>>
>>
>> On 16/05/2020 22:51, Sakari Ailus wrote:
>>> Hi Kieran,
>>>
>>> Thanks for the update.
>>>
>>> On Tue, May 12, 2020 at 04:51:03PM +0100, Kieran Bingham wrote:
>>>
>>> ...
>>>
>>>> +static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
>>>> +				  struct v4l2_subdev_pad_config *cfg,
>>>> +				  struct v4l2_subdev_mbus_code_enum *code)
>>>> +{
>>>> +	if (code->pad || code->index > 0)
>>>> +		return -EINVAL;
>>>> +
>>>> +	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
>>>
>>> Why UYVY8_2X8 and not UYVY8_1X16? In general, the single sample / pixel
>>> variant of the format is generally used on the serial busses. This choice
>>> was made when serial busses were introduced.
>>
>> Ok - I presume this doesn't really have much effect anyway, they just
>> have to match for the transmitter/receiver?
> 
> In this case, yes. But it's harder to change later, so let's indeed do that
> now.

Yes indeed, I have to change my test scripts for the new configuration
(or we should update the scripts to get the configuration from the
device ;D)


>> But it makes sense to me, so I'll update to the 1x16 variant.
> 
> ...

done anyway ;-)

I see the ADV748x is using the 2x8 variants though ... (all the more
reason for our scripts to /get/ the correct version when propagating
formats).

Perhaps I should/could add the 1x16 formats to the ADV748x too. (later)


>>> And as you don't, you also won't know which frequencies are known to be
>>> safe to use. That said, perhaps where this device is used having a random
>>> frequency on that bus could not be an issue. Perhaps.
>>
>> Does this generate a range? or a list of static supported frequencies?
>>
>> We configure the pixel clock based upon the number of cameras connected,
>> and their pixel rates etc ...
>>
>> Are you saying that the frequency of this clock should be validated to
>> be a specific range? or are you talking about a different frequency?
> 
> It depends on the system. In general, only frequencies known to be safe
> should be used. If this one has enough shielding to guarantee there won't
> be problems in using a random frequency in the entire range, is there a
> guarantee that will be the case for all systems with this chip?

I have no idea here... Maybe Niklas knows more having dealt more with
the RCar-VIN/CSI parts.

It seems like this is something we can add later if necessary, by
extending the descriptions in the DT?

--
Kieran