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@xxxxxxxxxxx>
>>>> ---
>>>> .../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
