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

[Sakari Ailus]

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@xxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxx
> Signed-off-by: Muzaffer Kadir <muzafferkadir@xxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx/
> 
> 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