On 14/04/2022 15:16, Sakari Ailus wrote:
On Thu, Apr 14, 2022 at 03:04:10PM +0100, Bryan O'Donoghue wrote:
On 14/04/2022 14:56, Sakari Ailus wrote:
On Thu, Apr 14, 2022 at 02:44:00PM +0100, Bryan O'Donoghue wrote:
On 14/04/2022 14:00, Sakari Ailus wrote:
ret = clk_prepare_enable(imx412->inclk);
if (ret) {
+ regulator_bulk_disable(imx412->num_supplies,
+ imx412->supplies);
As the function already has an error handling section using labels, this
should go there as well.
Are you asking to move regulator_bulk_disable() to error_reset ?
No. You'll need another label.
Hmm.
I think another label is not required, have a look at V4.
Ah, yes, indeed. There's just a single location where this will be needed.
On another note, gpiod_set_value_cansleep() seems to enable reset in
resume and disable it in suspend. I.e. the polarity is wrong.
Agreed, the polarity looks wrong - in my DTS right now I have
ACTIVE_HIGH for the relevant GPIO.
For example if I do this
@@ -1363,7 +1363,7 @@ camera@1a {
compatible = "sony,imx412";
reg = <0x1a>;
- reset-gpios = <&tlmm 78 GPIO_ACTIVE_HIGH>;
+ reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
pinctrl-names = "default", "suspend";
pinctrl-0 = <&cam2_default>;
pinctrl-1 = <&cam2_suspend>;
diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
index a9cdf4694d58..1442b416f5aa 100644
--- a/drivers/media/i2c/imx412.c
+++ b/drivers/media/i2c/imx412.c
@@ -1036,7 +1036,7 @@ static int imx412_power_on(struct device *dev)
return 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) {
@@ -1049,7 +1049,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);
regulator_bulk_disable(imx412->num_supplies, imx412->supplies);
return ret;
@@ -1068,7 +1068,7 @@ static int imx412_power_off(struct device *dev)
clk_disable_unprepare(imx412->inclk);
- gpiod_set_value_cansleep(imx412->reset_gpio, 0);
+ gpiod_set_value_cansleep(imx412->reset_gpio, 1);
Seems like changing the logic would negatively affect the Intel people.
Might have to churn ACPI to change that logic..
Easier probably to leave as is and define as ACTIVE_HIGH in DTS
