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

[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

[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

[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
> 

[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

[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

[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

[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

[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

[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: