Re: [PATCH v2 5/6] media: dt-bindings: media: i2c: Add mlx7502x camera sensor binding

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

On Thu, Jul 14, 2022 at 01:00:24PM +0200, Krzysztof Kozlowski wrote:
> On 14/07/2022 12:45, Laurent Pinchart wrote:
> > On Thu, Jul 14, 2022 at 12:35:52PM +0200, Krzysztof Kozlowski wrote:
> >> On 14/07/2022 12:06, Laurent Pinchart wrote:
> >>> Hi Volodymyr,
> >>>
> >>> Thank you for the patch.
> >>>
> >>> On Thu, Jul 14, 2022 at 11:34:47AM +0300, Volodymyr Kharuk wrote:
> >>>> Add device tree binding of the mlx7502x and update MAINTAINERS
> >>>>
> >>>> Signed-off-by: Volodymyr Kharuk <vkh@melexis.com>
> >>>> ---
> >>>>  .../bindings/media/i2c/melexis,mlx7502x.yaml  | 146 ++++++++++++++++++
> >>>>  MAINTAINERS                                   |   1 +
> >>>>  2 files changed, 147 insertions(+)
> >>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml b/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..4ac91f7a26b6
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/media/i2c/melexis,mlx7502x.yaml
> >>>> @@ -0,0 +1,146 @@
> >>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >>>> +%YAML 1.2
> >>>> +---
> >>>> +$id: http://devicetree.org/schemas/media/i2c/melexis,mlx7502x.yaml#
> >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>> +
> >>>> +title: Melexis ToF 7502x MIPI CSI-2 Sensor
> >>>> +
> >>>> +maintainers:
> >>>> +  - Volodymyr Kharuk <vkh@melexis.com>
> >>>> +
> >>>> +description: |-
> >>>> +  Melexis ToF 7502x sensors has a CSI-2 output. It supports 2 and 4 lanes,
> >>>> +  and mipi speeds are 300, 600, 704, 800, 904, 960Mbs. Supported format is RAW12.
> >>>> +  Sensor 75026 is QVGA, while 75027 is VGA sensor.
> >>>> +  If you use compatible = "melexis,mlx7502x", then autodetect will be called.
> >>>
> >>> I'd move this last line as a description of the compatible property, but
> >>> I'm also not sure this should be mentioned in the DT bindings, as it's a
> >>> driver implementation detail. I'm actually not sure we should support it
> >>> with three different compatible values as proposed, as without this
> >>> documentation users will have a hard time figuring out what compatible
> >>> value to pick.
> >>>
> >>> One option would be to support the following three compatible values:
> >>>
> >>> 	compatible = "melexis,mlx75026", "melexis,mlx7502x";
> >>> 	compatible = "melexis,mlx75027", "melexis,mlx7502x";
> >>> 	compatible = "melexis,mlx7502x";
> >>>
> >>> The last one only would trigger autodetection. I'm still not sure how to
> >>> document that properly in bindings though.
> >>
> >> I missed that part of binding.
> >>
> >> Wildcards are not allowed in compatible, so mlx7502x has to go.
> > 
> > Really ? We've had fallback generic compatible strings since the
> > beginning.
> 
> Fallback generic compatibles are allowed. Wildcards not. Wildcards were
> actually never explicitly allowed, they just slipped in to many
> bindings... We have several discussions on this on mailing list, so no
> real point to repeat the arguments.
> 
> There is a difference between generic fallback. If the device follows
> clear specification and version, e.g. "foo-bar-v4", you can use it for
> generic compatible. This is more common in SoC components. Requirement -
> there is a clear mapping between versions and SoCs.

I'm not sure to see a clear difference between the two concepts.

> >> Anyway what does this autodetection mean?
> > 
> > As far as I understand, it means that the driver will use a hardware
> > identification register to figure out if the sensor is a 75026 or 75027.
> 
> Then there is no need to define 75027 compatible. DT is for cases where
> autodetection does not work...

It's autodetection of the exact device model, those are I2C devices so
we still need DT, and we still need to know that it's one of the
MLX75026 or MLX75027.

> > The upside is that one doesn't need to change the device tree when
> > swapping between those two sensors. The downside is that the sensor
> > needs to be powered up at probe time. Depending on the platform, one of
> > those two behaviours is preferred. Auto-detection is nice, but in
> > laptops or tablets (not a use case for this particular device, but the
> > problem applies to camera sensors in general), it would mean that the
> > privacy LED of the camera could be briefly lit at boot time due to the
> > sensor being powered on, which can worry users.
> 
> OK, that's reasonable argument for dedicated compatible but I don't
> understand why you cannot perform autodetection the moment device is
> actually powered up (first time). I understand it is nice and easy to
> make everything in the probe and most devices perform it that way. But
> if you don't want to do it in the probe - DT is not a workaround for this...

For cameras, we often deal with complex pipelines with multiple external
devices and multiple IP cores, with drivers that need to communicate
with each other to initialize the complete camera system. For instance,
each camera-related component in the system registers itself in a media
graph that can be queried from userspace and exposes information about
all devices, including their model. There's no power up of any device
when this query is being performed from userspace. It could possibly be
changed (and maybe it should, for reasons unrelated to this discussion),
but we're looking at pretty much a complete redesign of V4L2 and MC
then.

-- 
Regards,

Laurent Pinchart