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

[Muzaffer Kadir <muzafferkadir@mainlining.org>]

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