| Message ID | 20220415115954.1649217-2-bryan.odonoghue@linaro.org |
|---|---|
| State | Accepted |
| Commit | bb25f071fc92d3d227178a45853347c7b3b45a6b |
| Headers | show |
| Series | media: i2c: imx412: Add regulator control to imx412 | expand |
Hi Bryan, On Fri, 2022-04-15 at 12:59 +0100, Bryan O'Donoghue wrote: > The imx412/imx577 sensor has a reset line that is active low not active > high. Currently the logic for this is inverted. > > The right way to define the reset line is to declare it active low in the > DTS and invert the logic currently contained in the driver. > > The DTS should represent the hardware does i.e. reset is active low. > So: > + reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>; > not: > - reset-gpios = <&tlmm 78 GPIO_ACTIVE_HIGH>; > > I was a bit reticent about changing this logic since I thought it might > negatively impact @intel.com users. Googling a bit though I believe this > sensor is used on "Keem Bay" which is clearly a DTS based system and is not > upstream yet. Thanks for the fix and sorry for the late reply (I've been off for the last 2 weeks). The fix looks good to me and it will not negatively affect us (I appreciate the consideration!). Reviewed-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com> > > Fixes: 9214e86c0cc1 ("media: i2c: Add imx412 camera sensor driver") > Cc: stable@vger.kernel.org > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > drivers/media/i2c/imx412.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c > index be3f6ea55559..e6be6b4250f5 100644 > --- a/drivers/media/i2c/imx412.c > +++ b/drivers/media/i2c/imx412.c > @@ -1011,7 +1011,7 @@ static int imx412_power_on(struct device *dev) > struct imx412 *imx412 = to_imx412(sd); > int ret; > > - gpiod_set_value_cansleep(imx412->reset_gpio, 1); > + gpiod_set_value_cansleep(imx412->reset_gpio, 0); > > ret = clk_prepare_enable(imx412->inclk); > if (ret) { > @@ -1024,7 +1024,7 @@ static int imx412_power_on(struct device *dev) > return 0; > > error_reset: > - gpiod_set_value_cansleep(imx412->reset_gpio, 0); > + gpiod_set_value_cansleep(imx412->reset_gpio, 1); > > return ret; > } > @@ -1040,7 +1040,7 @@ static int imx412_power_off(struct device *dev) > struct v4l2_subdev *sd = dev_get_drvdata(dev); > struct imx412 *imx412 = to_imx412(sd); > > - gpiod_set_value_cansleep(imx412->reset_gpio, 0); > + gpiod_set_value_cansleep(imx412->reset_gpio, 1); > > clk_disable_unprepare(imx412->inclk); >
diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c index be3f6ea55559..e6be6b4250f5 100644 --- a/drivers/media/i2c/imx412.c +++ b/drivers/media/i2c/imx412.c @@ -1011,7 +1011,7 @@ static int imx412_power_on(struct device *dev) struct imx412 *imx412 = to_imx412(sd); int ret; - gpiod_set_value_cansleep(imx412->reset_gpio, 1); + gpiod_set_value_cansleep(imx412->reset_gpio, 0); ret = clk_prepare_enable(imx412->inclk); if (ret) { @@ -1024,7 +1024,7 @@ static int imx412_power_on(struct device *dev) return 0; error_reset: - gpiod_set_value_cansleep(imx412->reset_gpio, 0); + gpiod_set_value_cansleep(imx412->reset_gpio, 1); return ret; } @@ -1040,7 +1040,7 @@ static int imx412_power_off(struct device *dev) struct v4l2_subdev *sd = dev_get_drvdata(dev); struct imx412 *imx412 = to_imx412(sd); - gpiod_set_value_cansleep(imx412->reset_gpio, 0); + gpiod_set_value_cansleep(imx412->reset_gpio, 1); clk_disable_unprepare(imx412->inclk);
The imx412/imx577 sensor has a reset line that is active low not active high. Currently the logic for this is inverted. The right way to define the reset line is to declare it active low in the DTS and invert the logic currently contained in the driver. The DTS should represent the hardware does i.e. reset is active low. So: + reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>; not: - reset-gpios = <&tlmm 78 GPIO_ACTIVE_HIGH>; I was a bit reticent about changing this logic since I thought it might negatively impact @intel.com users. Googling a bit though I believe this sensor is used on "Keem Bay" which is clearly a DTS based system and is not upstream yet. Fixes: 9214e86c0cc1 ("media: i2c: Add imx412 camera sensor driver") Cc: stable@vger.kernel.org Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/media/i2c/imx412.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)