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

From: Sakari Ailus <sakari.ailus@iki.fi>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Alexander Stein <alexander.stein@ew.tq-group.com>,
	"Paul J . Murphy" <paul.j.murphy@intel.com>,
	Daniele Alessandrelli <daniele.alessandrelli@intel.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	Dave Stevenson <dave.stevenson@raspberrypi.com>,
	Naushir Patuck <naush@raspberrypi.com>
Subject: Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible
Date: Tue, 2 Aug 2022 11:23:07 +0300	[thread overview]
Message-ID: <Yuje6wip8KEZG6Af@valkosipuli.retiisi.eu> (raw)
In-Reply-To: <8015285b-940b-503a-159e-66f10a34a999@linaro.org>

On Mon, Aug 01, 2022 at 08:08:58PM +0200, Krzysztof Kozlowski wrote:
> On 01/08/2022 20:07, Krzysztof Kozlowski wrote:
> > On 29/07/2022 10:18, Laurent Pinchart wrote:
> >> 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.
> > 
> > Specific compatible in binding is a requirement. No one discussed this.
> > However not in the driver. None of the arguments above justify adding
> > such binding, unless user-space depends on matching compatible, but not
> > real compatible?
> 
> Eh, now I used vague words. This should be instead:
> 
> "However not in the driver. None of the arguments above justify adding
> such compatible to driver, unless user-space depends on matching
> compatible, but not real compatible?"

If I understand you right, you'd put the more specific model name as well
as the more generic one to the compatible property and let the driver match
against the more generic one?

But in this case neither of these models is more generic than the other.

-- 
Sakari Ailus