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

[Sakari Ailus]

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.

> 
> 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.

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

...

> 
> > 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?

-- 
Kind regards,

Sakari Ailus