[PATCH v2 3/4] media: i2c: imx585: Add Sony IMX585 image-sensor driver

[Will Whang]

On Mon, Aug 11, 2025 at 1:06 AM Krzysztof Kozlowski <krzk at kernel.org> wrote:
>
> On Sun, Aug 10, 2025 at 11:09:20PM +0100, Will Whang wrote:
> > +
> > +/* --------------------------------------------------------------------------
> > + * Power / runtime PM
> > + * --------------------------------------------------------------------------
> > + */
> > +
> > +static int imx585_power_on(struct device *dev)
> > +{
> > +     struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > +     struct imx585 *imx585 = to_imx585(sd);
> > +     int ret;
> > +
> > +     dev_dbg(imx585->clientdev, "power_on\n");
> > +
> > +     ret = regulator_bulk_enable(IMX585_NUM_SUPPLIES, imx585->supplies);
> > +     if (ret) {
> > +             dev_err(imx585->clientdev, "Failed to enable regulators\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = clk_prepare_enable(imx585->xclk);
> > +     if (ret) {
> > +             dev_err(imx585->clientdev, "Failed to enable clock\n");
> > +             goto reg_off;
> > +     }
> > +
> > +     gpiod_set_value_cansleep(imx585->reset_gpio, 1);
>
> You asserted reset gpio causing it to enter reset and you call this
> "power on"?
>
> > +     usleep_range(IMX585_XCLR_MIN_DELAY_US,
> > +                  IMX585_XCLR_MIN_DELAY_US + IMX585_XCLR_DELAY_RANGE_US);
> > +     return 0;
> > +
> > +reg_off:
> > +     regulator_bulk_disable(IMX585_NUM_SUPPLIES, imx585->supplies);
> > +     return ret;
> > +}
> > +
> > +static int imx585_power_off(struct device *dev)
> > +{
> > +     struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > +     struct imx585 *imx585 = to_imx585(sd);
> > +
> > +     dev_dbg(imx585->clientdev, "power_off\n");
> > +
> > +     gpiod_set_value_cansleep(imx585->reset_gpio, 0);
>
> And here device comes up, but you call it power off? Your functions or
> reset gpio code are completely reversed/wrong.

Reset pin High -> Run normally
Reset pin Low -> Reset state

See drivers/media/i2c/imx219.c with the same logic:

static int imx219_power_on(struct device *dev)
{
struct v4l2_subdev *sd = dev_get_drvdata(dev);
struct imx219 *imx219 = to_imx219(sd);
int ret;

ret = regulator_bulk_enable(IMX219_NUM_SUPPLIES,
   imx219->supplies);
if (ret) {
dev_err(dev, "%s: failed to enable regulators\n",
__func__);
return ret;
}

ret = clk_prepare_enable(imx219->xclk);
if (ret) {
dev_err(dev, "%s: failed to enable clock\n",
__func__);
goto reg_off;
}

gpiod_set_value_cansleep(imx219->reset_gpio, 1);
usleep_range(IMX219_XCLR_MIN_DELAY_US,
    IMX219_XCLR_MIN_DELAY_US + IMX219_XCLR_DELAY_RANGE_US);

return 0;

reg_off:
regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);

return ret;
}

static int imx219_power_off(struct device *dev)
{
struct v4l2_subdev *sd = dev_get_drvdata(dev);
struct imx219 *imx219 = to_imx219(sd);

gpiod_set_value_cansleep(imx219->reset_gpio, 0);
regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);
clk_disable_unprepare(imx219->xclk);

return 0;
}

I really don't understand why this is a problem, it is up to the chip
designer to decide
what to do with reset behavior and not the reviewers.

I'm simply writing the code following the datasheets here.


> Best regards,
> Krzysztof
>