From: Kieran Bingham <kieran.bingham@ideasonboard.com> To: Dave Stevenson <dave.stevenson@raspberrypi.com>, Jacopo Mondi <jacopo.mondi@ideasonboard.com> Cc: Umang Jain <umang.jain@ideasonboard.com>, linux-media@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Lee Jackson <lee.jackson@arducam.com>, Sakari Ailus <sakari.ailus@linux.intel.com>, Hans Verkuil <hverkuil-cisco@xs4all.nl>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Shawn Tu <shawnx.tu@intel.com> Subject: Re: [PATCH v6 2/2] media: i2c: Add driver for IMX519 sensor Date: Fri, 08 Sep 2023 18:29:56 +0100 [thread overview] Message-ID: <169419419630.3437168.7121485951334169757@ping.linuxembedded.co.uk> (raw) In-Reply-To: <CAPY8ntCYz3D77R7G+JTKvOWW4uNTj-wKrsjJ7ASw_RoN4FU-ow@mail.gmail.com> 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@ideasonboard.com> wrote: > > > > Hi Umang > > > > On Fri, Sep 08, 2023 at 08:43:44AM -0400, Umang Jain wrote: > > > From: Lee Jackson <lee.jackson@arducam.com> > > > ... <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 = <®_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 = <®_imx283_0_3v3>; /* Analog 2.8v */ VDIG-supply = <®_imx283_0_3v3>; /* Digital Core 1.05v */ VDDL-supply = <®_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
WARNING: multiple messages have this Message-ID (diff)
From: Kieran Bingham <kieran.bingham@ideasonboard.com> To: Dave Stevenson <dave.stevenson@raspberrypi.com>, Jacopo Mondi <jacopo.mondi@ideasonboard.com> Cc: Umang Jain <umang.jain@ideasonboard.com>, linux-media@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Lee Jackson <lee.jackson@arducam.com>, Sakari Ailus <sakari.ailus@linux.intel.com>, Hans Verkuil <hverkuil-cisco@xs4all.nl>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Shawn Tu <shawnx.tu@intel.com> Subject: Re: [PATCH v6 2/2] media: i2c: Add driver for IMX519 sensor Date: Fri, 08 Sep 2023 18:29:56 +0100 [thread overview] Message-ID: <169419419630.3437168.7121485951334169757@ping.linuxembedded.co.uk> (raw) In-Reply-To: <CAPY8ntCYz3D77R7G+JTKvOWW4uNTj-wKrsjJ7ASw_RoN4FU-ow@mail.gmail.com> 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@ideasonboard.com> wrote: > > > > Hi Umang > > > > On Fri, Sep 08, 2023 at 08:43:44AM -0400, Umang Jain wrote: > > > From: Lee Jackson <lee.jackson@arducam.com> > > > ... <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 = <®_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 = <®_imx283_0_3v3>; /* Analog 2.8v */ VDIG-supply = <®_imx283_0_3v3>; /* Digital Core 1.05v */ VDDL-supply = <®_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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-09-08 17:30 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-09-08 12:43 [PATCH v6 0/2] media: i2c: imx519: Support for Sony IMX519 sensor Umang Jain 2023-09-08 12:43 ` Umang Jain 2023-09-08 12:43 ` [PATCH v6 1/2] dt-bindings: media: i2c: Add IMX519 CMOS sensor Umang Jain 2023-09-08 12:43 ` Umang Jain 2023-09-08 12:43 ` [PATCH v6 2/2] media: i2c: Add driver for IMX519 sensor Umang Jain 2023-09-08 12:43 ` Umang Jain 2023-09-08 15:18 ` Dave Stevenson 2023-09-08 15:18 ` Dave Stevenson 2023-09-08 16:23 ` Jacopo Mondi 2023-09-08 16:23 ` Jacopo Mondi 2023-09-08 16:57 ` Dave Stevenson 2023-09-08 16:57 ` Dave Stevenson 2023-09-08 17:29 ` Kieran Bingham [this message] 2023-09-08 17:29 ` Kieran Bingham 2023-09-08 17:49 ` Dave Stevenson 2023-09-08 17:49 ` Dave Stevenson 2023-09-08 17:26 ` Laurent Pinchart 2023-09-08 17:26 ` Laurent Pinchart
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=169419419630.3437168.7121485951334169757@ping.linuxembedded.co.uk \ --to=kieran.bingham@ideasonboard.com \ --cc=dave.stevenson@raspberrypi.com \ --cc=devicetree@vger.kernel.org \ --cc=hverkuil-cisco@xs4all.nl \ --cc=jacopo.mondi@ideasonboard.com \ --cc=laurent.pinchart@ideasonboard.com \ --cc=lee.jackson@arducam.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=sakari.ailus@linux.intel.com \ --cc=shawnx.tu@intel.com \ --cc=umang.jain@ideasonboard.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.