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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 16/08/2025 21:44, Will Whang wrote:
> On Mon, Aug 11, 2025 at 1:06 AM Krzysztof Kozlowski <krzk@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

This is not how resets work. Logical reset value high means it is
asserted and device does not work.

Read carefully your datasheet. Because if you claim above this is not a
reset line, but some other.

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

Great, so 10 year old buggy code is the example, but all other - 90% of
correct drivers - are not?

> 
> 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 explained to you. You are mixing logical level with line level. This
is the problem, because (again I said it):

It does not work.

Your code or your DTS is incorrect.



Best regards,
Krzysztof