diff mbox series

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

Message ID 20240403150355.189229-24-git@luigi311.com (mailing list archive)
State Superseded
Headers
Series imx258 improvement series |

Commit Message

Luis Garcia April 3, 2024, 3:03 p.m. UTC 
  From: Luis Garcia <git@luigi311.com>

On some boards powerdown signal needs to be deasserted for this
sensor to be enabled.

Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Luis Garcia <git@luigi311.com>
---
 drivers/media/i2c/imx258.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Sakari Ailus April 3, 2024, 4:25 p.m. UTC | #1 
Hi Luis, Ondrej,

On Wed, Apr 03, 2024 at 09:03:52AM -0600, git@luigi311.com wrote:
> From: Luis Garcia <git@luigi311.com>
> 
> On some boards powerdown signal needs to be deasserted for this
> sensor to be enabled.
> 
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Luis Garcia <git@luigi311.com>
> ---
>  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?

> +
>  	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);
>
Ondřej Jirman April 3, 2024, 4:57 p.m. UTC | #2 
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@luigi311.com wrote:
> > From: Luis Garcia <git@luigi311.com>
> > 
> > On some boards powerdown signal needs to be deasserted for this
> > sensor to be enabled.
> > 
> > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> > Signed-off-by: Luis Garcia <git@luigi311.com>
> > ---
> >  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.

> > +
> >  	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
Luis Garcia April 3, 2024, 7:34 p.m. UTC | #3 
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@luigi311.com wrote:
>>> From: Luis Garcia <git@luigi311.com>
>>>
>>> On some boards powerdown signal needs to be deasserted for this
>>> sensor to be enabled.
>>>
>>> Signed-off-by: Ondrej Jirman <megi@xff.cz>
>>> Signed-off-by: Luis Garcia <git@luigi311.com>
>>> ---
>>>  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.

>>> +
>>>  	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
Sakari Ailus April 4, 2024, 7:07 a.m. UTC | #4 
Hi Ondřej,

On Wed, Apr 03, 2024 at 06:57:56PM +0200, 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@luigi311.com wrote:
> > > From: Luis Garcia <git@luigi311.com>
> > > 
> > > On some boards powerdown signal needs to be deasserted for this
> > > sensor to be enabled.
> > > 
> > > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> > > Signed-off-by: Luis Garcia <git@luigi311.com>
> > > ---
> > >  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.

The hardware designers sometimes do not consider the difficulty of
implementing software support for their choice of design. Maybe this could
be modelled as a GPIO regulator? But it'd depend on another regulator and
I'm not sure that is covered.

The camera sensor generally consume very little power in software standby
mode so the potential saving here are probably irrelevant.

> 
> 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.
> 
> > > +
> > >  	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
Dave Stevenson April 4, 2024, 2:12 p.m. UTC | #5 
Hi Luigi

On Wed, 3 Apr 2024 at 20:34, Luigi311 <git@luigi311.com> 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@luigi311.com wrote:
> >>> From: Luis Garcia <git@luigi311.com>
> >>>
> >>> On some boards powerdown signal needs to be deasserted for this
> >>> sensor to be enabled.
> >>>
> >>> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> >>> Signed-off-by: Luis Garcia <git@luigi311.com>
> >>> ---
> >>>  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


> >>> +
> >>>     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
>
Luis Garcia April 4, 2024, 11:18 p.m. UTC | #6 
On 4/4/24 08:12, Dave Stevenson wrote:
> Hi Luigi
> 
> On Wed, 3 Apr 2024 at 20:34, Luigi311 <git@luigi311.com> 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@luigi311.com wrote:
>>>>> From: Luis Garcia <git@luigi311.com>
>>>>>
>>>>> On some boards powerdown signal needs to be deasserted for this
>>>>> sensor to be enabled.
>>>>>
>>>>> Signed-off-by: Ondrej Jirman <megi@xff.cz>
>>>>> Signed-off-by: Luis Garcia <git@luigi311.com>
>>>>> ---
>>>>>  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
>>
diff mbox series

Patch

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);
+
 	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);