| Message ID | 20240729110437.199428-3-umang.jain@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 99d30e2fdea4086be4e66e2deb10de854b547ab8 |
| Headers | show |
| Series | media: imx335: Fix reset-gpio handling | expand |
Hi Laurent, Krzysztof, On Wed, Jul 31, 2024 at 12:39:05PM +0300, Laurent Pinchart wrote: > On Wed, Jul 31, 2024 at 11:06:38AM +0200, Krzysztof Kozlowski wrote: > > On 31/07/2024 11:02, Kieran Bingham wrote: > > > Quoting Umang Jain (2024-07-31 06:41:35) > > >> On 30/07/24 2:40 pm, Laurent Pinchart wrote: > > >>> On Tue, Jul 30, 2024 at 10:42:01AM +0200, Krzysztof Kozlowski wrote: > > >>>> On 30/07/2024 10:24, Sakari Ailus wrote: > > >>>>> Hi Krzysztof, > > >>>>> > > >>>>> On Mon, Jul 29, 2024 at 04:09:39PM +0200, Krzysztof Kozlowski wrote: > > >>>>>> On 29/07/2024 13:04, Umang Jain wrote: > > >>>>>>> Rectify the logical value of reset-gpio so that it is set to > > >>>>>>> 0 (disabled) during power-on and to 1 (enabled) during power-off. > > >>>>>>> > > >>>>>>> Meanwhile at it, set the reset-gpio to GPIO_OUT_HIGH at initialization > > >>>>>>> time to make sure it starts off in reset. > > >>>>>>> > > >>>>>>> Fixes: 45d19b5fb9ae ("media: i2c: Add imx335 camera sensor driver") > > >>>>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > >>>>>>> --- > > >>>>>>> drivers/media/i2c/imx335.c | 8 ++++---- > > >>>>>>> 1 file changed, 4 insertions(+), 4 deletions(-) > > >>>>>>> > > >>>>>> This will break all the users, so no. At least not without mentioning > > >>>>>> ABI break and some sort of investigating how customers or users are > > >>>>>> affected. > > >>>>> > > >>>>> I know the original authors aren't using the driver anymore and it took > > >>>>> quite a bit of time until others started to contribute to it so I suspect > > >>>>> the driver hasn't been in use for that long. There are no instances of the > > >>>>> device in the in-kernel DTS either. > > >>>>> > > >>>>> Any DTS author should have also noticed the issue but of course there's a > > >>>>> risk someone could have just changed the polarity and not bothered to chech > > >>>>> what it was supposed to be. > > >>>>> > > >>>>> I agree the commit message should be more vocal about the effects on > > >>>>> existing DTS. > > >>>> > > >>>> I can imagine that all users (out of tree, in this case) inverted > > >>>> polarity in DTS based on what's implemented. You could go with some > > >>>> trivial hack, like I did for one of codecs - see 738455858a2d ("ASoC: > > >>>> codecs: wsa881x: Use proper shutdown GPIO polarity"), but I remember > > >>>> Mark Brown rejected similar commit for newer drivers. > > >>> > > >>> I don't think there's any out-of-tree user, because when we started > > >>> using the recently driver, it required lots of fixes to even work at > > >>> all. I'll let Kieran and Umang comment on that, I haven't follow the > > >>> development in details. > > >> > > >> indeed, initially we had to put up fixes like : > > >> > > >> 14a60786d72e ("media: imx335: Set reserved register to default value") > > >> 81495a59baeba ("media: imx335: Fix active area height discrepency") > > >> > > >> to make the sensor work properly on our platforms. Only after that we > > >> had a base to support more capabilities on the sensor (multiple lanes > > >> support, flips, TPG etc.) > > > > > > I would also add that we had to provide control for the regulators to be > > > able to power the device as well in fea91ee73b7c ("media: i2c: imx335: > > > Enable regulator supplies"). > > > > Hm? That's not a proof of anything. Supplies are often turned on by default. > > > > > Given the driver was posted from Intel, I would have anticipated perhaps > > > the driver was in fact only actually tested by Intel on ACPI platforms - > > > yet with no ACPI table registered in the driver - even that could likely > > > be considered broken. > > > > Nope, that does not work like that. Their platforms and such sensors are > > often used on DT based boards. > > What DT-platforms would that be ? > > > Not mentioning even PRP0001. > > I don't think PRP0001 is used by Intel for camera sensors. The original author is no longer using the driver nor it is used for its original purpose any more. The next users were quite probably Kieran and Umang late last year. > > Sakari, do you have any information about all this ? Do you think > there's a risk that the driver is currently used by anyone with a > mainline kernel ? I think it's extremely unlikely the driver has been or continues to be in use on ACPI based systems.
diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c index cd150606a8a9..7241fc87ef84 100644 --- a/drivers/media/i2c/imx335.c +++ b/drivers/media/i2c/imx335.c @@ -1057,7 +1057,7 @@ static int imx335_parse_hw_config(struct imx335 *imx335) /* Request optional reset pin */ imx335->reset_gpio = devm_gpiod_get_optional(imx335->dev, "reset", - GPIOD_OUT_LOW); + GPIOD_OUT_HIGH); if (IS_ERR(imx335->reset_gpio)) { dev_err(imx335->dev, "failed to get reset gpio %ld\n", PTR_ERR(imx335->reset_gpio)); @@ -1171,7 +1171,7 @@ static int imx335_power_on(struct device *dev) usleep_range(500, 550); /* Tlow */ /* Set XCLR */ - gpiod_set_value_cansleep(imx335->reset_gpio, 1); + gpiod_set_value_cansleep(imx335->reset_gpio, 0); ret = clk_prepare_enable(imx335->inclk); if (ret) { @@ -1184,7 +1184,7 @@ static int imx335_power_on(struct device *dev) return 0; error_reset: - gpiod_set_value_cansleep(imx335->reset_gpio, 0); + gpiod_set_value_cansleep(imx335->reset_gpio, 1); regulator_bulk_disable(ARRAY_SIZE(imx335_supply_name), imx335->supplies); return ret; @@ -1201,7 +1201,7 @@ static int imx335_power_off(struct device *dev) struct v4l2_subdev *sd = dev_get_drvdata(dev); struct imx335 *imx335 = to_imx335(sd); - gpiod_set_value_cansleep(imx335->reset_gpio, 0); + gpiod_set_value_cansleep(imx335->reset_gpio, 1); clk_disable_unprepare(imx335->inclk); regulator_bulk_disable(ARRAY_SIZE(imx335_supply_name), imx335->supplies);
Rectify the logical value of reset-gpio so that it is set to 0 (disabled) during power-on and to 1 (enabled) during power-off. Meanwhile at it, set the reset-gpio to GPIO_OUT_HIGH at initialization time to make sure it starts off in reset. Fixes: 45d19b5fb9ae ("media: i2c: Add imx335 camera sensor driver") Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- drivers/media/i2c/imx335.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)