[PATCH 0/5] media: Sony IMX335 improvements

[PATCH 0/5] media: Sony IMX335 improvements

[Kieran Bingham]

The Sony IMX335 is not yet compatible with libcamera, as it is missing
the get selection API call.

It also misses a way to describe how to power on the sensor.

Now that I've got this camera functioning on Debix-SOM and Pi5, I expect
to be able to do quite a bit more cleanup to the code here. But these
patches should already be valid for consideration.


Kieran Bingham (5):
  media: dt-bindings: media: imx335: Add supply bindings
  media: i2c: imx335: Enable regulator supplies
  media: i2c: imx335: Implement get selection API
  media: i2c: imx335: Fix hblank min/max values
  media: i2c: imx335: Improve configuration error reporting

 .../bindings/media/i2c/sony,imx335.yaml       | 16 ++++
 drivers/media/i2c/imx335.c                    | 95 ++++++++++++++++++-
 2 files changed, 106 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH 1/5] media: dt-bindings: media: imx335: Add supply bindings

[Kieran Bingham]

Add the bindings for the supply references used on the IMX335.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 .../bindings/media/i2c/sony,imx335.yaml          | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
index a167dcdb3a32..1863b5608a5c 100644
--- a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
@@ -32,6 +32,15 @@ properties:
     description: Clock frequency from 6 to 27 MHz, 37.125MHz, 74.25MHz
     maxItems: 1
 
+  avdd-supply:
+    description: Analog power supply (2.9V)
+
+  ovdd-supply:
+    description: Interface power supply (1.8V)
+
+  dvdd-supply:
+    description: Digital power supply (1.2V)
+
   reset-gpios:
     description: Reference to the GPIO connected to the XCLR pin, if any.
     maxItems: 1
@@ -60,6 +69,9 @@ required:
   - compatible
   - reg
   - clocks
+  - avdd-supply
+  - ovdd-supply
+  - dvdd-supply
   - port
 
 additionalProperties: false
@@ -79,6 +91,10 @@ examples:
             assigned-clock-parents = <&imx335_clk_parent>;
             assigned-clock-rates = <24000000>;
 
+            avdd-supply = <&camera_vdda_2v9>;
+            ovdd-supply = <&camera_vddo_1v8>;
+            dvdd-supply = <&camera_vddd_1v2>;
+
             port {
                 imx335: endpoint {
                     remote-endpoint = <&cam>;
-- 
2.34.1

Re: [PATCH 1/5] media: dt-bindings: media: imx335: Add supply bindings

[Umang Jain]

Hi Kieran,

Thank you for the patch

On 10/10/23 6:21 AM, Kieran Bingham wrote:
> Add the bindings for the supply references used on the IMX335.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

LGTM,

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
>   .../bindings/media/i2c/sony,imx335.yaml          | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> index a167dcdb3a32..1863b5608a5c 100644
> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> @@ -32,6 +32,15 @@ properties:
>       description: Clock frequency from 6 to 27 MHz, 37.125MHz, 74.25MHz
>       maxItems: 1
>   
> +  avdd-supply:
> +    description: Analog power supply (2.9V)
> +
> +  ovdd-supply:
> +    description: Interface power supply (1.8V)
> +
> +  dvdd-supply:
> +    description: Digital power supply (1.2V)
> +
>     reset-gpios:
>       description: Reference to the GPIO connected to the XCLR pin, if any.
>       maxItems: 1
> @@ -60,6 +69,9 @@ required:
>     - compatible
>     - reg
>     - clocks
> +  - avdd-supply
> +  - ovdd-supply
> +  - dvdd-supply
>     - port
>   
>   additionalProperties: false
> @@ -79,6 +91,10 @@ examples:
>               assigned-clock-parents = <&imx335_clk_parent>;
>               assigned-clock-rates = <24000000>;
>   
> +            avdd-supply = <&camera_vdda_2v9>;
> +            ovdd-supply = <&camera_vddo_1v8>;
> +            dvdd-supply = <&camera_vddd_1v2>;
> +
>               port {
>                   imx335: endpoint {
>                       remote-endpoint = <&cam>;

Re: [PATCH 1/5] media: dt-bindings: media: imx335: Add supply bindings

[Marco Felsch]

On 23-10-10, Kieran Bingham wrote:
> Add the bindings for the supply references used on the IMX335.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

> ---
>  .../bindings/media/i2c/sony,imx335.yaml          | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> index a167dcdb3a32..1863b5608a5c 100644
> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> @@ -32,6 +32,15 @@ properties:
>      description: Clock frequency from 6 to 27 MHz, 37.125MHz, 74.25MHz
>      maxItems: 1
>  
> +  avdd-supply:
> +    description: Analog power supply (2.9V)
> +
> +  ovdd-supply:
> +    description: Interface power supply (1.8V)
> +
> +  dvdd-supply:
> +    description: Digital power supply (1.2V)
> +
>    reset-gpios:
>      description: Reference to the GPIO connected to the XCLR pin, if any.
>      maxItems: 1
> @@ -60,6 +69,9 @@ required:
>    - compatible
>    - reg
>    - clocks
> +  - avdd-supply
> +  - ovdd-supply
> +  - dvdd-supply
>    - port
>  
>  additionalProperties: false
> @@ -79,6 +91,10 @@ examples:
>              assigned-clock-parents = <&imx335_clk_parent>;
>              assigned-clock-rates = <24000000>;
>  
> +            avdd-supply = <&camera_vdda_2v9>;
> +            ovdd-supply = <&camera_vddo_1v8>;
> +            dvdd-supply = <&camera_vddd_1v2>;
> +
>              port {
>                  imx335: endpoint {
>                      remote-endpoint = <&cam>;
> -- 
> 2.34.1
> 
> 
> 

Re: [PATCH 1/5] media: dt-bindings: media: imx335: Add supply bindings

[Sakari Ailus]

Hi Kieran,

On Tue, Oct 10, 2023 at 01:51:22AM +0100, Kieran Bingham wrote:
> Add the bindings for the supply references used on the IMX335.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  .../bindings/media/i2c/sony,imx335.yaml          | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> index a167dcdb3a32..1863b5608a5c 100644
> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> @@ -32,6 +32,15 @@ properties:
>      description: Clock frequency from 6 to 27 MHz, 37.125MHz, 74.25MHz
>      maxItems: 1
>  
> +  avdd-supply:
> +    description: Analog power supply (2.9V)
> +
> +  ovdd-supply:
> +    description: Interface power supply (1.8V)
> +
> +  dvdd-supply:
> +    description: Digital power supply (1.2V)

I wonder what's the policy in this case --- some of the regulators are
often hard-wired and the bindings didn't have them previously either (I
wonder why, maybe they were all hard wired in the board??).

Could they be optional? The driver will need to be able to do without these
in any case.

> +
>    reset-gpios:
>      description: Reference to the GPIO connected to the XCLR pin, if any.
>      maxItems: 1
> @@ -60,6 +69,9 @@ required:
>    - compatible
>    - reg
>    - clocks
> +  - avdd-supply
> +  - ovdd-supply
> +  - dvdd-supply
>    - port
>  
>  additionalProperties: false
> @@ -79,6 +91,10 @@ examples:
>              assigned-clock-parents = <&imx335_clk_parent>;
>              assigned-clock-rates = <24000000>;
>  
> +            avdd-supply = <&camera_vdda_2v9>;
> +            ovdd-supply = <&camera_vddo_1v8>;
> +            dvdd-supply = <&camera_vddd_1v2>;
> +
>              port {
>                  imx335: endpoint {
>                      remote-endpoint = <&cam>;

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/5] media: dt-bindings: media: imx335: Add supply bindings

[Kieran Bingham]

Hi Sakari,

Quoting Sakari Ailus (2023-10-10 07:06:26)
> Hi Kieran,
> 
> On Tue, Oct 10, 2023 at 01:51:22AM +0100, Kieran Bingham wrote:
> > Add the bindings for the supply references used on the IMX335.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  .../bindings/media/i2c/sony,imx335.yaml          | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> > index a167dcdb3a32..1863b5608a5c 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> > @@ -32,6 +32,15 @@ properties:
> >      description: Clock frequency from 6 to 27 MHz, 37.125MHz, 74.25MHz
> >      maxItems: 1
> >  
> > +  avdd-supply:
> > +    description: Analog power supply (2.9V)
> > +
> > +  ovdd-supply:
> > +    description: Interface power supply (1.8V)
> > +
> > +  dvdd-supply:
> > +    description: Digital power supply (1.2V)
> 
> I wonder what's the policy in this case --- some of the regulators are
> often hard-wired and the bindings didn't have them previously either (I
> wonder why, maybe they were all hard wired in the board??).
> 
> Could they be optional? The driver will need to be able to do without these
> in any case.

Indeed - many devices do not need to define how they are powered up.

But Krzysztof stated that supplies should be required by the bindings on
my recent posting for a VCM driver:

 - https://lore.kernel.org/all/6e163f4d-061d-3c20-4c2e-44c74d529f10@linaro.org/

So based on that I have made these 'required'.

Even in my case here, with a camera module that is compatible with the
Raspberry Pi camera connector - there isn't really 3 supplies. It's just
a single gpio enable pin to bring this device up for me. Of course
that's specific to the module not the sensor.


> > +
> >    reset-gpios:
> >      description: Reference to the GPIO connected to the XCLR pin, if any.
> >      maxItems: 1
> > @@ -60,6 +69,9 @@ required:
> >    - compatible
> >    - reg
> >    - clocks
> > +  - avdd-supply
> > +  - ovdd-supply
> > +  - dvdd-supply
> >    - port
> >  
> >  additionalProperties: false
> > @@ -79,6 +91,10 @@ examples:
> >              assigned-clock-parents = <&imx335_clk_parent>;
> >              assigned-clock-rates = <24000000>;
> >  
> > +            avdd-supply = <&camera_vdda_2v9>;
> > +            ovdd-supply = <&camera_vddo_1v8>;
> > +            dvdd-supply = <&camera_vddd_1v2>;
> > +
> >              port {
> >                  imx335: endpoint {
> >                      remote-endpoint = <&cam>;
> 
> -- 
> Kind regards,
> 
> Sakari Ailus

Re: [PATCH 1/5] media: dt-bindings: media: imx335: Add supply bindings

[Sakari Ailus]

Hi Kieran,

On Tue, Oct 10, 2023 at 02:25:09PM +0100, Kieran Bingham wrote:
> Hi Sakari,
> 
> Quoting Sakari Ailus (2023-10-10 07:06:26)
> > Hi Kieran,
> > 
> > On Tue, Oct 10, 2023 at 01:51:22AM +0100, Kieran Bingham wrote:
> > > Add the bindings for the supply references used on the IMX335.
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  .../bindings/media/i2c/sony,imx335.yaml          | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> > > index a167dcdb3a32..1863b5608a5c 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> > > @@ -32,6 +32,15 @@ properties:
> > >      description: Clock frequency from 6 to 27 MHz, 37.125MHz, 74.25MHz
> > >      maxItems: 1
> > >  
> > > +  avdd-supply:
> > > +    description: Analog power supply (2.9V)
> > > +
> > > +  ovdd-supply:
> > > +    description: Interface power supply (1.8V)
> > > +
> > > +  dvdd-supply:
> > > +    description: Digital power supply (1.2V)
> > 
> > I wonder what's the policy in this case --- some of the regulators are
> > often hard-wired and the bindings didn't have them previously either (I
> > wonder why, maybe they were all hard wired in the board??).
> > 
> > Could they be optional? The driver will need to be able to do without these
> > in any case.
> 
> Indeed - many devices do not need to define how they are powered up.
> 
> But Krzysztof stated that supplies should be required by the bindings on
> my recent posting for a VCM driver:
> 
>  - https://lore.kernel.org/all/6e163f4d-061d-3c20-4c2e-44c74d529f10@linaro.org/
> 
> So based on that I have made these 'required'.

I guess it's good to align bindings regarding this, in practice the driver
will need to work without regulators (or with dummies), too.

> 
> Even in my case here, with a camera module that is compatible with the
> Raspberry Pi camera connector - there isn't really 3 supplies. It's just
> a single gpio enable pin to bring this device up for me. Of course
> that's specific to the module not the sensor.

How do you declare that in DT? One of the regulators will be a GPIO one?

-- 
Regards,

Sakari Ailus

Re: [PATCH 1/5] media: dt-bindings: media: imx335: Add supply bindings

[Kieran Bingham]

Quoting Sakari Ailus (2023-10-11 12:01:23)
> Hi Kieran,
> 
> On Tue, Oct 10, 2023 at 02:25:09PM +0100, Kieran Bingham wrote:
> > Hi Sakari,
> > 
> > Quoting Sakari Ailus (2023-10-10 07:06:26)
> > > Hi Kieran,
> > > 
> > > On Tue, Oct 10, 2023 at 01:51:22AM +0100, Kieran Bingham wrote:
> > > > Add the bindings for the supply references used on the IMX335.
> > > > 
> > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > ---
> > > >  .../bindings/media/i2c/sony,imx335.yaml          | 16 ++++++++++++++++
> > > >  1 file changed, 16 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> > > > index a167dcdb3a32..1863b5608a5c 100644
> > > > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> > > > @@ -32,6 +32,15 @@ properties:
> > > >      description: Clock frequency from 6 to 27 MHz, 37.125MHz, 74.25MHz
> > > >      maxItems: 1
> > > >  
> > > > +  avdd-supply:
> > > > +    description: Analog power supply (2.9V)
> > > > +
> > > > +  ovdd-supply:
> > > > +    description: Interface power supply (1.8V)
> > > > +
> > > > +  dvdd-supply:
> > > > +    description: Digital power supply (1.2V)
> > > 
> > > I wonder what's the policy in this case --- some of the regulators are
> > > often hard-wired and the bindings didn't have them previously either (I
> > > wonder why, maybe they were all hard wired in the board??).
> > > 
> > > Could they be optional? The driver will need to be able to do without these
> > > in any case.
> > 
> > Indeed - many devices do not need to define how they are powered up.
> > 
> > But Krzysztof stated that supplies should be required by the bindings on
> > my recent posting for a VCM driver:
> > 
> >  - https://lore.kernel.org/all/6e163f4d-061d-3c20-4c2e-44c74d529f10@linaro.org/
> > 
> > So based on that I have made these 'required'.
> 
> I guess it's good to align bindings regarding this, in practice the driver
> will need to work without regulators (or with dummies), too.
> 
> > 
> > Even in my case here, with a camera module that is compatible with the
> > Raspberry Pi camera connector - there isn't really 3 supplies. It's just
> > a single gpio enable pin to bring this device up for me. Of course
> > that's specific to the module not the sensor.
> 
> How do you declare that in DT? One of the regulators will be a GPIO one?

I have the following as an imx335.dtsi which I include.
It /should/ be an overlay / dtbo - but the current bootloader on the
baord I have doesn't support applying overlays - so I just include it
directly for now.


```
/ {
	/* 24 MHz Crystal on the camera module */
	imx335_inclk_1: imx335_inclk_24m {
		compatible = "fixed-clock";
		#clock-cells = <0>;
		status = "okay";
		clock-frequency = <24000000>;
	};

	reg_imx335_1_3v3: regulator-imx335_1-vdd3v3 {
		compatible = "regulator-fixed";
		pinctrl-names = "default";
		regulator-min-microvolt = <3300000>;
		regulator-max-microvolt = <3300000>;
		regulator-name = "IMX335_1_POWER_EN";
		gpio = <&expander0 4 GPIO_ACTIVE_HIGH>;
		vin-supply = <&reg_csi2_3v3>;
		startup-delay-us = <300000>;
		enable-active-high;
	};
};

&i2c3 {
	imx335_0: sensor@1a {
		compatible = "sony,imx335";
		reg = <0x1a>;

		clocks = <&imx335_inclk_1>;
		clock-names = "xclk";

		rotation = <180>;
		orientation = <0>;

		status = "okay";

		/* The IMX335 module uses *only* the 3v3 line */
		avdd-supply = <&reg_imx335_1_3v3>;
		ovdd-supply = <&reg_imx335_1_3v3>;
		dvdd-supply = <&reg_imx335_1_3v3>;

		port {
			sensor_1_out: endpoint {
				remote-endpoint = <&mipi_csi_1_in>;
				clock-lanes = <0>;
				data-lanes = <1 2 3 4>;
				link-frequencies = /bits/ 64 <594000000>;
			};
		};
	};
};

&mipi_csi_1 {
	status = "okay";

	ports {
		port@0 {
			mipi_csi_1_in: endpoint {
				remote-endpoint = <&sensor_1_out>;
				clock-lanes = <0>;
				data-lanes = <1 2 3 4>;
			};
		};
	};
};

```

We could argue that the reg_imx335_1_3v3, should be 3 separate
regulators each targetting vin-supply = <&reg_csi2_3v3>;

But they are all wired up to the same enable pin, and I think they would
then fail to probe if they all tried to control that gpio - while a
regulator-fixed can be shared and handles this for us.

The gpio at:

 &reg_imx335_1_3v3 {
 	gpio = <&expander0 4 GPIO_ACTIVE_HIGH>;
 };

connects to the enable line of all three regulators on the camera
module.


In fact - looking at the schematics of the camera module - they all
power up at 'the same time'. There are no hardware delays introduced on
this module, so that might answer the regulator-bulk question on the
driver.
--
Kieran



> 
> -- 
> Regards,
> 
> Sakari Ailus

Re: [PATCH 1/5] media: dt-bindings: media: imx335: Add supply bindings

[Rob Herring]

On Tue, Oct 10, 2023 at 01:51:22AM +0100, Kieran Bingham wrote:
> Add the bindings for the supply references used on the IMX335.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  .../bindings/media/i2c/sony,imx335.yaml          | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> index a167dcdb3a32..1863b5608a5c 100644
> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> @@ -32,6 +32,15 @@ properties:
>      description: Clock frequency from 6 to 27 MHz, 37.125MHz, 74.25MHz
>      maxItems: 1
>  
> +  avdd-supply:
> +    description: Analog power supply (2.9V)
> +
> +  ovdd-supply:
> +    description: Interface power supply (1.8V)
> +
> +  dvdd-supply:
> +    description: Digital power supply (1.2V)
> +
>    reset-gpios:
>      description: Reference to the GPIO connected to the XCLR pin, if any.
>      maxItems: 1
> @@ -60,6 +69,9 @@ required:
>    - compatible
>    - reg
>    - clocks
> +  - avdd-supply
> +  - ovdd-supply
> +  - dvdd-supply

New required properties are an ABI break. That's fine only if you can 
explain no one is using this binding.

>    - port
>  
>  additionalProperties: false
> @@ -79,6 +91,10 @@ examples:
>              assigned-clock-parents = <&imx335_clk_parent>;
>              assigned-clock-rates = <24000000>;
>  
> +            avdd-supply = <&camera_vdda_2v9>;
> +            ovdd-supply = <&camera_vddo_1v8>;
> +            dvdd-supply = <&camera_vddd_1v2>;
> +
>              port {
>                  imx335: endpoint {
>                      remote-endpoint = <&cam>;
> -- 
> 2.34.1
> 

Re: [PATCH 1/5] media: dt-bindings: media: imx335: Add supply bindings

[Kieran Bingham]

Quoting Rob Herring (2023-10-10 18:09:41)
> On Tue, Oct 10, 2023 at 01:51:22AM +0100, Kieran Bingham wrote:
> > Add the bindings for the supply references used on the IMX335.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  .../bindings/media/i2c/sony,imx335.yaml          | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> > index a167dcdb3a32..1863b5608a5c 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> > @@ -32,6 +32,15 @@ properties:
> >      description: Clock frequency from 6 to 27 MHz, 37.125MHz, 74.25MHz
> >      maxItems: 1
> >  
> > +  avdd-supply:
> > +    description: Analog power supply (2.9V)
> > +
> > +  ovdd-supply:
> > +    description: Interface power supply (1.8V)
> > +
> > +  dvdd-supply:
> > +    description: Digital power supply (1.2V)
> > +
> >    reset-gpios:
> >      description: Reference to the GPIO connected to the XCLR pin, if any.
> >      maxItems: 1
> > @@ -60,6 +69,9 @@ required:
> >    - compatible
> >    - reg
> >    - clocks
> > +  - avdd-supply
> > +  - ovdd-supply
> > +  - dvdd-supply
> 
> New required properties are an ABI break. That's fine only if you can 
> explain no one is using this binding.

I made these required due to a previous review comment on another
driver:

- https://lore.kernel.org/all/6e163f4d-061d-3c20-4c2e-44c74d529f10@linaro.org/

I hadn't thought about the ABI break though.

So to clarify (for me):
 - New bindings should always add -supply's as required.
 - Adding -supply to existing bindings should be optional.

I guess that leaves a mix of devices that either are required or may be
optional - but perhaps that can't be helped if the bindings have already
got in.

The IMX335 driver was added in 45d19b5fb9ae ("media: i2c: Add imx335
camera sensor driver"), and the bindings in 932741d451a5 ("media:
dt-bindings: media: Add bindings for imx335") by Martina, who looks to
be an Intel employee - so I suspect this is used through ACPI so far and
not device tree.

Danielle, get_maintainer tells me you are looking after this device -
can you confirm this ?

--
Kieran


> 
> >    - port
> >  
> >  additionalProperties: false
> > @@ -79,6 +91,10 @@ examples:
> >              assigned-clock-parents = <&imx335_clk_parent>;
> >              assigned-clock-rates = <24000000>;
> >  
> > +            avdd-supply = <&camera_vdda_2v9>;
> > +            ovdd-supply = <&camera_vddo_1v8>;
> > +            dvdd-supply = <&camera_vddd_1v2>;
> > +
> >              port {
> >                  imx335: endpoint {
> >                      remote-endpoint = <&cam>;
> > -- 
> > 2.34.1
> >

Re: [PATCH 1/5] media: dt-bindings: media: imx335: Add supply bindings

[Kieran Bingham]

Hi Rob, Krzysztof,

Quoting Kieran Bingham (2023-10-11 10:51:08)
> Quoting Rob Herring (2023-10-10 18:09:41)
> > On Tue, Oct 10, 2023 at 01:51:22AM +0100, Kieran Bingham wrote:
> > > Add the bindings for the supply references used on the IMX335.
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  .../bindings/media/i2c/sony,imx335.yaml          | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> > > index a167dcdb3a32..1863b5608a5c 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml
> > > @@ -32,6 +32,15 @@ properties:
> > >      description: Clock frequency from 6 to 27 MHz, 37.125MHz, 74.25MHz
> > >      maxItems: 1
> > >  
> > > +  avdd-supply:
> > > +    description: Analog power supply (2.9V)
> > > +
> > > +  ovdd-supply:
> > > +    description: Interface power supply (1.8V)
> > > +
> > > +  dvdd-supply:
> > > +    description: Digital power supply (1.2V)
> > > +
> > >    reset-gpios:
> > >      description: Reference to the GPIO connected to the XCLR pin, if any.
> > >      maxItems: 1
> > > @@ -60,6 +69,9 @@ required:
> > >    - compatible
> > >    - reg
> > >    - clocks
> > > +  - avdd-supply
> > > +  - ovdd-supply
> > > +  - dvdd-supply
> > 
> > New required properties are an ABI break. That's fine only if you can 
> > explain no one is using this binding.
> 

No one is using this /in-kernel-tree/.

This could be because the original support for IMX335 was added with
ACPI devices in mind, but even for device-tree, that's not surprising as
cameras may often be described in overlays, unless embedded in specific
products.

I'm trying to revise this series for a v2. Could I get a decision from
the DT maintainers on which direction I should take this please?

Would you prefer supplies to be 'required' (if supplies should always be
required) - or should I leave this as optional as the binding has
previously been accepted?




> I made these required due to a previous review comment on another
> driver:
> 
> - https://lore.kernel.org/all/6e163f4d-061d-3c20-4c2e-44c74d529f10@linaro.org/
> 
> I hadn't thought about the ABI break though.
> 
> So to clarify (for me):
>  - New bindings should always add -supply's as required.
>  - Adding -supply to existing bindings should be optional.
> 
> I guess that leaves a mix of devices that either are required or may be
> optional - but perhaps that can't be helped if the bindings have already
> got in.
> 
> The IMX335 driver was added in 45d19b5fb9ae ("media: i2c: Add imx335
> camera sensor driver"), and the bindings in 932741d451a5 ("media:
> dt-bindings: media: Add bindings for imx335") by Martina, who looks to
> be an Intel employee - so I suspect this is used through ACPI so far and
> not device tree.
> 
> Danielle, get_maintainer tells me you are looking after this device -
> can you confirm this ?
> 
> --
> Kieran
> 
> 
> > 
> > >    - port
> > >  
> > >  additionalProperties: false
> > > @@ -79,6 +91,10 @@ examples:
> > >              assigned-clock-parents = <&imx335_clk_parent>;
> > >              assigned-clock-rates = <24000000>;
> > >  
> > > +            avdd-supply = <&camera_vdda_2v9>;
> > > +            ovdd-supply = <&camera_vddo_1v8>;
> > > +            dvdd-supply = <&camera_vddd_1v2>;
> > > +
> > >              port {
> > >                  imx335: endpoint {
> > >                      remote-endpoint = <&cam>;
> > > -- 
> > > 2.34.1
> > >

[PATCH 2/5] media: i2c: imx335: Enable regulator supplies

[Kieran Bingham]

Provide support for enabling and disabling regulator supplies to control
power to the camera sensor.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 41 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index ec729126274b..bf12b9b69fce 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -75,6 +75,19 @@ struct imx335_reg_list {
 	const struct imx335_reg *regs;
 };
 
+static const char * const imx335_supply_name[] = {
+	/*
+	 * Turn on the power supplies so that they rise in order of
+	 *           1.2v,-> 1.8 -> 2.9v
+	 * All power supplies should finish rising within 200ms.
+	 */
+	"avdd", /* Analog (2.9V) supply */
+	"ovdd", /* Digital I/O (1.8V) supply */
+	"dvdd", /* Digital Core (1.2V) supply */
+};
+
+#define IMX335_NUM_SUPPLIES ARRAY_SIZE(imx335_supply_name)
+
 /**
  * struct imx335_mode - imx335 sensor mode structure
  * @width: Frame width
@@ -126,6 +139,8 @@ struct imx335 {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 	struct gpio_desc *reset_gpio;
+	struct regulator_bulk_data supplies[IMX335_NUM_SUPPLIES];
+
 	struct clk *inclk;
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_ctrl *link_freq_ctrl;
@@ -781,6 +796,17 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
 		return PTR_ERR(imx335->reset_gpio);
 	}
 
+	for (i = 0; i < IMX335_NUM_SUPPLIES; i++)
+		imx335->supplies[i].supply = imx335_supply_name[i];
+
+	ret = devm_regulator_bulk_get(imx335->dev,
+				      IMX335_NUM_SUPPLIES,
+				      imx335->supplies);
+	if (ret) {
+		dev_err(imx335->dev, "Failed to get regulators\n");
+		return ret;
+	}
+
 	/* Get sensor input clock */
 	imx335->inclk = devm_clk_get(imx335->dev, NULL);
 	if (IS_ERR(imx335->inclk)) {
@@ -859,6 +885,17 @@ static int imx335_power_on(struct device *dev)
 	struct imx335 *imx335 = to_imx335(sd);
 	int ret;
 
+	ret = regulator_bulk_enable(IMX335_NUM_SUPPLIES,
+				    imx335->supplies);
+	if (ret) {
+		dev_err(dev, "%s: failed to enable regulators\n",
+			__func__);
+		return ret;
+	}
+
+	usleep_range(500, 550); /* Tlow */
+
+	/* Set XCLR */
 	gpiod_set_value_cansleep(imx335->reset_gpio, 1);
 
 	ret = clk_prepare_enable(imx335->inclk);
@@ -867,7 +904,7 @@ static int imx335_power_on(struct device *dev)
 		goto error_reset;
 	}
 
-	usleep_range(20, 22);
+	usleep_range(20, 22); /* T4 */
 
 	return 0;
 
@@ -889,8 +926,8 @@ static int imx335_power_off(struct device *dev)
 	struct imx335 *imx335 = to_imx335(sd);
 
 	gpiod_set_value_cansleep(imx335->reset_gpio, 0);
-
 	clk_disable_unprepare(imx335->inclk);
+	regulator_bulk_disable(IMX335_NUM_SUPPLIES, imx335->supplies);
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH 2/5] media: i2c: imx335: Enable regulator supplies

[Umang Jain]

Hi Kieran,

Thank you for the patch.

On 10/10/23 6:21 AM, Kieran Bingham wrote:
> Provide support for enabling and disabling regulator supplies to control
> power to the camera sensor.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>   drivers/media/i2c/imx335.c | 41 ++++++++++++++++++++++++++++++++++++--
>   1 file changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index ec729126274b..bf12b9b69fce 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -75,6 +75,19 @@ struct imx335_reg_list {
>   	const struct imx335_reg *regs;
>   };
>   
> +static const char * const imx335_supply_name[] = {
> +	/*
> +	 * Turn on the power supplies so that they rise in order of
> +	 *           1.2v,-> 1.8 -> 2.9v
> +	 * All power supplies should finish rising within 200ms.
> +	 */
> +	"avdd", /* Analog (2.9V) supply */
> +	"ovdd", /* Digital I/O (1.8V) supply */
> +	"dvdd", /* Digital Core (1.2V) supply */
> +};
> +
> +#define IMX335_NUM_SUPPLIES ARRAY_SIZE(imx335_supply_name)
> +
>   /**
>    * struct imx335_mode - imx335 sensor mode structure
>    * @width: Frame width
> @@ -126,6 +139,8 @@ struct imx335 {
>   	struct v4l2_subdev sd;
>   	struct media_pad pad;
>   	struct gpio_desc *reset_gpio;
> +	struct regulator_bulk_data supplies[IMX335_NUM_SUPPLIES];
> +
>   	struct clk *inclk;
>   	struct v4l2_ctrl_handler ctrl_handler;
>   	struct v4l2_ctrl *link_freq_ctrl;
> @@ -781,6 +796,17 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
>   		return PTR_ERR(imx335->reset_gpio);
>   	}
>   
> +	for (i = 0; i < IMX335_NUM_SUPPLIES; i++)
> +		imx335->supplies[i].supply = imx335_supply_name[i];
> +
> +	ret = devm_regulator_bulk_get(imx335->dev,
> +				      IMX335_NUM_SUPPLIES,
> +				      imx335->supplies);

Shouldn't this go directly in  probe() function ? (Taking IMX219 driver 
as a reference..)
> +	if (ret) {
> +		dev_err(imx335->dev, "Failed to get regulators\n");
> +		return ret;
> +	}
> +
>   	/* Get sensor input clock */
>   	imx335->inclk = devm_clk_get(imx335->dev, NULL);
>   	if (IS_ERR(imx335->inclk)) {
> @@ -859,6 +885,17 @@ static int imx335_power_on(struct device *dev)
>   	struct imx335 *imx335 = to_imx335(sd);
>   	int ret;
>   
> +	ret = regulator_bulk_enable(IMX335_NUM_SUPPLIES,
> +				    imx335->supplies);
> +	if (ret) {
> +		dev_err(dev, "%s: failed to enable regulators\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	usleep_range(500, 550); /* Tlow */
> +
> +	/* Set XCLR */
>   	gpiod_set_value_cansleep(imx335->reset_gpio, 1);
>   
>   	ret = clk_prepare_enable(imx335->inclk);
> @@ -867,7 +904,7 @@ static int imx335_power_on(struct device *dev)
>   		goto error_reset;
>   	}
>   
> -	usleep_range(20, 22);
> +	usleep_range(20, 22); /* T4 */

It would be help to document this addition of comment in the commit 
message as well.
>   
>   	return 0;
>   
> @@ -889,8 +926,8 @@ static int imx335_power_off(struct device *dev)
>   	struct imx335 *imx335 = to_imx335(sd);
>   
>   	gpiod_set_value_cansleep(imx335->reset_gpio, 0);
> -
>   	clk_disable_unprepare(imx335->inclk);
> +	regulator_bulk_disable(IMX335_NUM_SUPPLIES, imx335->supplies);
>   
>   	return 0;
>   }

Re: [PATCH 2/5] media: i2c: imx335: Enable regulator supplies

[Kieran Bingham]

Quoting Umang Jain (2023-10-10 05:06:45)
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On 10/10/23 6:21 AM, Kieran Bingham wrote:
> > Provide support for enabling and disabling regulator supplies to control
> > power to the camera sensor.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >   drivers/media/i2c/imx335.c | 41 ++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 39 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> > index ec729126274b..bf12b9b69fce 100644
> > --- a/drivers/media/i2c/imx335.c
> > +++ b/drivers/media/i2c/imx335.c
> > @@ -75,6 +75,19 @@ struct imx335_reg_list {
> >       const struct imx335_reg *regs;
> >   };
> >   
> > +static const char * const imx335_supply_name[] = {
> > +     /*
> > +      * Turn on the power supplies so that they rise in order of
> > +      *           1.2v,-> 1.8 -> 2.9v
> > +      * All power supplies should finish rising within 200ms.
> > +      */
> > +     "avdd", /* Analog (2.9V) supply */
> > +     "ovdd", /* Digital I/O (1.8V) supply */
> > +     "dvdd", /* Digital Core (1.2V) supply */
> > +};
> > +
> > +#define IMX335_NUM_SUPPLIES ARRAY_SIZE(imx335_supply_name)
> > +
> >   /**
> >    * struct imx335_mode - imx335 sensor mode structure
> >    * @width: Frame width
> > @@ -126,6 +139,8 @@ struct imx335 {
> >       struct v4l2_subdev sd;
> >       struct media_pad pad;
> >       struct gpio_desc *reset_gpio;
> > +     struct regulator_bulk_data supplies[IMX335_NUM_SUPPLIES];
> > +
> >       struct clk *inclk;
> >       struct v4l2_ctrl_handler ctrl_handler;
> >       struct v4l2_ctrl *link_freq_ctrl;
> > @@ -781,6 +796,17 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
> >               return PTR_ERR(imx335->reset_gpio);
> >       }
> >   
> > +     for (i = 0; i < IMX335_NUM_SUPPLIES; i++)
> > +             imx335->supplies[i].supply = imx335_supply_name[i];
> > +
> > +     ret = devm_regulator_bulk_get(imx335->dev,
> > +                                   IMX335_NUM_SUPPLIES,
> > +                                   imx335->supplies);
> 
> Shouldn't this go directly in  probe() function ? (Taking IMX219 driver 
> as a reference..)

I don't think it needs to no. This is a convenience function called
"imx335_parse_hw_config()" which is called from probe(). It just wraps
all of the interactions with the device-tree/firmware layer, and I think
identifying regulators counts as within that remit.


> > +     if (ret) {
> > +             dev_err(imx335->dev, "Failed to get regulators\n");
> > +             return ret;
> > +     }
> > +
> >       /* Get sensor input clock */
> >       imx335->inclk = devm_clk_get(imx335->dev, NULL);
> >       if (IS_ERR(imx335->inclk)) {
> > @@ -859,6 +885,17 @@ static int imx335_power_on(struct device *dev)
> >       struct imx335 *imx335 = to_imx335(sd);
> >       int ret;
> >   
> > +     ret = regulator_bulk_enable(IMX335_NUM_SUPPLIES,
> > +                                 imx335->supplies);
> > +     if (ret) {
> > +             dev_err(dev, "%s: failed to enable regulators\n",
> > +                     __func__);
> > +             return ret;
> > +     }
> > +
> > +     usleep_range(500, 550); /* Tlow */
> > +
> > +     /* Set XCLR */
> >       gpiod_set_value_cansleep(imx335->reset_gpio, 1);
> >   
> >       ret = clk_prepare_enable(imx335->inclk);
> > @@ -867,7 +904,7 @@ static int imx335_power_on(struct device *dev)
> >               goto error_reset;
> >       }
> >   
> > -     usleep_range(20, 22);
> > +     usleep_range(20, 22); /* T4 */
> 
> It would be help to document this addition of comment in the commit 
> message as well.

Yes, I can add a statement saying that "I also extend the comments of the
existing code regarding the power on sequence". T4 in this instance
relates to the entry in the data sheet which specifies how long this
delay should be, and the 'reset_gpio' is known as XCLR in the datasheet.

> >   
> >       return 0;
> >   
> > @@ -889,8 +926,8 @@ static int imx335_power_off(struct device *dev)
> >       struct imx335 *imx335 = to_imx335(sd);
> >   
> >       gpiod_set_value_cansleep(imx335->reset_gpio, 0);
> > -
> >       clk_disable_unprepare(imx335->inclk);
> > +     regulator_bulk_disable(IMX335_NUM_SUPPLIES, imx335->supplies);
> >   
> >       return 0;
> >   }
>

Re: [PATCH 2/5] media: i2c: imx335: Enable regulator supplies

[kernel test robot]

Hi Kieran,

kernel test robot noticed the following build warnings:

[auto build test WARNING on media-tree/master]
[also build test WARNING on sailus-media-tree/streams linus/master v6.6-rc5 next-20231009]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kieran-Bingham/media-dt-bindings-media-imx335-Add-supply-bindings/20231010-085313
base:   git://linuxtv.org/media_tree.git master
patch link:    https://lore.kernel.org/r/20231010005126.3425444-3-kieran.bingham%40ideasonboard.com
patch subject: [PATCH 2/5] media: i2c: imx335: Enable regulator supplies
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231010/202310101224.43dpo3Ng-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231010/202310101224.43dpo3Ng-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310101224.43dpo3Ng-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/media/i2c/imx335.c:159: warning: Function parameter or member 'supplies' not described in 'imx335'


vim +159 drivers/media/i2c/imx335.c

45d19b5fb9aeab Martina Krasteva 2021-05-27  116  
45d19b5fb9aeab Martina Krasteva 2021-05-27  117  /**
45d19b5fb9aeab Martina Krasteva 2021-05-27  118   * struct imx335 - imx335 sensor device structure
45d19b5fb9aeab Martina Krasteva 2021-05-27  119   * @dev: Pointer to generic device
45d19b5fb9aeab Martina Krasteva 2021-05-27  120   * @client: Pointer to i2c client
45d19b5fb9aeab Martina Krasteva 2021-05-27  121   * @sd: V4L2 sub-device
45d19b5fb9aeab Martina Krasteva 2021-05-27  122   * @pad: Media pad. Only one pad supported
45d19b5fb9aeab Martina Krasteva 2021-05-27  123   * @reset_gpio: Sensor reset gpio
45d19b5fb9aeab Martina Krasteva 2021-05-27  124   * @inclk: Sensor input clock
45d19b5fb9aeab Martina Krasteva 2021-05-27  125   * @ctrl_handler: V4L2 control handler
45d19b5fb9aeab Martina Krasteva 2021-05-27  126   * @link_freq_ctrl: Pointer to link frequency control
45d19b5fb9aeab Martina Krasteva 2021-05-27  127   * @pclk_ctrl: Pointer to pixel clock control
45d19b5fb9aeab Martina Krasteva 2021-05-27  128   * @hblank_ctrl: Pointer to horizontal blanking control
45d19b5fb9aeab Martina Krasteva 2021-05-27  129   * @vblank_ctrl: Pointer to vertical blanking control
45d19b5fb9aeab Martina Krasteva 2021-05-27  130   * @exp_ctrl: Pointer to exposure control
45d19b5fb9aeab Martina Krasteva 2021-05-27  131   * @again_ctrl: Pointer to analog gain control
45d19b5fb9aeab Martina Krasteva 2021-05-27  132   * @vblank: Vertical blanking in lines
45d19b5fb9aeab Martina Krasteva 2021-05-27  133   * @cur_mode: Pointer to current selected sensor mode
45d19b5fb9aeab Martina Krasteva 2021-05-27  134   * @mutex: Mutex for serializing sensor controls
45d19b5fb9aeab Martina Krasteva 2021-05-27  135   * @streaming: Flag indicating streaming state
45d19b5fb9aeab Martina Krasteva 2021-05-27  136   */
45d19b5fb9aeab Martina Krasteva 2021-05-27  137  struct imx335 {
45d19b5fb9aeab Martina Krasteva 2021-05-27  138  	struct device *dev;
45d19b5fb9aeab Martina Krasteva 2021-05-27  139  	struct i2c_client *client;
45d19b5fb9aeab Martina Krasteva 2021-05-27  140  	struct v4l2_subdev sd;
45d19b5fb9aeab Martina Krasteva 2021-05-27  141  	struct media_pad pad;
45d19b5fb9aeab Martina Krasteva 2021-05-27  142  	struct gpio_desc *reset_gpio;
15931cfe0b52d1 Kieran Bingham   2023-10-10  143  	struct regulator_bulk_data supplies[IMX335_NUM_SUPPLIES];
15931cfe0b52d1 Kieran Bingham   2023-10-10  144  
45d19b5fb9aeab Martina Krasteva 2021-05-27  145  	struct clk *inclk;
45d19b5fb9aeab Martina Krasteva 2021-05-27  146  	struct v4l2_ctrl_handler ctrl_handler;
45d19b5fb9aeab Martina Krasteva 2021-05-27  147  	struct v4l2_ctrl *link_freq_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27  148  	struct v4l2_ctrl *pclk_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27  149  	struct v4l2_ctrl *hblank_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27  150  	struct v4l2_ctrl *vblank_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27  151  	struct {
45d19b5fb9aeab Martina Krasteva 2021-05-27  152  		struct v4l2_ctrl *exp_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27  153  		struct v4l2_ctrl *again_ctrl;
45d19b5fb9aeab Martina Krasteva 2021-05-27  154  	};
45d19b5fb9aeab Martina Krasteva 2021-05-27  155  	u32 vblank;
45d19b5fb9aeab Martina Krasteva 2021-05-27  156  	const struct imx335_mode *cur_mode;
45d19b5fb9aeab Martina Krasteva 2021-05-27  157  	struct mutex mutex;
45d19b5fb9aeab Martina Krasteva 2021-05-27  158  	bool streaming;
45d19b5fb9aeab Martina Krasteva 2021-05-27 @159  };
45d19b5fb9aeab Martina Krasteva 2021-05-27  160  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/5] media: i2c: imx335: Enable regulator supplies

[Kieran Bingham]

Quoting kernel test robot (2023-10-10 05:10:17)
> Hi Kieran,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on media-tree/master]
> [also build test WARNING on sailus-media-tree/streams linus/master v6.6-rc5 next-20231009]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Kieran-Bingham/media-dt-bindings-media-imx335-Add-supply-bindings/20231010-085313
> base:   git://linuxtv.org/media_tree.git master
> patch link:    https://lore.kernel.org/r/20231010005126.3425444-3-kieran.bingham%40ideasonboard.com
> patch subject: [PATCH 2/5] media: i2c: imx335: Enable regulator supplies
> config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231010/202310101224.43dpo3Ng-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231010/202310101224.43dpo3Ng-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202310101224.43dpo3Ng-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
> >> drivers/media/i2c/imx335.c:159: warning: Function parameter or member 'supplies' not described in 'imx335'
> 

Aha, thank you KTR - I'll fix this.
--
Kieran

Re: [PATCH 2/5] media: i2c: imx335: Enable regulator supplies

[Sakari Ailus]

Hi Kieran,

On Tue, Oct 10, 2023 at 01:51:23AM +0100, Kieran Bingham wrote:
> Provide support for enabling and disabling regulator supplies to control
> power to the camera sensor.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  drivers/media/i2c/imx335.c | 41 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index ec729126274b..bf12b9b69fce 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -75,6 +75,19 @@ struct imx335_reg_list {
>  	const struct imx335_reg *regs;
>  };
>  
> +static const char * const imx335_supply_name[] = {
> +	/*
> +	 * Turn on the power supplies so that they rise in order of
> +	 *           1.2v,-> 1.8 -> 2.9v

This won't happen with regulator_bulk_enable(). Does the spec require this?

> +	 * All power supplies should finish rising within 200ms.
> +	 */
> +	"avdd", /* Analog (2.9V) supply */
> +	"ovdd", /* Digital I/O (1.8V) supply */
> +	"dvdd", /* Digital Core (1.2V) supply */
> +};
> +
> +#define IMX335_NUM_SUPPLIES ARRAY_SIZE(imx335_supply_name)
> +
>  /**
>   * struct imx335_mode - imx335 sensor mode structure
>   * @width: Frame width
> @@ -126,6 +139,8 @@ struct imx335 {
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
>  	struct gpio_desc *reset_gpio;
> +	struct regulator_bulk_data supplies[IMX335_NUM_SUPPLIES];
> +
>  	struct clk *inclk;
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	struct v4l2_ctrl *link_freq_ctrl;
> @@ -781,6 +796,17 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
>  		return PTR_ERR(imx335->reset_gpio);
>  	}
>  
> +	for (i = 0; i < IMX335_NUM_SUPPLIES; i++)
> +		imx335->supplies[i].supply = imx335_supply_name[i];
> +
> +	ret = devm_regulator_bulk_get(imx335->dev,
> +				      IMX335_NUM_SUPPLIES,
> +				      imx335->supplies);
> +	if (ret) {
> +		dev_err(imx335->dev, "Failed to get regulators\n");
> +		return ret;
> +	}
> +
>  	/* Get sensor input clock */
>  	imx335->inclk = devm_clk_get(imx335->dev, NULL);
>  	if (IS_ERR(imx335->inclk)) {
> @@ -859,6 +885,17 @@ static int imx335_power_on(struct device *dev)
>  	struct imx335 *imx335 = to_imx335(sd);
>  	int ret;
>  
> +	ret = regulator_bulk_enable(IMX335_NUM_SUPPLIES,
> +				    imx335->supplies);
> +	if (ret) {
> +		dev_err(dev, "%s: failed to enable regulators\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	usleep_range(500, 550); /* Tlow */

You're not handling the error case later on in the function.

> +
> +	/* Set XCLR */
>  	gpiod_set_value_cansleep(imx335->reset_gpio, 1);
>  
>  	ret = clk_prepare_enable(imx335->inclk);
> @@ -867,7 +904,7 @@ static int imx335_power_on(struct device *dev)
>  		goto error_reset;
>  	}
>  
> -	usleep_range(20, 22);
> +	usleep_range(20, 22); /* T4 */
>  
>  	return 0;
>  
> @@ -889,8 +926,8 @@ static int imx335_power_off(struct device *dev)
>  	struct imx335 *imx335 = to_imx335(sd);
>  
>  	gpiod_set_value_cansleep(imx335->reset_gpio, 0);
> -
>  	clk_disable_unprepare(imx335->inclk);
> +	regulator_bulk_disable(IMX335_NUM_SUPPLIES, imx335->supplies);
>  
>  	return 0;
>  }

-- 
Regards,

Sakari Ailus

Re: [PATCH 2/5] media: i2c: imx335: Enable regulator supplies

[Kieran Bingham]

Quoting Sakari Ailus (2023-10-10 07:12:08)
> Hi Kieran,
> 
> On Tue, Oct 10, 2023 at 01:51:23AM +0100, Kieran Bingham wrote:
> > Provide support for enabling and disabling regulator supplies to control
> > power to the camera sensor.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx335.c | 41 ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 39 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> > index ec729126274b..bf12b9b69fce 100644
> > --- a/drivers/media/i2c/imx335.c
> > +++ b/drivers/media/i2c/imx335.c
> > @@ -75,6 +75,19 @@ struct imx335_reg_list {
> >       const struct imx335_reg *regs;
> >  };
> >  
> > +static const char * const imx335_supply_name[] = {
> > +     /*
> > +      * Turn on the power supplies so that they rise in order of
> > +      *           1.2v,-> 1.8 -> 2.9v
> 
> This won't happen with regulator_bulk_enable(). Does the spec require this?

Every camera I've ever seen handles this in hardware. (I know that's not
an excuse as somewhere there could be a device that routes each of these
separately).

Perhaps I shouldn't have added the comment ;-) But I thought it was
useful while I was working through anyway, and could be important for
other devices indeed.

The datasheet states:

```
1. Turn On the power supplies so that the power supplies rise in order
of 1.2 V power supply (DVDD1, DVDD2) → 1.8 V power supply (OVDD) → 2.9 V
power supply (AVDD1, AVDD2). In addition, all power supplies should
finish rising within 200 ms.

2. The register values are undefined immediately after power-on, so the
system must be cleared. Hold XCLR at Low level for 500 ns or more after
all the power supplies have finished rising. (The register values after
a system clear are the default values.)

3. The system clear is applied by setting XCLR to High level. The maser
clock input after setting the XCLR pin to High level.

4. Make the sensor setting by register communication after the system clear.
```

Regarding 1: T0, and T1 minimum values are '0', so they can all be
enabled at the same time - but of course there will be 'some' interval
between each one. I don't know if that still stipulates the exact
ordering is essential. Perhaps it does.

I don't know how far splitting this out becomes overengineering. Are
there other sensor drivers that already split out each regulator line
with a dedicated ordering? If so - this probably calls for some sort of
ordered bulk regulator enable helper.


> > +      * All power supplies should finish rising within 200ms.
> > +      */
> > +     "avdd", /* Analog (2.9V) supply */
> > +     "ovdd", /* Digital I/O (1.8V) supply */
> > +     "dvdd", /* Digital Core (1.2V) supply */
> > +};
> > +
> > +#define IMX335_NUM_SUPPLIES ARRAY_SIZE(imx335_supply_name)
> > +
> >  /**
> >   * struct imx335_mode - imx335 sensor mode structure
> >   * @width: Frame width
> > @@ -126,6 +139,8 @@ struct imx335 {
> >       struct v4l2_subdev sd;
> >       struct media_pad pad;
> >       struct gpio_desc *reset_gpio;
> > +     struct regulator_bulk_data supplies[IMX335_NUM_SUPPLIES];
> > +
> >       struct clk *inclk;
> >       struct v4l2_ctrl_handler ctrl_handler;
> >       struct v4l2_ctrl *link_freq_ctrl;
> > @@ -781,6 +796,17 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
> >               return PTR_ERR(imx335->reset_gpio);
> >       }
> >  
> > +     for (i = 0; i < IMX335_NUM_SUPPLIES; i++)
> > +             imx335->supplies[i].supply = imx335_supply_name[i];
> > +
> > +     ret = devm_regulator_bulk_get(imx335->dev,
> > +                                   IMX335_NUM_SUPPLIES,
> > +                                   imx335->supplies);
> > +     if (ret) {
> > +             dev_err(imx335->dev, "Failed to get regulators\n");
> > +             return ret;
> > +     }
> > +
> >       /* Get sensor input clock */
> >       imx335->inclk = devm_clk_get(imx335->dev, NULL);
> >       if (IS_ERR(imx335->inclk)) {
> > @@ -859,6 +885,17 @@ static int imx335_power_on(struct device *dev)
> >       struct imx335 *imx335 = to_imx335(sd);
> >       int ret;
> >  
> > +     ret = regulator_bulk_enable(IMX335_NUM_SUPPLIES,
> > +                                 imx335->supplies);
> > +     if (ret) {
> > +             dev_err(dev, "%s: failed to enable regulators\n",
> > +                     __func__);
> > +             return ret;
> > +     }
> > +
> > +     usleep_range(500, 550); /* Tlow */
> 
> You're not handling the error case later on in the function.

Ah yes - thanks. I'll fix this.

--
Kieran


> 
> > +
> > +     /* Set XCLR */
> >       gpiod_set_value_cansleep(imx335->reset_gpio, 1);
> >  
> >       ret = clk_prepare_enable(imx335->inclk);
> > @@ -867,7 +904,7 @@ static int imx335_power_on(struct device *dev)
> >               goto error_reset;
> >       }
> >  
> > -     usleep_range(20, 22);
> > +     usleep_range(20, 22); /* T4 */
> >  
> >       return 0;
> >  
> > @@ -889,8 +926,8 @@ static int imx335_power_off(struct device *dev)
> >       struct imx335 *imx335 = to_imx335(sd);
> >  
> >       gpiod_set_value_cansleep(imx335->reset_gpio, 0);
> > -
> >       clk_disable_unprepare(imx335->inclk);
> > +     regulator_bulk_disable(IMX335_NUM_SUPPLIES, imx335->supplies);
> >  
> >       return 0;
> >  }
> 
> -- 
> Regards,
> 
> Sakari Ailus

Re: [PATCH 2/5] media: i2c: imx335: Enable regulator supplies

[Sakari Ailus]

Hi Kieran,

On Wed, Oct 11, 2023 at 10:41:59AM +0100, Kieran Bingham wrote:
> Quoting Sakari Ailus (2023-10-10 07:12:08)
> > Hi Kieran,
> > 
> > On Tue, Oct 10, 2023 at 01:51:23AM +0100, Kieran Bingham wrote:
> > > Provide support for enabling and disabling regulator supplies to control
> > > power to the camera sensor.
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/imx335.c | 41 ++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 39 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> > > index ec729126274b..bf12b9b69fce 100644
> > > --- a/drivers/media/i2c/imx335.c
> > > +++ b/drivers/media/i2c/imx335.c
> > > @@ -75,6 +75,19 @@ struct imx335_reg_list {
> > >       const struct imx335_reg *regs;
> > >  };
> > >  
> > > +static const char * const imx335_supply_name[] = {
> > > +     /*
> > > +      * Turn on the power supplies so that they rise in order of
> > > +      *           1.2v,-> 1.8 -> 2.9v
> > 
> > This won't happen with regulator_bulk_enable(). Does the spec require this?
> 
> Every camera I've ever seen handles this in hardware. (I know that's not
> an excuse as somewhere there could be a device that routes each of these
> separately).
> 
> Perhaps I shouldn't have added the comment ;-) But I thought it was
> useful while I was working through anyway, and could be important for
> other devices indeed.
> 
> The datasheet states:
> 
> ```
> 1. Turn On the power supplies so that the power supplies rise in order
> of 1.2 V power supply (DVDD1, DVDD2) → 1.8 V power supply (OVDD) → 2.9 V
> power supply (AVDD1, AVDD2). In addition, all power supplies should
> finish rising within 200 ms.

That's an annoying requirement but I'd presume this to work just fine in
practice. The device is reset in any case (as you describe below). Some
boards might not wire the reset GPIO though, and then it's when it gets
interesting.

To literally implement the documented sequence, you should separately
enable the regulators one by one.

Although I don't object just removing the comment either.

-- 
Regards,

Sakari Ailus

Re: [PATCH 2/5] media: i2c: imx335: Enable regulator supplies

[Kieran Bingham]

Quoting Sakari Ailus (2023-10-11 12:06:19)
> Hi Kieran,
> 
> On Wed, Oct 11, 2023 at 10:41:59AM +0100, Kieran Bingham wrote:
> > Quoting Sakari Ailus (2023-10-10 07:12:08)
> > > Hi Kieran,
> > > 
> > > On Tue, Oct 10, 2023 at 01:51:23AM +0100, Kieran Bingham wrote:
> > > > Provide support for enabling and disabling regulator supplies to control
> > > > power to the camera sensor.
> > > > 
> > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > ---
> > > >  drivers/media/i2c/imx335.c | 41 ++++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 39 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> > > > index ec729126274b..bf12b9b69fce 100644
> > > > --- a/drivers/media/i2c/imx335.c
> > > > +++ b/drivers/media/i2c/imx335.c
> > > > @@ -75,6 +75,19 @@ struct imx335_reg_list {
> > > >       const struct imx335_reg *regs;
> > > >  };
> > > >  
> > > > +static const char * const imx335_supply_name[] = {
> > > > +     /*
> > > > +      * Turn on the power supplies so that they rise in order of
> > > > +      *           1.2v,-> 1.8 -> 2.9v
> > > 
> > > This won't happen with regulator_bulk_enable(). Does the spec require this?
> > 
> > Every camera I've ever seen handles this in hardware. (I know that's not
> > an excuse as somewhere there could be a device that routes each of these
> > separately).
> > 
> > Perhaps I shouldn't have added the comment ;-) But I thought it was
> > useful while I was working through anyway, and could be important for
> > other devices indeed.
> > 
> > The datasheet states:
> > 
> > ```
> > 1. Turn On the power supplies so that the power supplies rise in order
> > of 1.2 V power supply (DVDD1, DVDD2) → 1.8 V power supply (OVDD) → 2.9 V
> > power supply (AVDD1, AVDD2). In addition, all power supplies should
> > finish rising within 200 ms.
> 
> That's an annoying requirement but I'd presume this to work just fine in
> practice. The device is reset in any case (as you describe below). Some
> boards might not wire the reset GPIO though, and then it's when it gets
> interesting.

Correct - this board does not expose the reset/XCLR to me anyway, so I
can not control that.

> To literally implement the documented sequence, you should separately
> enable the regulators one by one.
> 
> Although I don't object just removing the comment either.

Given that neither the existing user, nor this user (me) need this,
*and* the schematics of my (working) camera module show that it's fine
to enable all regulators at the same time - I'll remove the comment.

--
Thanks

Kieran

> 
> -- 
> Regards,
> 
> Sakari Ailus

[PATCH 3/5] media: i2c: imx335: Implement get selection API

[Kieran Bingham]

Support reporting of the Sensor Native and Active pixel array areas
through the Selection API.

The implementation reports a single target crop only for the mode that
is presently exposed by the driver.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 44 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index bf12b9b69fce..026777eb362e 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -55,6 +55,14 @@
 #define IMX335_REG_MIN		0x00
 #define IMX335_REG_MAX		0xfffff
 
+/* IMX335 native and active pixel array size. */
+#define IMX335_NATIVE_WIDTH		2616U
+#define IMX335_NATIVE_HEIGHT		1964U
+#define IMX335_PIXEL_ARRAY_LEFT		12U
+#define IMX335_PIXEL_ARRAY_TOP		12U
+#define IMX335_PIXEL_ARRAY_WIDTH	2592U
+#define IMX335_PIXEL_ARRAY_HEIGHT	1944U
+
 /**
  * struct imx335_reg - imx335 sensor register
  * @address: Register address
@@ -651,6 +659,41 @@ static int imx335_init_pad_cfg(struct v4l2_subdev *sd,
 	return imx335_set_pad_format(sd, sd_state, &fmt);
 }
 
+/**
+ * imx335_get_selection() - Selection API
+ * @sd: pointer to imx335 V4L2 sub-device structure
+ * @sd_state: V4L2 sub-device configuration
+ * @sel: V4L2 selection info
+ *
+ * Return: 0 if successful, error code otherwise.
+ */
+static int imx335_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *sd_state,
+				struct v4l2_subdev_selection *sel)
+{
+	switch (sel->target) {
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = IMX335_NATIVE_WIDTH;
+		sel->r.height = IMX335_NATIVE_HEIGHT;
+
+		return 0;
+
+	case V4L2_SEL_TGT_CROP:
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.top = IMX335_PIXEL_ARRAY_TOP;
+		sel->r.left = IMX335_PIXEL_ARRAY_LEFT;
+		sel->r.width = IMX335_PIXEL_ARRAY_WIDTH;
+		sel->r.height = IMX335_PIXEL_ARRAY_HEIGHT;
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 /**
  * imx335_start_streaming() - Start sensor stream
  * @imx335: pointer to imx335 device
@@ -864,6 +907,7 @@ static const struct v4l2_subdev_pad_ops imx335_pad_ops = {
 	.init_cfg = imx335_init_pad_cfg,
 	.enum_mbus_code = imx335_enum_mbus_code,
 	.enum_frame_size = imx335_enum_frame_size,
+	.get_selection = imx335_get_selection,
 	.get_fmt = imx335_get_pad_format,
 	.set_fmt = imx335_set_pad_format,
 };
-- 
2.34.1

Re: [PATCH 3/5] media: i2c: imx335: Implement get selection API

[Umang Jain]

Hi Kieran

On 10/10/23 6:21 AM, Kieran Bingham wrote:
> Support reporting of the Sensor Native and Active pixel array areas
> through the Selection API.
>
> The implementation reports a single target crop only for the mode that
> is presently exposed by the driver.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

LGTM,

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
>   drivers/media/i2c/imx335.c | 44 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 44 insertions(+)
>
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index bf12b9b69fce..026777eb362e 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -55,6 +55,14 @@
>   #define IMX335_REG_MIN		0x00
>   #define IMX335_REG_MAX		0xfffff
>   
> +/* IMX335 native and active pixel array size. */
> +#define IMX335_NATIVE_WIDTH		2616U
> +#define IMX335_NATIVE_HEIGHT		1964U
> +#define IMX335_PIXEL_ARRAY_LEFT		12U
> +#define IMX335_PIXEL_ARRAY_TOP		12U
> +#define IMX335_PIXEL_ARRAY_WIDTH	2592U
> +#define IMX335_PIXEL_ARRAY_HEIGHT	1944U
> +
>   /**
>    * struct imx335_reg - imx335 sensor register
>    * @address: Register address
> @@ -651,6 +659,41 @@ static int imx335_init_pad_cfg(struct v4l2_subdev *sd,
>   	return imx335_set_pad_format(sd, sd_state, &fmt);
>   }
>   
> +/**
> + * imx335_get_selection() - Selection API
> + * @sd: pointer to imx335 V4L2 sub-device structure
> + * @sd_state: V4L2 sub-device configuration
> + * @sel: V4L2 selection info
> + *
> + * Return: 0 if successful, error code otherwise.
> + */
> +static int imx335_get_selection(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_state *sd_state,
> +				struct v4l2_subdev_selection *sel)
> +{
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = IMX335_NATIVE_WIDTH;
> +		sel->r.height = IMX335_NATIVE_HEIGHT;
> +
> +		return 0;
> +
> +	case V4L2_SEL_TGT_CROP:
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +		sel->r.top = IMX335_PIXEL_ARRAY_TOP;
> +		sel->r.left = IMX335_PIXEL_ARRAY_LEFT;
> +		sel->r.width = IMX335_PIXEL_ARRAY_WIDTH;
> +		sel->r.height = IMX335_PIXEL_ARRAY_HEIGHT;
> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>   /**
>    * imx335_start_streaming() - Start sensor stream
>    * @imx335: pointer to imx335 device
> @@ -864,6 +907,7 @@ static const struct v4l2_subdev_pad_ops imx335_pad_ops = {
>   	.init_cfg = imx335_init_pad_cfg,
>   	.enum_mbus_code = imx335_enum_mbus_code,
>   	.enum_frame_size = imx335_enum_frame_size,
> +	.get_selection = imx335_get_selection,
>   	.get_fmt = imx335_get_pad_format,
>   	.set_fmt = imx335_set_pad_format,
>   };

Re: [PATCH 3/5] media: i2c: imx335: Implement get selection API

[Sakari Ailus]

Hi Kieran,

On Tue, Oct 10, 2023 at 01:51:24AM +0100, Kieran Bingham wrote:
> Support reporting of the Sensor Native and Active pixel array areas
> through the Selection API.
> 
> The implementation reports a single target crop only for the mode that
> is presently exposed by the driver.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Shouldn't you use the same callback for .set_selection? I guess this is
somewhat grey area but doing so would be in line with how V4L2 API works in
general.

Cc Laurent.

> ---
>  drivers/media/i2c/imx335.c | 44 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index bf12b9b69fce..026777eb362e 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -55,6 +55,14 @@
>  #define IMX335_REG_MIN		0x00
>  #define IMX335_REG_MAX		0xfffff
>  
> +/* IMX335 native and active pixel array size. */
> +#define IMX335_NATIVE_WIDTH		2616U
> +#define IMX335_NATIVE_HEIGHT		1964U
> +#define IMX335_PIXEL_ARRAY_LEFT		12U
> +#define IMX335_PIXEL_ARRAY_TOP		12U
> +#define IMX335_PIXEL_ARRAY_WIDTH	2592U
> +#define IMX335_PIXEL_ARRAY_HEIGHT	1944U
> +
>  /**
>   * struct imx335_reg - imx335 sensor register
>   * @address: Register address
> @@ -651,6 +659,41 @@ static int imx335_init_pad_cfg(struct v4l2_subdev *sd,
>  	return imx335_set_pad_format(sd, sd_state, &fmt);
>  }
>  
> +/**
> + * imx335_get_selection() - Selection API
> + * @sd: pointer to imx335 V4L2 sub-device structure
> + * @sd_state: V4L2 sub-device configuration
> + * @sel: V4L2 selection info
> + *
> + * Return: 0 if successful, error code otherwise.
> + */
> +static int imx335_get_selection(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_state *sd_state,
> +				struct v4l2_subdev_selection *sel)
> +{
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = IMX335_NATIVE_WIDTH;
> +		sel->r.height = IMX335_NATIVE_HEIGHT;
> +
> +		return 0;
> +
> +	case V4L2_SEL_TGT_CROP:
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +		sel->r.top = IMX335_PIXEL_ARRAY_TOP;
> +		sel->r.left = IMX335_PIXEL_ARRAY_LEFT;
> +		sel->r.width = IMX335_PIXEL_ARRAY_WIDTH;
> +		sel->r.height = IMX335_PIXEL_ARRAY_HEIGHT;
> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  /**
>   * imx335_start_streaming() - Start sensor stream
>   * @imx335: pointer to imx335 device
> @@ -864,6 +907,7 @@ static const struct v4l2_subdev_pad_ops imx335_pad_ops = {
>  	.init_cfg = imx335_init_pad_cfg,
>  	.enum_mbus_code = imx335_enum_mbus_code,
>  	.enum_frame_size = imx335_enum_frame_size,
> +	.get_selection = imx335_get_selection,
>  	.get_fmt = imx335_get_pad_format,
>  	.set_fmt = imx335_set_pad_format,
>  };

-- 
Regards,

Sakari Ailus

Re: [PATCH 3/5] media: i2c: imx335: Implement get selection API

[Kieran Bingham]

Quoting Sakari Ailus (2023-10-10 07:14:09)
> Hi Kieran,
> 
> On Tue, Oct 10, 2023 at 01:51:24AM +0100, Kieran Bingham wrote:
> > Support reporting of the Sensor Native and Active pixel array areas
> > through the Selection API.
> > 
> > The implementation reports a single target crop only for the mode that
> > is presently exposed by the driver.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Shouldn't you use the same callback for .set_selection? I guess this is
> somewhat grey area but doing so would be in line with how V4L2 API works in
> general.

Hrm ... I didn't think it was needed as it's not possible to /set/
anything.

I expect to change this once I add support for setting crops later
though. It was going to be something I'd add when it is used.

Only the 'get_selection' call is necessary to make this camera operate
on both i.MX8MP and RPi5 platforms with libcamera, so that's what I've
done so far. My goal of this series was to bring the existing driver up
to a point that it can be used, before I start making new feature
additions.

--
Kieran

> 
> Cc Laurent.
> 
> > ---
> >  drivers/media/i2c/imx335.c | 44 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> > index bf12b9b69fce..026777eb362e 100644
> > --- a/drivers/media/i2c/imx335.c
> > +++ b/drivers/media/i2c/imx335.c
> > @@ -55,6 +55,14 @@
> >  #define IMX335_REG_MIN               0x00
> >  #define IMX335_REG_MAX               0xfffff
> >  
> > +/* IMX335 native and active pixel array size. */
> > +#define IMX335_NATIVE_WIDTH          2616U
> > +#define IMX335_NATIVE_HEIGHT         1964U
> > +#define IMX335_PIXEL_ARRAY_LEFT              12U
> > +#define IMX335_PIXEL_ARRAY_TOP               12U
> > +#define IMX335_PIXEL_ARRAY_WIDTH     2592U
> > +#define IMX335_PIXEL_ARRAY_HEIGHT    1944U
> > +
> >  /**
> >   * struct imx335_reg - imx335 sensor register
> >   * @address: Register address
> > @@ -651,6 +659,41 @@ static int imx335_init_pad_cfg(struct v4l2_subdev *sd,
> >       return imx335_set_pad_format(sd, sd_state, &fmt);
> >  }
> >  
> > +/**
> > + * imx335_get_selection() - Selection API
> > + * @sd: pointer to imx335 V4L2 sub-device structure
> > + * @sd_state: V4L2 sub-device configuration
> > + * @sel: V4L2 selection info
> > + *
> > + * Return: 0 if successful, error code otherwise.
> > + */
> > +static int imx335_get_selection(struct v4l2_subdev *sd,
> > +                             struct v4l2_subdev_state *sd_state,
> > +                             struct v4l2_subdev_selection *sel)
> > +{
> > +     switch (sel->target) {
> > +     case V4L2_SEL_TGT_NATIVE_SIZE:
> > +             sel->r.top = 0;
> > +             sel->r.left = 0;
> > +             sel->r.width = IMX335_NATIVE_WIDTH;
> > +             sel->r.height = IMX335_NATIVE_HEIGHT;
> > +
> > +             return 0;
> > +
> > +     case V4L2_SEL_TGT_CROP:
> > +     case V4L2_SEL_TGT_CROP_DEFAULT:
> > +     case V4L2_SEL_TGT_CROP_BOUNDS:
> > +             sel->r.top = IMX335_PIXEL_ARRAY_TOP;
> > +             sel->r.left = IMX335_PIXEL_ARRAY_LEFT;
> > +             sel->r.width = IMX335_PIXEL_ARRAY_WIDTH;
> > +             sel->r.height = IMX335_PIXEL_ARRAY_HEIGHT;
> > +
> > +             return 0;
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> >  /**
> >   * imx335_start_streaming() - Start sensor stream
> >   * @imx335: pointer to imx335 device
> > @@ -864,6 +907,7 @@ static const struct v4l2_subdev_pad_ops imx335_pad_ops = {
> >       .init_cfg = imx335_init_pad_cfg,
> >       .enum_mbus_code = imx335_enum_mbus_code,
> >       .enum_frame_size = imx335_enum_frame_size,
> > +     .get_selection = imx335_get_selection,
> >       .get_fmt = imx335_get_pad_format,
> >       .set_fmt = imx335_set_pad_format,
> >  };
> 
> -- 
> Regards,
> 
> Sakari Ailus

Re: [PATCH 3/5] media: i2c: imx335: Implement get selection API

[Sakari Ailus]

Hi Kieran,

On Wed, Oct 11, 2023 at 10:58:38AM +0100, Kieran Bingham wrote:
> Quoting Sakari Ailus (2023-10-10 07:14:09)
> > Hi Kieran,
> > 
> > On Tue, Oct 10, 2023 at 01:51:24AM +0100, Kieran Bingham wrote:
> > > Support reporting of the Sensor Native and Active pixel array areas
> > > through the Selection API.
> > > 
> > > The implementation reports a single target crop only for the mode that
> > > is presently exposed by the driver.
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > Shouldn't you use the same callback for .set_selection? I guess this is
> > somewhat grey area but doing so would be in line with how V4L2 API works in
> > general.
> 
> Hrm ... I didn't think it was needed as it's not possible to /set/
> anything.

Similarly, VIDIOC_SUBDEV_S_FMT is available even if you can't change the
format.

> 
> I expect to change this once I add support for setting crops later
> though. It was going to be something I'd add when it is used.
> 
> Only the 'get_selection' call is necessary to make this camera operate
> on both i.MX8MP and RPi5 platforms with libcamera, so that's what I've
> done so far. My goal of this series was to bring the existing driver up
> to a point that it can be used, before I start making new feature
> additions.

I don't have concerns with that, just that we implement the IOCTLs
consitently. This has been discussed before but AFAIR without any firm
conclusions.

Additionally, some targets are settable while some won't be, and it may
well depend on the driver.

v4l2-compliance appears to be happy with G_SELECTION without S_SELECTION
though.

Also cc Hans.

-- 
Regards,

Sakari Ailus

[PATCH 4/5] media: i2c: imx335: Fix hblank min/max values

[Kieran Bingham]

The V4L2_CID_HBLANK control is marked as readonly and can only be a
single value.

Set the minimum and maximum value to match the mode value.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index 026777eb362e..1a34b2a43718 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -1043,8 +1043,8 @@ static int imx335_init_controls(struct imx335 *imx335)
 	imx335->hblank_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
 						&imx335_ctrl_ops,
 						V4L2_CID_HBLANK,
-						IMX335_REG_MIN,
-						IMX335_REG_MAX,
+						mode->hblank,
+						mode->hblank,
 						1, mode->hblank);
 	if (imx335->hblank_ctrl)
 		imx335->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-- 
2.34.1

Re: [PATCH 4/5] media: i2c: imx335: Fix hblank min/max values

[Umang Jain]

Hi Kieran

On 10/10/23 6:21 AM, Kieran Bingham wrote:
> The V4L2_CID_HBLANK control is marked as readonly and can only be a
> single value.
>
> Set the minimum and maximum value to match the mode value.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
>   drivers/media/i2c/imx335.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index 026777eb362e..1a34b2a43718 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -1043,8 +1043,8 @@ static int imx335_init_controls(struct imx335 *imx335)
>   	imx335->hblank_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
>   						&imx335_ctrl_ops,
>   						V4L2_CID_HBLANK,
> -						IMX335_REG_MIN,
> -						IMX335_REG_MAX,
> +						mode->hblank,
> +						mode->hblank,
>   						1, mode->hblank);
>   	if (imx335->hblank_ctrl)
>   		imx335->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;

[PATCH 5/5] media: i2c: imx335: Improve configuration error reporting

[Kieran Bingham]

The existing imx335_parse_hw_config function has two paths
that can be taken without reporting to the user the reason
for failing to accept the hardware configuration.

Extend the error reporting paths to identify failures when
probing the device.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index 1a34b2a43718..753e5c39e0fa 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -864,8 +864,10 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
 	}
 
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
-	if (!ep)
+	if (!ep) {
+		dev_err(imx335->dev, "Failed to get next endpoint");
 		return -ENXIO;
+	}
 
 	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
 	fwnode_handle_put(ep);
@@ -890,6 +892,8 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
 		if (bus_cfg.link_frequencies[i] == IMX335_LINK_FREQ)
 			goto done_endpoint_free;
 
+	dev_err(imx335->dev, "no compatible link frequencies found");
+
 	ret = -EINVAL;
 
 done_endpoint_free:
-- 
2.34.1

Re: [PATCH 5/5] media: i2c: imx335: Improve configuration error reporting

[Umang Jain]

Hi Kieran,

Thank you for the patch.

On 10/10/23 6:21 AM, Kieran Bingham wrote:
> The existing imx335_parse_hw_config function has two paths
> that can be taken without reporting to the user the reason
> for failing to accept the hardware configuration.
>
> Extend the error reporting paths to identify failures when
> probing the device.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>   drivers/media/i2c/imx335.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index 1a34b2a43718..753e5c39e0fa 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -864,8 +864,10 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
>   	}
>   
>   	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> -	if (!ep)
> +	if (!ep) {
> +		dev_err(imx335->dev, "Failed to get next endpoint");

missing '\n' at the end.
>   		return -ENXIO;
> +	}
>   
>   	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
>   	fwnode_handle_put(ep);
> @@ -890,6 +892,8 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
>   		if (bus_cfg.link_frequencies[i] == IMX335_LINK_FREQ)
>   			goto done_endpoint_free;
>   
> +	dev_err(imx335->dev, "no compatible link frequencies found");

Ditto.

Other than that,

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> +
>   	ret = -EINVAL;
>   
>   done_endpoint_free: