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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <adfb8732-3ac7-4496-9671-1fa2aa71139c@kernel.org>
Date: Sun, 17 Aug 2025 08:13:01 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Will Whang <will@...lwhang.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
 Sascha Hauer <s.hauer@...gutronix.de>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 3/4] media: i2c: imx585: Add Sony IMX585 image-sensor
 driver

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