Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sakari,

(Adding Dave and Naush to the CC list)

On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
> > On 28/07/2022 15:02, Alexander Stein wrote:
> > > According to product brief they are identical from software point of view.
> > > Differences are a different chief ray angle (CRA) and the package.
> > > 
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> > > ---
> > >  drivers/media/i2c/ov9282.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index 8a252bf3b59f..c8d83a29f9bb 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
> > >  };
> > >  
> > >  static const struct of_device_id ov9282_of_match[] = {
> > > +	{ .compatible = "ovti,ov9281" },
> > 
> > The devices seem entirely compatible, so why you add a new compatible
> > and not re-use existing?
> > 
> > The difference in lens does not explain this.
> 
> It is typically necessary to know what kind of related hardware can be
> found in the system, beyond just the device's register interface. Apart
> from USB cameras, less integrated cameras require low-level software
> control in which specific device properties are important. In this case it
> could be the lens shading table, among other things.
> 
> 	https://www.ovt.com/sensor/ov9282/
> 
> Therefore I think adding a specific compatible string for this one is
> justified.
> 
> Also cc Laurent.

Interesting coincidence, we've talked about this topic (as part of a
broader discussion) no later than yesterday.

I agree with Sakari in that userspace needs to know the exact model of
the camera sensor. I don't see a good alternative to providing that
information through the platform firmware, so the device tree in this
case. The question is how it should be provided (the question of how it
should then be exposed to userspace is also important, but out of scope
in this discussion).

The compatible string is meant to indicate a device's compatibility with
"something", and that something is often considered from the point of
view of software support, and in particular to pick an appropriate
kernel driver and tune its behaviour for the device. Here, one could
argue that the exact model is also needed to ensure proper software
support, but in userspace this time, not in the kernel. I think using a
dedicated compatible string would be reasonable. An alternative would be
to use another DT property, which should then be standardized. I'm not
sure it's worth it.

Broadening the discussion, we also need to know detailed information
about the camera lens (I'm talking about the lens itself here, not the
lens controller IC that controls the motor that moves the focus lens).
The lens isn't described in the device tree with a dedicated device tree
node today, and I don't think it should (I'd have a hard time coming up
with a naming scheme for lenses that we could use in compatible strings,
and the lens-related data that a system requires can possibly vary based
not only on the lens itself but on the ISP that the camera sensor is
used with). Typical useful data are the lens movement range, the
hyperfocal distance, but also the lens shading tables. (Part of) that
information is sometimes stored in non-volatile memory in the camera
module (OTP in the camera sensor itself, or a separate EEPROM), but
that's not always the case. We have considered the possibility of
storing the information in the device tree, but I doubt that would be
accepted. We can store the information in userspace in configuration
files, but we will still need to device tree to provide lens
identification information to select the correct configuration file. I
don't know how that should be done.

-- 
Regards,

Laurent Pinchart