Re: [PATCH v3 23/25] drivers: media: i2c: imx258: Add support for powerdown gpio

[Luis Garcia]

On 4/4/24 08:12, Dave Stevenson wrote:
> Hi Luigi
> 
> On Wed, 3 Apr 2024 at 20:34, Luigi311 <git@xxxxxxxxxxxx> wrote:
>>
>> On 4/3/24 10:57, Ondřej Jirman wrote:
>>> Hi Sakari and Luis,
>>>
>>> On Wed, Apr 03, 2024 at 04:25:41PM GMT, Sakari Ailus wrote:
>>>> Hi Luis, Ondrej,
>>>>
>>>> On Wed, Apr 03, 2024 at 09:03:52AM -0600, git@xxxxxxxxxxxx wrote:
>>>>> From: Luis Garcia <git@xxxxxxxxxxxx>
>>>>>
>>>>> On some boards powerdown signal needs to be deasserted for this
>>>>> sensor to be enabled.
>>>>>
>>>>> Signed-off-by: Ondrej Jirman <megi@xxxxxx>
>>>>> Signed-off-by: Luis Garcia <git@xxxxxxxxxxxx>
>>>>> ---
>>>>>  drivers/media/i2c/imx258.c | 13 +++++++++++++
>>>>>  1 file changed, 13 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>>>>> index 30352c33f63c..163f04f6f954 100644
>>>>> --- a/drivers/media/i2c/imx258.c
>>>>> +++ b/drivers/media/i2c/imx258.c
>>>>> @@ -679,6 +679,8 @@ struct imx258 {
>>>>>     unsigned int lane_mode_idx;
>>>>>     unsigned int csi2_flags;
>>>>>
>>>>> +   struct gpio_desc *powerdown_gpio;
>>>>> +
>>>>>     /*
>>>>>      * Mutex for serialized access:
>>>>>      * Protect sensor module set pad format and start/stop streaming safely.
>>>>> @@ -1213,6 +1215,8 @@ static int imx258_power_on(struct device *dev)
>>>>>     struct imx258 *imx258 = to_imx258(sd);
>>>>>     int ret;
>>>>>
>>>>> +   gpiod_set_value_cansleep(imx258->powerdown_gpio, 0);
>>>>
>>>> What does the spec say? Should this really happen before switching on the
>>>> supplies below?
>>>
>>> There's no powerdown input in the IMX258 manual. The manual only mentions
>>> that XCLR (reset) should be held low during power on.
>>>
>>> https://megous.com/dl/tmp/15b0992a720ab82d.png
>>>
>>> https://megous.com/dl/tmp/f2cc991046d97641.png
>>>
>>>    This sensor doesn’t have a built-in “Power ON Reset” function. The XCLR pin
>>>    is set to “LOW” and the power supplies are brought up. Then the XCLR pin
>>>    should be set to “High” after INCK supplied.
>>>
>>> So this input is some feature on camera module itself outside of the
>>> IMX258 chip, which I think is used to gate power to the module. Eg. on Pinephone
>>> Pro, there are two modules with shared power rails, so enabling supply to
>>> one module enables it to the other one, too. So this input becomes the only way
>>> to really enable/disable power to the chip when both are used at once at some
>>> point, because regulator_bulk_enable/disable becomes ineffective at that point.
>>>
>>> Luis, maybe you saw some other datasheet that mentions this input? IMO,
>>> it just gates the power rails via some mosfets on the module itself, since
>>> there's not power down input to the chip itself.
>>>
>>> kind regards,
>>>       o.
>>>
>>
>> Ondrej, I did not see anything else in the datasheet since I'm pretty sure
>> I'm looking at the same datasheet as it was supplied to me by Pine64. I'm
>> not sure what datasheet Dave has access to since he got his for a
>> completely different module than what we are testing with though.
> 
> I only have a leaked datasheet (isn't the internet wonderful!)  [1]
> XCLR is documented in that, as Ondrej has said.
> 
> If this powerdown GPIO is meant to be driving XCLR, then it is in the
> wrong order against the supplies.
> 
> This does make me confused over the difference between this powerdown
> GPIO and the reset GPIO that you implement in 24/25.
> 
> Following the PinePhone Pro DT [3] and schematics [4]
> reset-gpios = <&gpio1 RK_PA0 GPIO_ACTIVE_LOW>;
> powerdown-gpios = <&gpio2 RK_PD4 GPIO_ACTIVE_HIGH>;
> 
> Schematic page 11 upper right block
> GPIO1_A0/ISP0_SHUTTER_EN/ISP1_SHUTTER_EN/TCPD_VBUS_SINK_EN_d becomes
> Camera_RST_L. Page 18 feeds that through to the RESET on the camera
> connector.
> Page 11 left middle block GPIO2_D4/SDIO0_BKPWR_d becomes DVP_PDN1_H.
> Page 18 feeds that through to the PWDN on the camera connector.
> 
> Seeing as we apparently have a lens driver kicking around as well,
> potentially one is reset to the VCM, and one to the sensor? DW9714
> does have an XSD shutdown pin.
> Only the module integrator is going to really know the answer,
> although potentially a little poking with gpioset and i2cdetect may
> tell you more.
> 
>   Dave
> 
> [1] https://web.archive.org/web/20201027131326/www.hi.app/IMX258-datasheet.pdf
> [2] https://files.pine64.org/doc/PinePhonePro/PinephonePro-Schematic-V1.0-20211127.pdf
> [3] https://xff.cz/git/linux/tree/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts?h=orange-pi-5.18#n868
> [4] https://files.pine64.org/doc/PinePhonePro/PinephonePro-Schematic-V1.0-20211127.pdf
> 
> 

Out of curiosity I dropped this and tested it on my PPP and it still loads
up the camera correctly so I am fine with dropping this and patch 22 that
adds in the dt binding

>>>>> +
>>>>>     ret = regulator_bulk_enable(IMX258_NUM_SUPPLIES,
>>>>>                                 imx258->supplies);
>>>>>     if (ret) {
>>>>> @@ -1224,6 +1228,7 @@ static int imx258_power_on(struct device *dev)
>>>>>     ret = clk_prepare_enable(imx258->clk);
>>>>>     if (ret) {
>>>>>             dev_err(dev, "failed to enable clock\n");
>>>>> +           gpiod_set_value_cansleep(imx258->powerdown_gpio, 1);
>>>>>             regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
>>>>>     }
>>>>>
>>>>> @@ -1238,6 +1243,8 @@ static int imx258_power_off(struct device *dev)
>>>>>     clk_disable_unprepare(imx258->clk);
>>>>>     regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
>>>>>
>>>>> +   gpiod_set_value_cansleep(imx258->powerdown_gpio, 1);
>>>>> +
>>>>>     return 0;
>>>>>  }
>>>>>
>>>>> @@ -1541,6 +1548,12 @@ static int imx258_probe(struct i2c_client *client)
>>>>>     if (!imx258->variant_cfg)
>>>>>             imx258->variant_cfg = &imx258_cfg;
>>>>>
>>>>> +   /* request optional power down pin */
>>>>> +   imx258->powerdown_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
>>>>> +                                               GPIOD_OUT_HIGH);
>>>>> +   if (IS_ERR(imx258->powerdown_gpio))
>>>>> +           return PTR_ERR(imx258->powerdown_gpio);
>>>>> +
>>>>>     /* Initialize subdev */
>>>>>     v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops);
>>>>>
>>>>
>>>> --
>>>> Regards,
>>>>
>>>> Sakari Ailus
>>