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
