media: i2c: imx258: Add reset-gpio support

[PATCH] media: i2c: imx258: Add reset-gpio support

Posted by Muzaffer Kadir via B4 Relay 4 weeks, 1 day ago

From: Muzaffer Kadir <muzafferkadir@mainlining.org>

reset-gpio is already documented in dt-bindings but never implemented
in the driver.
Reset deassert delay comes from Luis Garcia's and Ondrej Jirman's patch.

Link: https://lore.kernel.org/all/20240602201345.328737-22-git@luigi311.com
Signed-off-by: Muzaffer Kadir <muzafferkadir@mainlining.org>
---
I have a device that is not upstreamed yet (General Mobile Shamrock)
whose camera needs reset gpio to probe, it is documented for
dts check but not implemented for some reason.
With adding it rear camera on the device probes correctly.

I created this patch without knowing the older one that submitted
before: https://lore.kernel.org/all/20240602201345.328737-22-git@luigi311.com/

I don't fully know the correct reset timing so I was using a random wait before,
after I discovered existing patch I reused previous work for delay time after reset.
---
 drivers/media/i2c/imx258.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index bc9ee449a87c..af3f12c7452a 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -9,6 +9,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <linux/unaligned.h>
+#include <linux/gpio/consumer.h>
 
 #include <media/v4l2-cci.h>
 #include <media/v4l2-ctrls.h>
@@ -681,6 +682,7 @@ struct imx258 {
 
 	struct clk *clk;
 	struct regulator_bulk_data supplies[IMX258_NUM_SUPPLIES];
+	struct gpio_desc *reset_gpio;
 };
 
 static inline struct imx258 *to_imx258(struct v4l2_subdev *_sd)
@@ -1128,6 +1130,18 @@ static int imx258_power_on(struct device *dev)
 	if (ret) {
 		dev_err(dev, "failed to enable clock\n");
 		regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
+		return ret;
+	}
+
+	if (imx258->reset_gpio) {
+		ret = gpiod_set_value_cansleep(imx258->reset_gpio, 0);
+		if (ret) {
+			dev_err(dev, "failed to deassert reset\n");
+			clk_disable_unprepare(imx258->clk);
+			regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
+			return ret;
+		}
+		usleep_range(400, 500);
 	}
 
 	return ret;
@@ -1138,6 +1152,7 @@ static int imx258_power_off(struct device *dev)
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct imx258 *imx258 = to_imx258(sd);
 
+	gpiod_set_value_cansleep(imx258->reset_gpio, 1);
 	clk_disable_unprepare(imx258->clk);
 	regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
 
@@ -1382,6 +1397,11 @@ static int imx258_probe(struct i2c_client *client)
 		return ret;
 	}
 
+	imx258->reset_gpio = devm_gpiod_get_optional(imx258->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(imx258->reset_gpio))
+		return dev_err_probe(imx258->dev, PTR_ERR(imx258->reset_gpio),
+				     "Failed to get reset-gpios\n");
+
 	ret = imx258_get_regulators(imx258);
 	if (ret)
 		return dev_err_probe(imx258->dev, ret,

---
base-commit: db2ddb87143519e20a95aa36c60b36107b736a58
change-id: 20260825-imx258-add-reset-gpio-patch-c438dd35f9cc

Best regards,
-- 
Muzaffer Kadir <muzafferkadir@mainlining.org>

Re: [PATCH] media: i2c: imx258: Add reset-gpio support

Posted by Ondřej Jirman 4 weeks, 1 day ago

Hello Muzaffer,

On Fri, Aug 28, 2026 at 01:51:47PM +0300, Muzaffer Kadir via B4 Relay wrote:
> From: Muzaffer Kadir <muzafferkadir@mainlining.org>
> 
> reset-gpio is already documented in dt-bindings but never implemented
> in the driver.
> Reset deassert delay comes from Luis Garcia's and Ondrej Jirman's patch.
> 
> Link: https://lore.kernel.org/all/20240602201345.328737-22-git@luigi311.com
> Signed-off-by: Muzaffer Kadir <muzafferkadir@mainlining.org>
> ---
> I have a device that is not upstreamed yet (General Mobile Shamrock)
> whose camera needs reset gpio to probe, it is documented for
> dts check but not implemented for some reason.
> With adding it rear camera on the device probes correctly.
> 
> I created this patch without knowing the older one that submitted
> before: https://lore.kernel.org/all/20240602201345.328737-22-git@luigi311.com/
> 
> I don't fully know the correct reset timing so I was using a random wait before,
> after I discovered existing patch I reused previous work for delay time after reset.
> ---
>  drivers/media/i2c/imx258.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index bc9ee449a87c..af3f12c7452a 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -9,6 +9,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/unaligned.h>
> +#include <linux/gpio/consumer.h>
>  
>  #include <media/v4l2-cci.h>
>  #include <media/v4l2-ctrls.h>
> @@ -681,6 +682,7 @@ struct imx258 {
>  
>  	struct clk *clk;
>  	struct regulator_bulk_data supplies[IMX258_NUM_SUPPLIES];
> +	struct gpio_desc *reset_gpio;
>  };
>  
>  static inline struct imx258 *to_imx258(struct v4l2_subdev *_sd)
> @@ -1128,6 +1130,18 @@ static int imx258_power_on(struct device *dev)
>  	if (ret) {
>  		dev_err(dev, "failed to enable clock\n");
>  		regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
> +		return ret;
> +	}
> +
> +	if (imx258->reset_gpio) {
> +		ret = gpiod_set_value_cansleep(imx258->reset_gpio, 0);
> +		if (ret) {
> +			dev_err(dev, "failed to deassert reset\n");
> +			clk_disable_unprepare(imx258->clk);
> +			regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
> +			return ret;
> +		}
> +		usleep_range(400, 500);
>  	}

There are two times given in the datasheet. min 400 us between reset deassert
and ID register read over CCI. And minimum time between deassert and stream
start, which is 12ms.

  https://xff.cz/dl/tmp/b8efc244280a2b35.png

  https://xff.cz/dl/tmp/ed9b967a140bd347.png

I wonder if this driver satisfied the second constraint. That may perhaps depend
on how userspace uses it.

Best regards,
	o.

>  	return ret;
> @@ -1138,6 +1152,7 @@ static int imx258_power_off(struct device *dev)
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>  	struct imx258 *imx258 = to_imx258(sd);
>  
> +	gpiod_set_value_cansleep(imx258->reset_gpio, 1);
>  	clk_disable_unprepare(imx258->clk);
>  	regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
>  
> @@ -1382,6 +1397,11 @@ static int imx258_probe(struct i2c_client *client)
>  		return ret;
>  	}
>  
> +	imx258->reset_gpio = devm_gpiod_get_optional(imx258->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(imx258->reset_gpio))
> +		return dev_err_probe(imx258->dev, PTR_ERR(imx258->reset_gpio),
> +				     "Failed to get reset-gpios\n");
> +
>  	ret = imx258_get_regulators(imx258);
>  	if (ret)
>  		return dev_err_probe(imx258->dev, ret,
> 
> ---
> base-commit: db2ddb87143519e20a95aa36c60b36107b736a58
> change-id: 20260825-imx258-add-reset-gpio-patch-c438dd35f9cc
> 
> Best regards,
> -- 
> Muzaffer Kadir <muzafferkadir@mainlining.org>
> 
>

Re: [PATCH] media: i2c: imx258: Add reset-gpio support

Posted by Muzaffer Kadir 4 weeks ago

Hi Ondřej,

On Fri, 28 Aug 2026 20:16:30 +0200, Ondřej Jirman wrote:
>> @@ -1128,6 +1130,18 @@ static int imx258_power_on(struct device *dev)
>>  	if (ret) {
>>  		dev_err(dev, "failed to enable clock\n");
>>  		regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
>> +		return ret;
>> +	}
>> +
>> +	if (imx258->reset_gpio) {
>> +		ret = gpiod_set_value_cansleep(imx258->reset_gpio, 0);
>> +		if (ret) {
>> +			dev_err(dev, "failed to deassert reset\n");
>> +			clk_disable_unprepare(imx258->clk);
>> +			regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
>> +			return ret;
>> +		}
>> +		usleep_range(400, 500);
>>  	}
> 
> There are two times given in the datasheet. min 400 us between reset deassert
> and ID register read over CCI. And minimum time between deassert and stream
> start, which is 12ms.
> 
>   https://xff.cz/dl/tmp/b8efc244280a2b35.png
> 
>   https://xff.cz/dl/tmp/ed9b967a140bd347.png
> 
> I wonder if this driver satisfied the second constraint. That may perhaps depend
> on how userspace uses it.
Thanks for datasheet timings. There is an existing software reset and 12ms wait call
inside imx258_start_streaming function. It should satisfy stream start.
Please correct me if I am wrong.
-- 
Best Regards,

Muzaffer Kadir

Re: [PATCH] media: i2c: imx258: Add reset-gpio support

Posted by Ondřej Jirman 4 weeks ago

On Sat, Aug 29, 2026 at 01:28:31PM +0300, Muzaffer Kadir wrote:
> Hi Ondřej,
> 
> On Fri, 28 Aug 2026 20:16:30 +0200, Ondřej Jirman wrote:
> >> @@ -1128,6 +1130,18 @@ static int imx258_power_on(struct device *dev)
> >>  	if (ret) {
> >>  		dev_err(dev, "failed to enable clock\n");
> >>  		regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
> >> +		return ret;
> >> +	}
> >> +
> >> +	if (imx258->reset_gpio) {
> >> +		ret = gpiod_set_value_cansleep(imx258->reset_gpio, 0);
> >> +		if (ret) {
> >> +			dev_err(dev, "failed to deassert reset\n");
> >> +			clk_disable_unprepare(imx258->clk);
> >> +			regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
> >> +			return ret;
> >> +		}
> >> +		usleep_range(400, 500);
> >>  	}
> > 
> > There are two times given in the datasheet. min 400 us between reset deassert
> > and ID register read over CCI. And minimum time between deassert and stream
> > start, which is 12ms.
> > 
> >   https://xff.cz/dl/tmp/b8efc244280a2b35.png
> > 
> >   https://xff.cz/dl/tmp/ed9b967a140bd347.png
> > 
> > I wonder if this driver satisfied the second constraint. That may perhaps depend
> > on how userspace uses it.
> Thanks for datasheet timings. There is an existing software reset and 12ms wait call
> inside imx258_start_streaming function. It should satisfy stream start.
> Please correct me if I am wrong.

Yes, so that's fine then. :)

Best regards,
	o.

> -- 
> Best Regards,
> 
> Muzaffer Kadir

Re: [PATCH] media: i2c: imx258: Add reset-gpio support

Posted by Sakari Ailus 4 weeks, 1 day ago

Hi Muzaffer,

Thanks for the set.

On Fri, Aug 28, 2026 at 01:51:47PM +0300, Muzaffer Kadir via B4 Relay wrote:
> From: Muzaffer Kadir <muzafferkadir@mainlining.org>
> 
> reset-gpio is already documented in dt-bindings but never implemented
> in the driver.
> Reset deassert delay comes from Luis Garcia's and Ondrej Jirman's patch.
> 
> Link: https://lore.kernel.org/all/20240602201345.328737-22-git@luigi311.com
> Signed-off-by: Muzaffer Kadir <muzafferkadir@mainlining.org>
> ---
> I have a device that is not upstreamed yet (General Mobile Shamrock)
> whose camera needs reset gpio to probe, it is documented for
> dts check but not implemented for some reason.
> With adding it rear camera on the device probes correctly.
> 
> I created this patch without knowing the older one that submitted
> before: https://lore.kernel.org/all/20240602201345.328737-22-git@luigi311.com/
> 
> I don't fully know the correct reset timing so I was using a random wait before,
> after I discovered existing patch I reused previous work for delay time after reset.
> ---
>  drivers/media/i2c/imx258.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index bc9ee449a87c..af3f12c7452a 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -9,6 +9,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/unaligned.h>
> +#include <linux/gpio/consumer.h>
>  
>  #include <media/v4l2-cci.h>
>  #include <media/v4l2-ctrls.h>
> @@ -681,6 +682,7 @@ struct imx258 {
>  
>  	struct clk *clk;
>  	struct regulator_bulk_data supplies[IMX258_NUM_SUPPLIES];
> +	struct gpio_desc *reset_gpio;
>  };
>  
>  static inline struct imx258 *to_imx258(struct v4l2_subdev *_sd)
> @@ -1128,6 +1130,18 @@ static int imx258_power_on(struct device *dev)
>  	if (ret) {
>  		dev_err(dev, "failed to enable clock\n");
>  		regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
> +		return ret;
> +	}
> +
> +	if (imx258->reset_gpio) {
> +		ret = gpiod_set_value_cansleep(imx258->reset_gpio, 0);
> +		if (ret) {
> +			dev_err(dev, "failed to deassert reset\n");
> +			clk_disable_unprepare(imx258->clk);
> +			regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
> +			return ret;

This warrants reworking error handling; please use gotos and move it to the
end of the function. Same for clock error handling.

> +		}
> +		usleep_range(400, 500);

The delay seems right. Can you use fsleep()?

In fact the delay should always have been there so this is a bugfix. It
should go to a separate patch.

>  	}
>  
>  	return ret;
> @@ -1138,6 +1152,7 @@ static int imx258_power_off(struct device *dev)
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>  	struct imx258 *imx258 = to_imx258(sd);
>  
> +	gpiod_set_value_cansleep(imx258->reset_gpio, 1);
>  	clk_disable_unprepare(imx258->clk);
>  	regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
>  
> @@ -1382,6 +1397,11 @@ static int imx258_probe(struct i2c_client *client)
>  		return ret;
>  	}
>  
> +	imx258->reset_gpio = devm_gpiod_get_optional(imx258->dev, "reset", GPIOD_OUT_HIGH);

Over 80, please wrap (there's another earlier, too).

> +	if (IS_ERR(imx258->reset_gpio))
> +		return dev_err_probe(imx258->dev, PTR_ERR(imx258->reset_gpio),
> +				     "Failed to get reset-gpios\n");
> +
>  	ret = imx258_get_regulators(imx258);
>  	if (ret)
>  		return dev_err_probe(imx258->dev, ret,
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: i2c: imx258: Add reset-gpio support

Posted by Muzaffer Kadir 4 weeks, 1 day ago

Thanks for review

On Fri, 28 Aug 2026 15:05:55 +0300, Sakari Ailus wrote:

>>  	if (ret) {
>>  		dev_err(dev, "failed to enable clock\n");
>>  		regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
>> +		return ret;
>> +	}
>> +
>> +	if (imx258->reset_gpio) {
>> +		ret = gpiod_set_value_cansleep(imx258->reset_gpio, 0);
>> +		if (ret) {
>> +			dev_err(dev, "failed to deassert reset\n");
>> +			clk_disable_unprepare(imx258->clk);
>> +			regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
>> +			return ret;
> 
> This warrants reworking error handling; please use gotos and move it to the
> end of the function. Same for clock error handling.
I will move error handling to goto by sending a v2.

>> +		usleep_range(400, 500);
> 
> The delay seems right. Can you use fsleep()?
I am going to replace it with fsleep(400) by v2

> In fact the delay should always have been there so this is a bugfix. It
> should go to a separate patch.
Then I will split this by making delay first patch
and reset handling second patch for v2.

>>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>>  	struct imx258 *imx258 = to_imx258(sd);
>>  
>> +	gpiod_set_value_cansleep(imx258->reset_gpio, 1);
>>  	clk_disable_unprepare(imx258->clk);
>>  	regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
>>  
>> @@ -1382,6 +1397,11 @@ static int imx258_probe(struct i2c_client *client)
>>  		return ret;
>>  	}
>>  
>> +	imx258->reset_gpio = devm_gpiod_get_optional(imx258->dev, "reset", GPIOD_OUT_HIGH);
> 
> Over 80, please wrap (there's another earlier, too).
I am going to wrap this by v2 but I couldn't spot the second one.
I think it is regulator_bulk_disable in error handling.
It should be lower than 80 after moving it to goto error handling.

-- 
Best Regards,

Muzaffer Kadir