Re: [PATCH v2 1/2] media: dt-bindings: Add ST VD56G3 camera sensor binding

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 27/05/2024 15:14, Sylvain Petinot wrote:
>>
>>> Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com>
>>> ---
>>>  .../bindings/media/i2c/st,st-vd56g3.yaml      | 132 ++++++++++++++++++
>>>  MAINTAINERS                                   |   9 ++
>>>  2 files changed, 141 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml b/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
>>> new file mode 100644
>>> index 000000000000..22cb2557e311
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml
>>
>> Why duplicated 'st'?
> 
> Legacy : our first st-mipid02 driver was upstream this way few years back.
> 
> We have 3 options :
> 
> 1- keep this unpleasant naming to keep consistency with st-mipid02 [1]
> and st-vgxy61 [2]

? Unpleasant?
Please follow generic rules. Filename must match compatible and
compatible must follow vendor,device format.

> 2- rename this driver properly ('vd56g3') and keep the two others the
> old way (I personally don't like this option)

We do not talk about driver here. Does not matter.

> 3- rename this driver properly ('vd56g3') and in a second patch rename
> the two others drivers.
> 
> I would be interested to get Sakari's opinion on this subject.

About what? Renaming drivers?

> 
> [1]:
> https://elixir.bootlin.com/linux/v6.9.1/source/drivers/media/i2c/st-mipid02.c
> 
> [2]:
> https://elixir.bootlin.com/linux/v6.9.1/source/drivers/media/i2c/st-vgxy61.c

Howe are these drivers anyhow related to the *binding*?


...

>>> +
>>> +  st,leds:
>>> +    description:
>>> +      Sensor's GPIOs used for external LED control. Signal being the enveloppe
>>> +      of the integration time.
>>
>> More information is needed. GPIOs coming from LED or SoC? What's the
>> meaning of values?
> 
> The vd56g3 image sensor provides 8 GPIOS that can be used for different
> use cases (external led controls, synchronization between master/slave
> sensors, external sensor trigger, etc.). This submission supports only
> the first use case: the control of one(or multiple) external LED.

What your driver supports is not really relevant. Describe hardware.

> 
> The vd56g3 sensor family are optimized for visible and near infrared
> scenes. In NIR, external IR leds are generally used for illumination.
> 
> With such use case, a led (or a led driver) can be connected directly to
> one of the 8 GPIOs of the sensor. On the driver side, when a led is
> configured in the dt, the driver will configure the sensor accordingly.
> It will also offer an optional "V4L2_FLASH_LED_MODE_FLASH" control to
> start/stop the external control.
> 
> Different signal modes are supported by the HW, but the default
> (implemented) one is a "strobe" mode where signal is the envelope of the
> integration time (IR led is on while image sensor is integrating).

You did not explain the meaning of the property. Please describe the
hardware and the meaning of values used in this property.



Best regards,
Krzysztof