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
>>
