Re: [PATCH v6 2/2] media: i2c: Add driver for IMX519 sensor

[Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>]

Hi Dave,

Quoting Dave Stevenson (2023-09-08 17:57:01)
> Hi Jacopo
> 
> On Fri, 8 Sept 2023 at 17:24, Jacopo Mondi
> <jacopo.mondi@xxxxxxxxxxxxxxxx> wrote:
> >
> > Hi Umang
> >
> > On Fri, Sep 08, 2023 at 08:43:44AM -0400, Umang Jain wrote:
> > > From: Lee Jackson <lee.jackson@xxxxxxxxxxx>
> > >

... <snip>

> > > +/* Power/clock management functions */
> > > +static int imx519_power_on(struct device *dev)
> > > +{
> > > +     struct i2c_client *client = to_i2c_client(dev);
> > > +     struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +     struct imx519 *imx519 = to_imx519(sd);
> > > +     int ret;
> > > +
> > > +     ret = regulator_bulk_enable(ARRAY_SIZE(imx519_supply_name),
> > > +                                 imx519->supplies);
> > > +     if (ret) {
> > > +             dev_err(&client->dev, "%s: failed to enable regulators\n",
> > > +                     __func__);
> > > +             return ret;
> > > +     }
> > > +
> > > +     ret = clk_prepare_enable(imx519->xclk);
> > > +     if (ret) {
> > > +             dev_err(&client->dev, "%s: failed to enable clock\n",
> > > +                     __func__);
> > > +             goto reg_off;
> > > +     }
> > > +
> > > +     gpiod_set_value_cansleep(imx519->reset_gpio, 1);
> >
> > Usually on power_on the reset/poweron gpios are set to logical 0.
> > If this works for you I think you need to invert the line polarity in
> > your .dts.
> 
> XCLR needs to go high for IMX519 to power on.
> I think it possibly depends on what you've named it - reset vs power_on
> 
> Quick sample (admittedly on 6.1 as that's what I happen to have):
> gpiod_set_value_cansleep(XXX_gpio, 1); in power_on.
> - imx219
> - imx214
> - imx274
> - imx334
> - imx335
> - ov7251
> 
> gpiod_set_value_cansleep(XXX_gpio, 0) in power_on
> - imx290
> - imx296
> - imx412

As this is a camera connected using the Raspberry Pi connector - I have
been looking in this area too to add support for an IMX283.

It occured to me that the GPIO line on the RPi Camera Connector seems to
be named 'POWER_EN' ... and is usually responsible for enabling the
power to the regulators on the camera module...

It seems to me more 'clean/clear' to do something like the following:


/ {
        /* 12 MHz Crystal on the camera module */
        imx283_inclk_0: imx283-inclk-12m {
                compatible = "fixed-clock";
                #clock-cells = <0>;
                status = "okay";
                clock-frequency = <12000000>;
        };

        reg_imx283_0_3v3: regulator-imx283-0-vdd3v3 {
                compatible = "regulator-fixed";
                pinctrl-names = "default";
                pinctrl-0 = <&pinctrl_csi0_pwdn>;
                regulator-min-microvolt = <3300000>;
                regulator-max-microvolt = <3300000>;
                regulator-name = "IMX283_0_POWER";
                gpio = <&gpio2 11 GPIO_ACTIVE_HIGH>;
                vin-supply = <&reg_csi1_3v3>;
                startup-delay-us = <300000>;
                enable-active-high;
        };
};


&i2c2 {
        sensor@1a {
                compatible = "sony,imx283";
                reg = <0x1a>;

                clocks = <&imx283_inclk_0>;
                clock-names = "xclk";

                rotation = <180>;
                orientation = <0>;

                status = "okay";

                VANA-supply = <&reg_imx283_0_3v3>; /* Analog 2.8v */
                VDIG-supply = <&reg_imx283_0_3v3>; /* Digital Core 1.05v */
                VDDL-supply = <&reg_imx283_0_3v3>; /* IF 1.8v */

                port {
                        imx283_0_ep: endpoint {
                                remote-endpoint = <&mipi_csi_0_in>;
                                clock-lanes = <0>;
                                data-lanes = <1 2 3 4>;
                        };
                };
        };
};


I wondered if that's more clear than using a 'reset' line which isn't
actually what the schematics show.




> > > +     usleep_range(IMX519_XCLR_MIN_DELAY_US,
> > > +                  IMX519_XCLR_MIN_DELAY_US + IMX519_XCLR_DELAY_RANGE_US);
> >
> > fsleep() will do
> >
> > > +
> > > +     return 0;
> > > +
> > > +reg_off:
> > > +     regulator_bulk_disable(ARRAY_SIZE(imx519_supply_name), imx519->supplies);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static int imx519_power_off(struct device *dev)
> > > +{
> > > +     struct i2c_client *client = to_i2c_client(dev);
> > > +     struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +     struct imx519 *imx519 = to_imx519(sd);
> > > +
> > > +     gpiod_set_value_cansleep(imx519->reset_gpio, 0);
> > > +     regulator_bulk_disable(ARRAY_SIZE(imx519_supply_name), imx519->supplies);
> > > +     clk_disable_unprepare(imx519->xclk);
> >
> > Usually, the reverse power up sequence is used. It shouldn't make any
> > difference, unless the datasheet prescribes this sequence.
> 
> I'd agree.
> T1 from XCLR falling to VANA/VDIG/VIF falling is 0us, so XCLR must go
> low first. VANA, VDIG, and VIF can then fall in any order.
> 
> The diagram shows INCK stopping before XCLR is dropped.
> Driving a clock signal into powered down electronics is generally "a
> bad thing", so the clock should be stopped before the regulators are
> killed.
> 
> Again this is copied from imx477. Our modules don't matter as all 3
> regulators, clock, and XCLR are sequenced off one GPIO.

Indeed, that's the part that makes me think modelling the regulator part
rather than a reset gpio could potentially make sense, but I'm sure this
is one of those scenarios that someone is about to shoot down my idea
;-)

--
Kieran