LinuxLists.cc - [PATCH v2 0/2] media: i2c: Add support for alvium camera

2023-05-26 18:06:13

Subject: [PATCH v2 0/2] media: i2c: Add support for alvium camera

Hello All,

This series add support for Allied Vision Alvium camera.
The Alvium camera is shipped with sensor + isp in the same housing.
The camera can be equipped with one out of various sensor and abstract
the user from this. Camera is connected via MIPI CSI-2.

Working on top of Ideas on Board (branch: ideasonboard/v6.2/isi)
I'm able to test the driver on imx8mp-evk.
I collect also some patches to enable HDMI on imx8mp-evk from Pengutronix
(branch: pengutronix-imx8mp-hdmi)

I collect the patchset required to enable ISI + HDMI on imx8mp-evk into
the following branch from Avnet Silica Software & Services EMEA [1].

Some documentation on testing ISP and ISI of imx8mp-evk here [2].

Thanks all for the great work!

[1] - https://github.com/avs-sas/linux/tree/tm/ideasonboard/v6.4.0-rc2/isi/imx8mp_evk/alvium_drv_skel1e_v1
[2] - https://gist.github.com/Scott31393/077a10024a6058536d3f2fdde476265a

Tommaso Merciai (2):
media: dt-bindings: alvium: add document YAML binding
media: i2c: Add support for alvium camera

.../media/i2c/alliedvision,alvium.yaml | 115 +
drivers/media/i2c/Kconfig | 9 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/alvium.c | 3547 +++++++++++++++++
drivers/media/i2c/alvium.h | 500 +++
5 files changed, 4172 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/alliedvision,alvium.yaml
create mode 100644 drivers/media/i2c/alvium.c
create mode 100644 drivers/media/i2c/alvium.h

--
2.34.1

2023-05-26 18:10:36

by Tommaso Merciai

[permalink] [raw]

Subject: [PATCH v2 1/2] media: dt-bindings: alvium: add document YAML binding

Add documentation of device tree in YAML schema for the ALVIUM
Camera from Allied Vision Inc.

References:
- https://www.alliedvision.com/en/products/embedded-vision-solutions

Signed-off-by: Tommaso Merciai <[email protected]>
---
Changes since v1:
- Fixed build error as suggested by RHerring bot

.../media/i2c/alliedvision,alvium.yaml | 115 ++++++++++++++++++
1 file changed, 115 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/alliedvision,alvium.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium.yaml b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium.yaml
new file mode 100644
index 000000000000..81e9e560c99d
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium.yaml
@@ -0,0 +1,115 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/alliedvision,alvium.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Alliedvision Alvium Camera
+
+maintainers:
+ - Tommaso Merciai <[email protected]>
+ - Martin Hecht <[email protected]>
+
+allOf:
+ - $ref: /schemas/media/video-interface-devices.yaml#
+
+properties:
+ compatible:
+ const: alliedvision,alvium
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ description: XCLK Input Clock
+
+ clock-names:
+ const: xclk
+
+ powerdown-gpios:
+ maxItems: 1
+ description: >
+ Reference to the GPIO connected to the powerdown pin, if any.
+
+ reset-gpios:
+ maxItems: 1
+ description: >
+ Reference to the GPIO connected to the reset pin, if any.
+
+ streamon-delay:
+ maxItems: 1
+ description: >
+ Delay before camera start capturing frames in us.
+
+ rotation:
+ enum:
+ - 0
+ - 180
+
+ port:
+ description: Digital Output Port
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ additionalProperties: false
+
+ properties:
+ endpoint:
+ $ref: /schemas/media/video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ clock-lanes:
+ const: 0
+ data-lanes:
+ minItems: 1
+ maxItems: 4
+ link-frequencies: true
+
+ required:
+ - data-lanes
+ - link-frequencies
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - port
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/clock/imx8mp-clock.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ camera: alvium@3c {
+ compatible = "alliedvision,alvium";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_csi0_pwn>, <&pinctrl_csi0_rst>, <&pinctrl_csi_mclk>;
+ reg = <0x3c>;
+ clocks = <&clk IMX8MP_CLK_IPP_DO_CLKO2>;
+ clock-names = "xclk";
+ assigned-clocks = <&clk IMX8MP_CLK_IPP_DO_CLKO2>;
+ assigned-clock-parents = <&clk IMX8MP_CLK_24M>;
+ assigned-clock-rates = <24000000>;
+ streamon-delay = <20>;
+ powerdown-gpios = <&gpio2 11 GPIO_ACTIVE_HIGH>;
+ reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
+ status = "okay";
+
+ port {
+ alvium_out: endpoint {
+ remote-endpoint = <&mipi_csi_0_in>;
+ data-lanes = <1 2 3 4>;
+ link-frequencies = /bits/ 64 <681250000>;
+ clock-lanes = <0>;
+ };
+ };
+ };
+ };
+
+...
--
2.34.1

2023-05-26 19:14:50

Hi Tommaso,

On Wed, May 31, 2023 at 04:01:57PM +0200, Tommaso Merciai wrote:
> On Wed, May 31, 2023 at 02:06:00PM +0300, Laurent Pinchart wrote:
> > On Wed, May 31, 2023 at 12:20:47PM +0200, Tommaso Merciai wrote:
> > > On Mon, May 29, 2023 at 09:43:26AM +0300, Laurent Pinchart wrote:
> > > > On Mon, May 29, 2023 at 09:39:13AM +0300, Laurent Pinchart wrote:
> > > > > On Sun, May 28, 2023 at 09:16:05PM +0000, Sakari Ailus wrote:
> > > > > > On Fri, May 26, 2023 at 07:39:43PM +0200, Tommaso Merciai wrote:
> > > > > > > Add documentation of device tree in YAML schema for the ALVIUM
> > > > > > > Camera from Allied Vision Inc.
> > > > > > >
> > > > > > > References:
> > > > > > > - https://www.alliedvision.com/en/products/embedded-vision-solutions
> > > > > > >
> > > > > > > Signed-off-by: Tommaso Merciai <[email protected]>
> > > > > > > ---
> > > > > > > Changes since v1:
> > > > > > > - Fixed build error as suggested by RHerring bot
> > > > > > >
> > > > > > > .../media/i2c/alliedvision,alvium.yaml | 115 ++++++++++++++++++
> > > > > > > 1 file changed, 115 insertions(+)
> > > > > > > create mode 100644 Documentation/devicetree/bindings/media/i2c/alliedvision,alvium.yaml
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium.yaml b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium.yaml
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..81e9e560c99d
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium.yaml
> > > > > > > @@ -0,0 +1,115 @@
> > > > > > > +# SPDX-License-Identifier: GPL-2.0
> > > > > > > +%YAML 1.2
> > > > > > > +---
> > > > > > > +$id: http://devicetree.org/schemas/media/i2c/alliedvision,alvium.yaml#
> > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > +
> > > > > > > +title: Alliedvision Alvium Camera
> > > > >
> > > > > s/Alliedvision/Allied Vision/
> > > > >
> > > > > > > +
> > > > > > > +maintainers:
> > > > > > > + - Tommaso Merciai <[email protected]>
> > > > > > > + - Martin Hecht <[email protected]>
> > > > > > > +
> > > > > > > +allOf:
> > > > > > > + - $ref: /schemas/media/video-interface-devices.yaml#
> > > > > > > +
> > > > > > > +properties:
> > > > > > > + compatible:
> > > > > > > + const: alliedvision,alvium
> > > > >
> > > > > The name is very generic. There are Alvium camera modules that have a
> > > > > GMSL or FPD-Link interface, and I'm pretty sure those will require a
> > > > > different driver. I would add module-specific compatible strings (e.g.
> > > > > "alliedvision,alvium-1500c", ...) here, with a generic fallback.
> > > > > "alliedvision,alvium" isn't good as it won't cover GMSL or FPD-Link,
> > > > > maybe "alliedvision,alvium-csi2" would be an option.
> > > >
> > > > Actually, "alvium-1500c" as a specific compatible string won't do. You
> > > > need the exact model in the compatible string, otherwise it won't be
> > > > possible for the driver to handle device-specific configuration (for
> > > > instance accessing registers of the camera sensor for fine-grained
> > > > configuration). I would thus recommend using "alliedvision,alvium-1500c"
> > > > and "alliedvision,alvium-1800c" as generic fallbacks, along compatible
> > > > strings that include the exact device model.
> > >
> > > Agree with alliedvision,alvium-csi2 and thanks for your suggestion.
> > > In my opinion we don’t need names for 1500c and
> > > others because the same driver can drive all the alvium models.
> > > Alvium is taking care of different sensor abstractions.
> > >
> > > I test with this driver with the following models:
> > >
> > > - 1800 C-1240c
> > > - 1800 C-040c
> > > - 1500 C-500
> > >
> > > What do you think about?
> >
> > As far as I understand, the camera modules allow accessing sensors
> > registers from the host (through the ISP) for fine-grained
> > configuration. Even if that's not implemented in the driver at the
> > moment, I think it's an important feature to eventually support, and it
> > will require a way for the system to identify the camera module
> > precisely, to know which sensor the module uses. That's why I would like
> > that information to be available in DT, in the form of a compatible
> > string. For instance,
> >
> > compatible = "alliedvision,alvium-1500c-1240c",
> > "alliedvision,alvium-1500c";
> >
> > The driver will only need DT match entries for
> > "alliedvision,alvium-1500c" and "alliedvision,alvium-1800c".
>
> Thanks for your feedback.
>
> Some clarification:
>
> The Alvium doesn't allow direct access on the sensor.
> Alvium hides the particular sensor totally.

I think I got misled by the "Direct Register Access (DRA) to control the
cameras via registers for advanced users" mention on the Allied Vision
website, and thought direct register access to the sensor was possible.

> For this reason I would prefer to keep generic
> "alvium-csi2" name.

Given that the camera has device identification registers, that should
be OK.

> > > > > > > +
> > > > > > > + reg:
> > > > > > > + maxItems: 1
> > > > > > > +
> > > > > > > + clocks:
> > > > > > > + description: XCLK Input Clock
> > > > > > > +
> > > > > > > + clock-names:
> > > > > > > + const: xclk
> > > > > >
> > > > > > I'd also drop this as you have a single clock only: it's redundant.
> > > > > >
> > > > > > > +
> > > > > > > + powerdown-gpios:
> > > > > > > + maxItems: 1
> > > > > > > + description: >
> > > > > > > + Reference to the GPIO connected to the powerdown pin, if any.
> > > > > > > +
> > > > > > > + reset-gpios:
> > > > > > > + maxItems: 1
> > > > > > > + description: >
> > > > > > > + Reference to the GPIO connected to the reset pin, if any.
> > > > >
> > > > > Reading the Alvium CSI-2 Cameras User Guide, I don't see any powerdown
> > > > > or reset pin on the 22-pin connector. Am I missing something ? There are
> > > > > however two GPIOs (in addition to the I2C signals that are also
> > > > > documented as GPIOs), do you plan to support those ?
> > > > >
> > > > > > > +
> > > > > > > + streamon-delay:
> > > > > > > + maxItems: 1
> > > > > > > + description: >
> > > > > > > + Delay before camera start capturing frames in us.
> > > > >
> > > > > Add "-us" to the property name to indicate the unit.
> > > > >
> > > > > This is a vendor-specific property, and should thus have a vendor
> > > > > prefix.
> > > > >
> > > > > A longer description is needed, from that single line I have no idea
> > > > > what the property does exactly.
> > > > >
> > > > > > > +
> > > > > > > + rotation:
> > > > > > > + enum:
> > > > > > > + - 0
> > > > > > > + - 180
> > > > >
> > > > > Why is the rotation restricted to 0 or 180 ? Someone could mount the
> > > > > module with 90 degrees rotation, shouldn't the DT bindings allow
> > > > > describing that ?
> > > > >
> > > > > You need a property for the vcc-ext-in supply.
> > > > >
> > > > > > > +
> > > > > > > + port:
> > > > > > > + description: Digital Output Port
> > > > > > > + $ref: /schemas/graph.yaml#/$defs/port-base
> > > > > > > + additionalProperties: false
> > > > > > > +
> > > > > > > + properties:
> > > > > > > + endpoint:
> > > > > > > + $ref: /schemas/media/video-interfaces.yaml#
> > > > > > > + unevaluatedProperties: false
> > > > > > > +
> > > > > > > + properties:
> > > > > > > + clock-lanes:
> > > > > > > + const: 0
> > > > > >
> > > > > > The driver can know this, no need to have it in DT, i.e. please drop it.
> > > > > >
> > > > > > > + data-lanes:
> > > > > > > + minItems: 1
> > > > > > > + maxItems: 4
> > > > > > > + link-frequencies: true
> > > > > > > +
> > > > > > > + required:
> > > > > > > + - data-lanes
> > > > > > > + - link-frequencies
> > > > > > > +
> > > > > > > +required:
> > > > > > > + - compatible
> > > > > > > + - reg
> > > > > > > + - clocks
> > > > > > > + - clock-names
> > > > > > > + - port
> > > > > > > +
> > > > > > > +additionalProperties: false
> > > > > > > +
> > > > > > > +examples:
> > > > > > > + - |
> > > > > > > + #include <dt-bindings/gpio/gpio.h>
> > > > > > > + #include <dt-bindings/clock/imx8mp-clock.h>
> > > > > > > +
> > > > > > > + i2c {
> > > > > > > + #address-cells = <1>;
> > > > > > > + #size-cells = <0>;
> > > > > > > +
> > > > > > > + camera: alvium@3c {
> > > > > > > + compatible = "alliedvision,alvium";
> > > > >
> > > > > The "alliedvision" prefix is missing from
> > > > > Documentation/devicetree/bindings/vendor-prefixes.yaml.
> > > > >
> > > > > > > + pinctrl-names = "default";
> > > > > > > + pinctrl-0 = <&pinctrl_csi0_pwn>, <&pinctrl_csi0_rst>, <&pinctrl_csi_mclk>;
> > > > >
> > > > > I'd drop pinctrl, it makes the example longer without adding much value.
> > > > >
> > > > > > > + reg = <0x3c>;
> > > > > > > + clocks = <&clk IMX8MP_CLK_IPP_DO_CLKO2>;
> > > > > > > + clock-names = "xclk";
> > > > > > > + assigned-clocks = <&clk IMX8MP_CLK_IPP_DO_CLKO2>;
> > > > > > > + assigned-clock-parents = <&clk IMX8MP_CLK_24M>;
> > > > > > > + assigned-clock-rates = <24000000>;
> > > > > > > + streamon-delay = <20>;
> > > > > > > + powerdown-gpios = <&gpio2 11 GPIO_ACTIVE_HIGH>;
> > > > > > > + reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
> > > > > > > + status = "okay";
> > > > > > > +
> > > > > > > + port {
> > > > > > > + alvium_out: endpoint {
> > > > > > > + remote-endpoint = <&mipi_csi_0_in>;
> > > > > > > + data-lanes = <1 2 3 4>;
> > > > > > > + link-frequencies = /bits/ 64 <681250000>;
> > > > > > > + clock-lanes = <0>;
> > > > > > > + };
> > > > > > > + };
> > > > > > > + };
> > > > > > > + };
> > > > > > > +
> > > > > > > +...

--
Regards,

Laurent Pinchart