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

[Sylvain Petinot <sylvain.petinot@foss.st.com>]

Hello Krzysztof,


On 5/27/2024 9:00 PM, Krzysztof Kozlowski wrote:
> 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*?

I got your point. bindings will be updated accordingly in V3 to follow
vendor,device format.

My point was more about consistency :
1- ensure driver name is aligned with the bindings name (without vendor
prefix)
2- ensure all ST image sensor drivers and bindings follow the same rules.
But, you're right, this is a out of bindings topic and I will handle it
separately.

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

Sure, this was more contextual information. Please find below a proposal
for the "st,leds" property :

st,leds:
  description:
    List sensor's GPIOs used to control strobe light sources during exposure
    time. The numbers identify the sensor pin on which the illumination
system
    is connected. GPIOs are active-high.
  $ref: /schemas/types.yaml#/definitions/uint32-array
  minItems: 1
  maxItems: 8
  items:
    minimum: 0
    maximum: 7

> 
> Best regards,
> Krzysztof
> 

--
Sylvain