| Message ID | 20240521162950.6987-2-sylvain.petinot@foss.st.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: Add driver for ST VD56G3 camera sensor | expand |
On 21/05/2024 18:29, Sylvain Petinot wrote: > Add devicetree bindings Documentation for ST VD56G3 & ST VD66GY camera > sensors. Update MAINTAINERS file. > A nit, subject: drop second/last, redundant "binding". The "dt-bindings" prefix is already stating that these are bindings. See also: https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > 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'? > @@ -0,0 +1,132 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +# Copyright (c) 2024 STMicroelectronics SA. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/st,st-vd56g3.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: STMicroelectronics VD56G3 Global Shutter Image Sensor > + > +maintainers: > + - Benjamin Mugnier <benjamin.mugnier@foss.st.com> > + - Sylvain Petinot <sylvain.petinot@foss.st.com> > + > +description: |- > + The STMicroelectronics VD56G3 is a 1.5 M pixel global shutter image sensor This claims device is VD56G3, not ST-VD56G3. > + with an active array size of 1124 x 1364 (portrait orientation). It is > + programmable through I2C, the address is fixed to 0x10. The sensor output is > + available via CSI-2, which is configured as either 1 or 2 data lanes. The > + sensor provides 8 GPIOS that can be used for external LED signal > + (synchronized with sensor integration periods) > + > +properties: > + compatible: > + enum: > + - st,st-vd56g3 > + - st,st-vd66gy > + description: > + Two variants are availables; VD56G3 is a monochrome sensor while VD66GY > + is a colour variant. > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + vcore-supply: > + description: Digital core power supply (1.15V) > + > + vddio-supply: > + description: Digital IO power supply (1.8V) > + > + vana-supply: > + description: Analog power supply (2.8V) > + > + reset-gpios: > + description: Sensor reset active low GPIO (XSHUTDOWN) > + maxItems: 1 > + > + 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? > + $ref: /schemas/types.yaml#/definitions/uint32-array > + minItems: 1 > + maxItems: 8 > + items: > + minimum: 0 > + maximum: 7 > + > + port: > + $ref: /schemas/graph.yaml#/$defs/port-base missing additionalProperties: false > + > + properties: > + endpoint: > + $ref: /schemas/media/video-interfaces.yaml# > + unevaluatedProperties: false > + > + properties: > + data-lanes: > + minItems: 1 > + maxItems: 2 > + items: > + enum: [1, 2] > + > + link-frequencies: > + minItems: 1 maxItems is enough > + maxItems: 1 > + items: > + enum: [402000000, 750000000] > + > + lane-polarities: > + minItems: 1 > + maxItems: 3 > + description: Any lane can be inverted or not. > + > + required: > + - data-lanes > + - link-frequencies > + > +required: > + - compatible > + - reg > + - clocks > + - vcore-supply > + - vddio-supply > + - vana-supply > + - reset-gpios > + - port > + Not a video-interface-device.yaml type of device? Best regards, Krzysztof
Hi Krzysztof, Thanks for the review. On 5/21/2024 7:37 PM, Krzysztof Kozlowski wrote: > On 21/05/2024 18:29, Sylvain Petinot wrote: >> Add devicetree bindings Documentation for ST VD56G3 & ST VD66GY camera >> sensors. Update MAINTAINERS file. >> > > A nit, subject: drop second/last, redundant "binding". The "dt-bindings" > prefix is already stating that these are bindings. > See also: > https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > Ok, fixed in V3. > >> 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] 2- rename this driver properly ('vd56g3') and keep the two others the old way (I personally don't like this option) 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. [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 > >> @@ -0,0 +1,132 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +# Copyright (c) 2024 STMicroelectronics SA. >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/media/i2c/st,st-vd56g3.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: STMicroelectronics VD56G3 Global Shutter Image Sensor >> + >> +maintainers: >> + - Benjamin Mugnier <benjamin.mugnier@foss.st.com> >> + - Sylvain Petinot <sylvain.petinot@foss.st.com> >> + >> +description: |- >> + The STMicroelectronics VD56G3 is a 1.5 M pixel global shutter image sensor > > This claims device is VD56G3, not ST-VD56G3. Sure, linked with previous point. > >> + with an active array size of 1124 x 1364 (portrait orientation). It is >> + programmable through I2C, the address is fixed to 0x10. The sensor output is >> + available via CSI-2, which is configured as either 1 or 2 data lanes. The >> + sensor provides 8 GPIOS that can be used for external LED signal >> + (synchronized with sensor integration periods) >> + >> +properties: >> + compatible: >> + enum: >> + - st,st-vd56g3 >> + - st,st-vd66gy >> + description: >> + Two variants are availables; VD56G3 is a monochrome sensor while VD66GY >> + is a colour variant. >> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + maxItems: 1 >> + >> + vcore-supply: >> + description: Digital core power supply (1.15V) >> + >> + vddio-supply: >> + description: Digital IO power supply (1.8V) >> + >> + vana-supply: >> + description: Analog power supply (2.8V) >> + >> + reset-gpios: >> + description: Sensor reset active low GPIO (XSHUTDOWN) >> + maxItems: 1 >> + >> + 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. 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). > >> + $ref: /schemas/types.yaml#/definitions/uint32-array >> + minItems: 1 >> + maxItems: 8 >> + items: >> + minimum: 0 >> + maximum: 7 >> + >> + port: >> + $ref: /schemas/graph.yaml#/$defs/port-base > > missing additionalProperties: false Ok, fixed in V3. > >> + >> + properties: >> + endpoint: >> + $ref: /schemas/media/video-interfaces.yaml# >> + unevaluatedProperties: false >> + >> + properties: >> + data-lanes: >> + minItems: 1 >> + maxItems: 2 >> + items: >> + enum: [1, 2] > > >> + >> + link-frequencies: >> + minItems: 1 > > maxItems is enough Ok, fixed in V3. > >> + maxItems: 1 >> + items: >> + enum: [402000000, 750000000] >> + >> + lane-polarities: >> + minItems: 1 >> + maxItems: 3 >> + description: Any lane can be inverted or not. >> + >> + required: >> + - data-lanes >> + - link-frequencies >> + >> +required: >> + - compatible >> + - reg >> + - clocks >> + - vcore-supply >> + - vddio-supply >> + - vana-supply >> + - reset-gpios >> + - port >> + > > > Not a video-interface-device.yaml type of device? Good point, something I'll consider in V3 > > Best regards, > Krzysztof > -- Sylvain
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
On 21/05/2024 18:29, Sylvain Petinot wrote: > Add devicetree bindings Documentation for ST VD56G3 & ST VD66GY camera > sensors. Update MAINTAINERS file. > > Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com> > diff --git a/MAINTAINERS b/MAINTAINERS > index ef6be9d95143..554e6861425b 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -20885,6 +20885,15 @@ S: Maintained > F: Documentation/hwmon/stpddc60.rst > F: drivers/hwmon/pmbus/stpddc60.c > > +ST VD56G3 DRIVER > +M: Benjamin Mugnier <benjamin.mugnier@foss.st.com> > +M: Sylvain Petinot <sylvain.petinot@foss.st.com> > +L: linux-media@vger.kernel.org > +S: Maintained > +T: git git://linuxtv.org/media_tree.git This is a friendly reminder during the review process. It seems my or other reviewer's previous comments were not fully addressed. Maybe the feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them. Thank you. Best regards, Krzysztof
Hi Sylvain, On Mon, May 27, 2024 at 03:14:35PM +0200, Sylvain Petinot wrote: > >> 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] > 2- rename this driver properly ('vd56g3') and keep the two others the > old way (I personally don't like this option) > 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. > > [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 The driver could be renamed to align with a large majority that use the same name as the bindings without the vendor prefix. You could add MODULE_ALIAS() to help user space to cope with the change. The DT compatible string indeed should reflect the name of the device, the driver is indeed another matter.
Hi Krzysztof, On Mon, May 27, 2024 at 09:04:38PM +0200, Krzysztof Kozlowski wrote: > On 21/05/2024 18:29, Sylvain Petinot wrote: > > Add devicetree bindings Documentation for ST VD56G3 & ST VD66GY camera > > sensors. Update MAINTAINERS file. > > > > Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com> > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index ef6be9d95143..554e6861425b 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -20885,6 +20885,15 @@ S: Maintained > > F: Documentation/hwmon/stpddc60.rst > > F: drivers/hwmon/pmbus/stpddc60.c > > > > +ST VD56G3 DRIVER I might add this is a sensor, i.e. "ST VD653G IMAGE SENSOR DRIVER". > > +M: Benjamin Mugnier <benjamin.mugnier@foss.st.com> > > +M: Sylvain Petinot <sylvain.petinot@foss.st.com> > > +L: linux-media@vger.kernel.org > > +S: Maintained > > +T: git git://linuxtv.org/media_tree.git > > This is a friendly reminder during the review process. > > It seems my or other reviewer's previous comments were not fully > addressed. Maybe the feedback got lost between the quotes, maybe you > just forgot to apply it. Please go back to the previous discussion and > either implement all requested changes or keep discussing them. The above MAINTAINERS entry is roughly in line with what else we have for the Media tree. I'm in favour of listing the people who would look after the driver, not just those who merge the patches (or even send PRs to Linus). In other words, I think the above entry is fine as-is. Sylvain: could you also add the file under "V4L2 CAMERA SENSOR DRIVERS" that lists myself as the maintainer? Thanks.
On 28/05/2024 11:22, Sakari Ailus wrote: > Hi Krzysztof, > > On Mon, May 27, 2024 at 09:04:38PM +0200, Krzysztof Kozlowski wrote: >> On 21/05/2024 18:29, Sylvain Petinot wrote: >>> Add devicetree bindings Documentation for ST VD56G3 & ST VD66GY camera >>> sensors. Update MAINTAINERS file. >>> >>> Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com> >> >> >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index ef6be9d95143..554e6861425b 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -20885,6 +20885,15 @@ S: Maintained >>> F: Documentation/hwmon/stpddc60.rst >>> F: drivers/hwmon/pmbus/stpddc60.c >>> >>> +ST VD56G3 DRIVER > > I might add this is a sensor, i.e. "ST VD653G IMAGE SENSOR DRIVER". > >>> +M: Benjamin Mugnier <benjamin.mugnier@foss.st.com> >>> +M: Sylvain Petinot <sylvain.petinot@foss.st.com> >>> +L: linux-media@vger.kernel.org >>> +S: Maintained >>> +T: git git://linuxtv.org/media_tree.git >> >> This is a friendly reminder during the review process. >> >> It seems my or other reviewer's previous comments were not fully >> addressed. Maybe the feedback got lost between the quotes, maybe you >> just forgot to apply it. Please go back to the previous discussion and >> either implement all requested changes or keep discussing them. > > The above MAINTAINERS entry is roughly in line with what else we have for > the Media tree. I'm in favour of listing the people who would look after > the driver, not just those who merge the patches (or even send PRs to > Linus). I did not propose to drop the entry. > > In other words, I think the above entry is fine as-is. I propose to drop duplicated, redundant git entry. Maintainer of this driver does not have access to git tree and the git tree is already explained in media subsystem entry. If you ever update the git tree, you need to update 100 driver entries which is meaningless... Best regards, Krzysztof
Hi Krzysztof, On Tue, May 28, 2024 at 11:46:00AM +0200, Krzysztof Kozlowski wrote: > On 28/05/2024 11:22, Sakari Ailus wrote: > > Hi Krzysztof, > > > > On Mon, May 27, 2024 at 09:04:38PM +0200, Krzysztof Kozlowski wrote: > >> On 21/05/2024 18:29, Sylvain Petinot wrote: > >>> Add devicetree bindings Documentation for ST VD56G3 & ST VD66GY camera > >>> sensors. Update MAINTAINERS file. > >>> > >>> Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com> > >> > >> > >>> diff --git a/MAINTAINERS b/MAINTAINERS > >>> index ef6be9d95143..554e6861425b 100644 > >>> --- a/MAINTAINERS > >>> +++ b/MAINTAINERS > >>> @@ -20885,6 +20885,15 @@ S: Maintained > >>> F: Documentation/hwmon/stpddc60.rst > >>> F: drivers/hwmon/pmbus/stpddc60.c > >>> > >>> +ST VD56G3 DRIVER > > > > I might add this is a sensor, i.e. "ST VD653G IMAGE SENSOR DRIVER". > > > >>> +M: Benjamin Mugnier <benjamin.mugnier@foss.st.com> > >>> +M: Sylvain Petinot <sylvain.petinot@foss.st.com> > >>> +L: linux-media@vger.kernel.org > >>> +S: Maintained > >>> +T: git git://linuxtv.org/media_tree.git > >> > >> This is a friendly reminder during the review process. > >> > >> It seems my or other reviewer's previous comments were not fully > >> addressed. Maybe the feedback got lost between the quotes, maybe you > >> just forgot to apply it. Please go back to the previous discussion and > >> either implement all requested changes or keep discussing them. > > > > The above MAINTAINERS entry is roughly in line with what else we have for > > the Media tree. I'm in favour of listing the people who would look after > > the driver, not just those who merge the patches (or even send PRs to > > Linus). > > I did not propose to drop the entry. > > > > > In other words, I think the above entry is fine as-is. > > I propose to drop duplicated, redundant git entry. Maintainer of this Ah, I agree, that makes sense. > driver does not have access to git tree and the git tree is already > explained in media subsystem entry. If you ever update the git tree, you > need to update 100 driver entries which is meaningless...
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
Hi Krzysztof, Sakari On 5/28/2024 12:01 PM, Sakari Ailus wrote: > Hi Krzysztof, > > On Tue, May 28, 2024 at 11:46:00AM +0200, Krzysztof Kozlowski wrote: >> On 28/05/2024 11:22, Sakari Ailus wrote: >>> Hi Krzysztof, >>> >>> On Mon, May 27, 2024 at 09:04:38PM +0200, Krzysztof Kozlowski wrote: >>>> On 21/05/2024 18:29, Sylvain Petinot wrote: >>>>> Add devicetree bindings Documentation for ST VD56G3 & ST VD66GY camera >>>>> sensors. Update MAINTAINERS file. >>>>> >>>>> Signed-off-by: Sylvain Petinot <sylvain.petinot@foss.st.com> >>>> >>>> >>>>> diff --git a/MAINTAINERS b/MAINTAINERS >>>>> index ef6be9d95143..554e6861425b 100644 >>>>> --- a/MAINTAINERS >>>>> +++ b/MAINTAINERS >>>>> @@ -20885,6 +20885,15 @@ S: Maintained >>>>> F: Documentation/hwmon/stpddc60.rst >>>>> F: drivers/hwmon/pmbus/stpddc60.c >>>>> >>>>> +ST VD56G3 DRIVER >>> >>> I might add this is a sensor, i.e. "ST VD653G IMAGE SENSOR DRIVER". >>> >>>>> +M: Benjamin Mugnier <benjamin.mugnier@foss.st.com> >>>>> +M: Sylvain Petinot <sylvain.petinot@foss.st.com> >>>>> +L: linux-media@vger.kernel.org >>>>> +S: Maintained >>>>> +T: git git://linuxtv.org/media_tree.git >>>> >>>> This is a friendly reminder during the review process. >>>> >>>> It seems my or other reviewer's previous comments were not fully >>>> addressed. Maybe the feedback got lost between the quotes, maybe you >>>> just forgot to apply it. Please go back to the previous discussion and >>>> either implement all requested changes or keep discussing them. >>> >>> The above MAINTAINERS entry is roughly in line with what else we have for >>> the Media tree. I'm in favour of listing the people who would look after >>> the driver, not just those who merge the patches (or even send PRs to >>> Linus). >> >> I did not propose to drop the entry. >> >>> >>> In other words, I think the above entry is fine as-is. >> >> I propose to drop duplicated, redundant git entry. Maintainer of this > > Ah, I agree, that makes sense. Thanks for clarifying, git entry will be drop in V3. > >> driver does not have access to git tree and the git tree is already >> explained in media subsystem entry. If you ever update the git tree, you >> need to update 100 driver entries which is meaningless... > -- Sylvain
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 @@ -0,0 +1,132 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright (c) 2024 STMicroelectronics SA. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/st,st-vd56g3.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: STMicroelectronics VD56G3 Global Shutter Image Sensor + +maintainers: + - Benjamin Mugnier <benjamin.mugnier@foss.st.com> + - Sylvain Petinot <sylvain.petinot@foss.st.com> + +description: |- + The STMicroelectronics VD56G3 is a 1.5 M pixel global shutter image sensor + with an active array size of 1124 x 1364 (portrait orientation). It is + programmable through I2C, the address is fixed to 0x10. The sensor output is + available via CSI-2, which is configured as either 1 or 2 data lanes. The + sensor provides 8 GPIOS that can be used for external LED signal + (synchronized with sensor integration periods) + +properties: + compatible: + enum: + - st,st-vd56g3 + - st,st-vd66gy + description: + Two variants are availables; VD56G3 is a monochrome sensor while VD66GY + is a colour variant. + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + vcore-supply: + description: Digital core power supply (1.15V) + + vddio-supply: + description: Digital IO power supply (1.8V) + + vana-supply: + description: Analog power supply (2.8V) + + reset-gpios: + description: Sensor reset active low GPIO (XSHUTDOWN) + maxItems: 1 + + st,leds: + description: + Sensor's GPIOs used for external LED control. Signal being the enveloppe + of the integration time. + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 1 + maxItems: 8 + items: + minimum: 0 + maximum: 7 + + port: + $ref: /schemas/graph.yaml#/$defs/port-base + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + minItems: 1 + maxItems: 2 + items: + enum: [1, 2] + + link-frequencies: + minItems: 1 + maxItems: 1 + items: + enum: [402000000, 750000000] + + lane-polarities: + minItems: 1 + maxItems: 3 + description: Any lane can be inverted or not. + + required: + - data-lanes + - link-frequencies + +required: + - compatible + - reg + - clocks + - vcore-supply + - vddio-supply + - vana-supply + - reset-gpios + - port + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + camera-sensor@10 { + compatible = "st,st-vd56g3"; + reg = <0x10>; + + clocks = <&camera_clk_12M>; + + vcore-supply = <&camera_vcore_v1v15>; + vddio-supply = <&camera_vddio_v1v8>; + vana-supply = <&camera_vana_v2v8>; + + reset-gpios = <&gpio 5 GPIO_ACTIVE_LOW>; + st,leds = <6>; + + port { + endpoint { + data-lanes = <1 2>; + link-frequencies = /bits/ 64 <402000000>; + remote-endpoint = <&csiphy0_ep>; + }; + }; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index ef6be9d95143..554e6861425b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -20885,6 +20885,15 @@ S: Maintained F: Documentation/hwmon/stpddc60.rst F: drivers/hwmon/pmbus/stpddc60.c +ST VD56G3 DRIVER +M: Benjamin Mugnier <benjamin.mugnier@foss.st.com> +M: Sylvain Petinot <sylvain.petinot@foss.st.com> +L: linux-media@vger.kernel.org +S: Maintained +T: git git://linuxtv.org/media_tree.git +F: Documentation/devicetree/bindings/media/i2c/st,st-vd56g3.yaml +F: drivers/media/i2c/st-vd56g3.c + ST VGXY61 DRIVER M: Benjamin Mugnier <benjamin.mugnier@foss.st.com> M: Sylvain Petinot <sylvain.petinot@foss.st.com>
Add devicetree bindings Documentation for ST VD56G3 & ST VD66GY camera sensors. Update MAINTAINERS file. 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