Received-SPF: pass (google.com: domain of linux-kernel+bounces-198811-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <832ab797-3852-4b30-a0b1-7ea9c85fdfa6@foss.st.com>
Date: Mon, 3 Jun 2024 10:46:12 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From: Sylvain Petinot <sylvain.petinot@foss.st.com>
Subject: Re: [PATCH v2 1/2] media: dt-bindings: Add ST VD56G3 camera sensor
 binding
References: <20240521162950.6987-1-sylvain.petinot@foss.st.com>
 <20240521162950.6987-2-sylvain.petinot@foss.st.com>
 <2110ba34-658e-4d60-b524-2f5ead6c8d3e@linaro.org>
 <77fa3ed3-2341-4106-adf2-ec8bd9de91ff@foss.st.com>
 <ceab83fe-b741-4f9e-8b0c-9de3ca79fc55@linaro.org>
Content-Language: en-US
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        <benjamin.mugnier@foss.st.com>, <mchehab@kernel.org>,
        <robh@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <conor+dt@kernel.org>, Sakari Ailus
	<sakari.ailus@iki.fi>
CC: <linux-media@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
In-Reply-To: <ceab83fe-b741-4f9e-8b0c-9de3ca79fc55@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

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