linux-kernel - Re: [PATCH v2 2/2] media: i2c: add ov2735 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 for Android: free password hash cracker in your pocket

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

Message-ID: <550105c6-1520-4f2b-ae4f-0f135a8f3b5f@kernel.org>
Date: Tue, 8 Jul 2025 15:26:13 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>,
 "sakari.ailus@...ux.intel.com" <sakari.ailus@...ux.intel.com>,
 "andriy.shevchenko@...ux.intel.com" <andriy.shevchenko@...ux.intel.com>,
 "krzk+dt@...nel.org" <krzk+dt@...nel.org>
Cc: Himanshu Bhavani <himanshu.bhavani@...iconsignals.io>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Hans Verkuil <hverkuil@...all.nl>,
 André Apitzsch <git@...tzsch.eu>,
 Hans de Goede <hansg@...nel.org>, Ricardo Ribalda <ribalda@...omium.org>,
 Heimir Thor Sverrisson <heimir.sverrisson@...il.com>,
 Matthias Fend <matthias.fend@...end.at>,
 Benjamin Mugnier <benjamin.mugnier@...s.st.com>,
 Jingjing Xiong <jingjing.xiong@...el.com>,
 Sylvain Petinot <sylvain.petinot@...s.st.com>,
 Dongcheng Yan <dongcheng.yan@...el.com>, Arnd Bergmann <arnd@...db.de>,
 Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
 "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] media: i2c: add ov2735 image sensor driver

On 08/07/2025 13:28, Hardevsinh Palaniya wrote:
>> On 08/07/2025 12:25, Hardevsinh Palaniya wrote:
>>> +
>>> +static const struct v4l2_subdev_ops ov2735_subdev_ops = {
>>> +     .video = &ov2735_video_ops,
>>> +     .pad = &ov2735_pad_ops,
>>> +};
>>> +
>>> +static const struct v4l2_subdev_internal_ops ov2735_internal_ops = {
>>> +     .init_state = ov2735_init_state,
>>> +};
>>> +
>>> +static int ov2735_power_on(struct device *dev)
>>> +{
>>> +     struct v4l2_subdev *sd = dev_get_drvdata(dev);
>>> +     struct ov2735 *ov2735 = to_ov2735(sd);
>>> +     unsigned long delay_us;
>>> +     int ret;
>>> +
>>> +     gpiod_set_value_cansleep(ov2735->pwdn_gpio, 0);
>>
>> So you power up here...
>>
>>> +     fsleep(3000);
>>> +
>>> +     ret = regulator_bulk_enable(ARRAY_SIZE(ov2735_supply_name),
>>> +                                 ov2735->supplies);
>>> +     if (ret) {
>>> +             dev_err(ov2735->dev, "failed to enable regulators\n");
>>> +             return ret;
>>> +     }
>>> +
>>> +     gpiod_set_value_cansleep(ov2735->pwdn_gpio, 1);
>>
>> And here you power down your device. This makes no sense... unless this
>> is not a power down GPIO.
>  
> In Datasheet 
> Power-up sequence:
> 
> 	  |--------------------------------------
> avdd  ----|
> 	   <-t1->
> 		|--------------------------------
> dovdd  ---------|
> 		  <-T2->
> 			|--------------------------
> dvdd  ------------------|
> 			 <-T3->
>           |---------------------|
> pwdn -----|			|-------------------

So that is not really a typical power down pin and this should be
explained clearly in property description. It rather feels like power-on
pin, which you need to pull for some time to boot the process and later
the supplies are keeping it on.

Best regards,
Krzysztof