From 8f4346915bb7e3a3ad3eea2c24b6da09dac257b2 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 29 Nov 2022 15:06:23 +0100 Subject: [PATCH 1/4] sensors: Use clk-framework instead of a "clken" GPIO Use the clk-framework to get a clk-provider reference and use clk_prepare_enable() / clk_disable_unprepare() to control the clk. This replace modelling the clock as a "clken" GPIO, which is not a valid way to model it when the clk is e.g. generated by the clk-generator of a TPS68470 PMIC. This relies on the following upstream bugfix for the INT3472 clk provider: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cf5ac2d45f6e4d11ad78e7b10ae9a4121ba5e995 "platform/x86: int3472/discrete: Ensure the clk/power enable pins are in output mode" This patch is available since upstream kernel 6.1.7, so the new code is only enabled for LINUX_VERSION_CODE >= KERNEL_VERSION(6, 1, 7) This allow susing the IPU6 sensor drivers with the upstream int3472 driver with unmodified upstream kernels >= 6.1.7 . Signed-off-by: Hans de Goede --- drivers/media/i2c/hm11b1.c | 18 ++++++++++++++++++ drivers/media/i2c/ov01a1s.c | 18 ++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/drivers/media/i2c/hm11b1.c b/drivers/media/i2c/hm11b1.c index 1cc5cd761fbf..e14810bdd612 100644 --- a/drivers/media/i2c/hm11b1.c +++ b/drivers/media/i2c/hm11b1.c @@ -468,8 +468,13 @@ struct hm11b1 { struct gpio_desc *reset_gpio; /* GPIO for powerdown */ struct gpio_desc *powerdown_gpio; +#if LINUX_VERSION_CODE < KERNEL_VERSION(6, 1, 7) /* GPIO for clock enable */ struct gpio_desc *clken_gpio; +#else + /* Clock provider */ + struct clk *clk; +#endif /* GPIO for privacy LED */ struct gpio_desc *pled_gpio; #endif @@ -508,7 +513,14 @@ static void hm11b1_set_power(struct hm11b1 *hm11b1, int on) return; gpiod_set_value_cansleep(hm11b1->reset_gpio, on); gpiod_set_value_cansleep(hm11b1->powerdown_gpio, on); +#if LINUX_VERSION_CODE < KERNEL_VERSION(6, 1, 7) gpiod_set_value_cansleep(hm11b1->clken_gpio, on); +#else + if (on) + clk_prepare_enable(hm11b1->clk); + else + clk_disable_unprepare(hm11b1->clk); +#endif gpiod_set_value_cansleep(hm11b1->pled_gpio, on); msleep(20); #elif IS_ENABLED(CONFIG_POWER_CTRL_LOGIC) @@ -1093,12 +1105,18 @@ static int hm11b1_parse_dt(struct hm11b1 *hm11b1) return ret; } +#if LINUX_VERSION_CODE < KERNEL_VERSION(6, 1, 7) hm11b1->clken_gpio = devm_gpiod_get(dev, "clken", GPIOD_OUT_HIGH); ret = PTR_ERR_OR_ZERO(hm11b1->clken_gpio); if (ret < 0) { dev_err(dev, "error while getting clken_gpio gpio: %d\n", ret); return ret; } +#else + hm11b1->clk = devm_clk_get_optional(dev, "clk"); + if (IS_ERR(hm11b1->clk)) + return dev_err_probe(dev, PTR_ERR(hm11b1->clk), "getting clk\n"); +#endif hm11b1->pled_gpio = devm_gpiod_get(dev, "pled", GPIOD_OUT_HIGH); ret = PTR_ERR_OR_ZERO(hm11b1->pled_gpio); diff --git a/drivers/media/i2c/ov01a1s.c b/drivers/media/i2c/ov01a1s.c index e4477625ce3b..628a1dd83ddf 100644 --- a/drivers/media/i2c/ov01a1s.c +++ b/drivers/media/i2c/ov01a1s.c @@ -317,8 +317,13 @@ struct ov01a1s { struct gpio_desc *reset_gpio; /* GPIO for powerdown */ struct gpio_desc *powerdown_gpio; +#if LINUX_VERSION_CODE < KERNEL_VERSION(6, 1, 7) /* GPIO for clock enable */ struct gpio_desc *clken_gpio; +#else + /* Clock provider */ + struct clk *clk; +#endif /* GPIO for privacy LED */ struct gpio_desc *pled_gpio; #endif @@ -339,7 +344,14 @@ static void ov01a1s_set_power(struct ov01a1s *ov01a1s, int on) return; gpiod_set_value_cansleep(ov01a1s->reset_gpio, on); gpiod_set_value_cansleep(ov01a1s->powerdown_gpio, on); +#if LINUX_VERSION_CODE < KERNEL_VERSION(6, 1, 7) gpiod_set_value_cansleep(ov01a1s->clken_gpio, on); +#else + if (on) + clk_prepare_enable(ov01a1s->clk); + else + clk_disable_unprepare(ov01a1s->clk); +#endif gpiod_set_value_cansleep(ov01a1s->pled_gpio, on); msleep(20); #elif IS_ENABLED(CONFIG_POWER_CTRL_LOGIC) @@ -945,12 +957,18 @@ static int ov01a1s_parse_dt(struct ov01a1s *ov01a1s) return -EPROBE_DEFER; } +#if LINUX_VERSION_CODE < KERNEL_VERSION(6, 1, 7) ov01a1s->clken_gpio = devm_gpiod_get(dev, "clken", GPIOD_OUT_HIGH); ret = PTR_ERR_OR_ZERO(ov01a1s->clken_gpio); if (ret < 0) { dev_err(dev, "error while getting clken_gpio gpio: %d\n", ret); return -EPROBE_DEFER; } +#else + ov01a1s->clk = devm_clk_get_optional(dev, "clk"); + if (IS_ERR(ov01a1s->clk)) + return dev_err_probe(dev, PTR_ERR(ov01a1s->clk), "getting clk\n"); +#endif ov01a1s->pled_gpio = devm_gpiod_get(dev, "pled", GPIOD_OUT_HIGH); ret = PTR_ERR_OR_ZERO(ov01a1s->pled_gpio); From b04fdf6433f6b64840d46f92ddf3d6d18e86ede3 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 29 Nov 2022 23:37:50 +0100 Subject: [PATCH 2/4] sensors: Make powerdown and reset signals active-low by default The powerdown and reset functions should be set to 0, as in not-powered-down, not-in-reset when the sensor is turned on. Adjust the gpiod_set() value parameters for the powerdown_gpio and reset_gpio to !on to properly reflect this. Typical sensors however have a NRESET aka /RESET pin which needs to be driven low to put the device in reset and the have a powerup/enable pin rather then a powerdown pin. So at the physicical level the pins associated with the reset and powerdown functions need to be driven low to put the chip in reset / to power the chip down. Mark the pins as active-low in the added gpio-lookup table entries for these pin to reflect this. This double negation has 0 net effect, but it uses the GPIO subsystem functionality as intended (setting reset to 0 on poweron makes lot more sense then setting it to 1 on poweron) and it aligns the use of these GPIOs with that of the mainline kernel allowing future use of the IPU6 driver with the mainline INT3472 driver without needing to patch the mainline kernel. Signed-off-by: Hans de Goede --- drivers/media/i2c/hm11b1.c | 4 ++-- drivers/media/i2c/ov01a1s.c | 4 ++-- drivers/media/i2c/ov2740.c | 2 +- ...nt3472-support-independent-clock-and-LED-gpios-5.17+.patch | 4 ++-- patch/int3472-support-independent-clock-and-LED-gpios.patch | 4 ++-- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/drivers/media/i2c/hm11b1.c b/drivers/media/i2c/hm11b1.c index e14810bdd612..652e8f177044 100644 --- a/drivers/media/i2c/hm11b1.c +++ b/drivers/media/i2c/hm11b1.c @@ -511,8 +511,8 @@ static void hm11b1_set_power(struct hm11b1 *hm11b1, int on) #if IS_ENABLED(CONFIG_INTEL_SKL_INT3472) if (!(hm11b1->reset_gpio && hm11b1->powerdown_gpio)) return; - gpiod_set_value_cansleep(hm11b1->reset_gpio, on); - gpiod_set_value_cansleep(hm11b1->powerdown_gpio, on); + gpiod_set_value_cansleep(hm11b1->reset_gpio, !on); + gpiod_set_value_cansleep(hm11b1->powerdown_gpio, !on); #if LINUX_VERSION_CODE < KERNEL_VERSION(6, 1, 7) gpiod_set_value_cansleep(hm11b1->clken_gpio, on); #else diff --git a/drivers/media/i2c/ov01a1s.c b/drivers/media/i2c/ov01a1s.c index 628a1dd83ddf..2ce81d04abf6 100644 --- a/drivers/media/i2c/ov01a1s.c +++ b/drivers/media/i2c/ov01a1s.c @@ -342,8 +342,8 @@ static void ov01a1s_set_power(struct ov01a1s *ov01a1s, int on) #if IS_ENABLED(CONFIG_INTEL_SKL_INT3472) if (!(ov01a1s->reset_gpio && ov01a1s->powerdown_gpio)) return; - gpiod_set_value_cansleep(ov01a1s->reset_gpio, on); - gpiod_set_value_cansleep(ov01a1s->powerdown_gpio, on); + gpiod_set_value_cansleep(ov01a1s->reset_gpio, !on); + gpiod_set_value_cansleep(ov01a1s->powerdown_gpio, !on); #if LINUX_VERSION_CODE < KERNEL_VERSION(6, 1, 7) gpiod_set_value_cansleep(ov01a1s->clken_gpio, on); #else diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c index 67fb17e08e36..a8bb101776bd 100644 --- a/drivers/media/i2c/ov2740.c +++ b/drivers/media/i2c/ov2740.c @@ -596,7 +596,7 @@ static void ov2740_set_power(struct ov2740 *ov2740, int on) { if (!(ov2740->reset_gpio && ov2740->pled_gpio)) return; - gpiod_set_value_cansleep(ov2740->reset_gpio, on); + gpiod_set_value_cansleep(ov2740->reset_gpio, !on); gpiod_set_value_cansleep(ov2740->pled_gpio, on); msleep(20); } diff --git a/patch/int3472-support-independent-clock-and-LED-gpios-5.17+.patch b/patch/int3472-support-independent-clock-and-LED-gpios-5.17+.patch index 57373ac85f39..66ed770b68a0 100644 --- a/patch/int3472-support-independent-clock-and-LED-gpios-5.17+.patch +++ b/patch/int3472-support-independent-clock-and-LED-gpios-5.17+.patch @@ -65,7 +65,7 @@ index ed4c9d760757..f5857ec334fa 100644 case INT3472_GPIO_TYPE_RESET: ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, "reset", - GPIO_ACTIVE_LOW); -+ polarity); ++ polarity ^ GPIO_ACTIVE_LOW); if (ret) err_msg = "Failed to map reset pin to sensor\n"; @@ -73,7 +73,7 @@ index ed4c9d760757..f5857ec334fa 100644 case INT3472_GPIO_TYPE_POWERDOWN: ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, "powerdown", - GPIO_ACTIVE_LOW); -+ polarity); ++ polarity ^ GPIO_ACTIVE_LOW); if (ret) err_msg = "Failed to map powerdown pin to sensor\n"; diff --git a/patch/int3472-support-independent-clock-and-LED-gpios.patch b/patch/int3472-support-independent-clock-and-LED-gpios.patch index a2def0d76852..df70ce4a7117 100644 --- a/patch/int3472-support-independent-clock-and-LED-gpios.patch +++ b/patch/int3472-support-independent-clock-and-LED-gpios.patch @@ -65,7 +65,7 @@ index e59d79c7e82f..5cf6dd63d43f 100644 case INT3472_GPIO_TYPE_RESET: ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, "reset", - GPIO_ACTIVE_LOW); -+ polarity); ++ polarity ^ GPIO_ACTIVE_LOW); if (ret) err_msg = "Failed to map reset pin to sensor\n"; @@ -73,7 +73,7 @@ index e59d79c7e82f..5cf6dd63d43f 100644 case INT3472_GPIO_TYPE_POWERDOWN: ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, "powerdown", - GPIO_ACTIVE_LOW); -+ polarity); ++ polarity ^ GPIO_ACTIVE_LOW); if (ret) err_msg = "Failed to map powerdown pin to sensor\n"; From 90d4b2d9cb07292c6a2580572252938a836f4a86 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Thu, 15 Dec 2022 16:00:31 +0100 Subject: [PATCH 3/4] sensors: Make "pled" GPIO optional Starting with kernel 6.3 the mainline int3472 driver models the privacy LED device as a LED class device rather then as a GPIO. As part of these changed the v4l2-core subdev code in 6.3 turns the LED on/off on s_stream() on/off calls on the sensor v4l2-subdev, so sensor drivers don't have to take care of this themselves. Change the devm_gpiod_get() calls for the "pled" GPIO into devm_gpiod_get_optional() calls so that the sensor drivers can work with both older kernel (controlling the GPIO) and with newer kernels which don't have a "pled" GPIO. Signed-off-by: Hans de Goede --- drivers/media/i2c/hm11b1.c | 2 +- drivers/media/i2c/ov01a1s.c | 2 +- drivers/media/i2c/ov2740.c | 4 +--- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/media/i2c/hm11b1.c b/drivers/media/i2c/hm11b1.c index 652e8f177044..6257f7987268 100644 --- a/drivers/media/i2c/hm11b1.c +++ b/drivers/media/i2c/hm11b1.c @@ -1118,7 +1118,7 @@ static int hm11b1_parse_dt(struct hm11b1 *hm11b1) return dev_err_probe(dev, PTR_ERR(hm11b1->clk), "getting clk\n"); #endif - hm11b1->pled_gpio = devm_gpiod_get(dev, "pled", GPIOD_OUT_HIGH); + hm11b1->pled_gpio = devm_gpiod_get_optional(dev, "pled", GPIOD_OUT_HIGH); ret = PTR_ERR_OR_ZERO(hm11b1->pled_gpio); if (ret < 0) { dev_err(dev, "error while getting pled gpio: %d\n", ret); diff --git a/drivers/media/i2c/ov01a1s.c b/drivers/media/i2c/ov01a1s.c index 2ce81d04abf6..1bc6199713f3 100644 --- a/drivers/media/i2c/ov01a1s.c +++ b/drivers/media/i2c/ov01a1s.c @@ -970,7 +970,7 @@ static int ov01a1s_parse_dt(struct ov01a1s *ov01a1s) return dev_err_probe(dev, PTR_ERR(ov01a1s->clk), "getting clk\n"); #endif - ov01a1s->pled_gpio = devm_gpiod_get(dev, "pled", GPIOD_OUT_HIGH); + ov01a1s->pled_gpio = devm_gpiod_get_optional(dev, "pled", GPIOD_OUT_HIGH); ret = PTR_ERR_OR_ZERO(ov01a1s->pled_gpio); if (ret < 0) { dev_err(dev, "error while getting pled gpio: %d\n", ret); diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c index a8bb101776bd..08f284d4aca1 100644 --- a/drivers/media/i2c/ov2740.c +++ b/drivers/media/i2c/ov2740.c @@ -594,8 +594,6 @@ static u64 to_pixels_per_line(u32 hts, u32 f_index) static void ov2740_set_power(struct ov2740 *ov2740, int on) { - if (!(ov2740->reset_gpio && ov2740->pled_gpio)) - return; gpiod_set_value_cansleep(ov2740->reset_gpio, !on); gpiod_set_value_cansleep(ov2740->pled_gpio, on); msleep(20); @@ -633,7 +631,7 @@ static int ov2740_parse_dt(struct ov2740 *ov2740) return ret; } - ov2740->pled_gpio = devm_gpiod_get(dev, "pled", GPIOD_OUT_HIGH); + ov2740->pled_gpio = devm_gpiod_get_optional(dev, "pled", GPIOD_OUT_HIGH); ret = PTR_ERR_OR_ZERO(ov2740->pled_gpio); if (ret < 0) { dev_err(dev, "error while getting pled gpio: %d\n", ret); From 5ed1980822f0cb4787d1346493d126aad1bf9210 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 29 Nov 2022 15:15:15 +0100 Subject: [PATCH 4/4] ov01a1s: Drop unused link_freq variable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the unused link_freq variable, fixing this compiler warning: drivers/media/i2c/ov01a1s.c:994:13: warning: unused variable ‘link_freq’ [-Wunused-variable] 994 | s64 link_freq; | ^~~~~~~~~ Signed-off-by: Hans de Goede --- drivers/media/i2c/ov01a1s.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/media/i2c/ov01a1s.c b/drivers/media/i2c/ov01a1s.c index 1bc6199713f3..ab4ff255d4c1 100644 --- a/drivers/media/i2c/ov01a1s.c +++ b/drivers/media/i2c/ov01a1s.c @@ -988,7 +988,6 @@ static int ov01a1s_probe(struct i2c_client *client) #if IS_ENABLED(CONFIG_INTEL_VSC) struct vsc_mipi_config conf; struct vsc_camera_status status; - s64 link_freq; #endif ov01a1s = devm_kzalloc(&client->dev, sizeof(*ov01a1s), GFP_KERNEL);