Re: [PATCH v2 2/2] media: i2c: add ov2735 image sensor driver

[Krzysztof Kozlowski <krzk@kernel.org>]

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