[PATCH 1/3] platform/x86: int3472: Avoid crash in unregistering regulator gpio

[PATCH 1/3] platform/x86: int3472: Avoid crash in unregistering regulator gpio

[bingbu.cao]

From: Hao Yao <hao.yao@intel.com>

When int3472 is loaded before GPIO driver, acpi_get_and_request_gpiod()
failed but the returned gpio descriptor is not NULL, it will cause panic
in later gpiod_put(), so set the gpio_desc to NULL in register error
handling to avoid such crash.

Signed-off-by: Hao Yao <hao.yao@intel.com>
Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
---
 .../x86/intel/int3472/clk_and_regulator.c        | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index 1086c3d83494..d1088be5af78 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -101,9 +101,12 @@ int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
 
 	int3472->clock.ena_gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0],
 							     "int3472,clk-enable");
-	if (IS_ERR(int3472->clock.ena_gpio))
-		return dev_err_probe(int3472->dev, PTR_ERR(int3472->clock.ena_gpio),
-				     "getting clk-enable GPIO\n");
+	if (IS_ERR(int3472->clock.ena_gpio)) {
+		ret = PTR_ERR(int3472->clock.ena_gpio);
+		int3472->clock.ena_gpio = NULL;
+		return dev_err_probe(int3472->dev, ret,
+				     "failed to get gpio for clock\n");
+	}
 
 	if (polarity == GPIO_ACTIVE_LOW)
 		gpiod_toggle_active_low(int3472->clock.ena_gpio);
@@ -199,8 +202,11 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
 	int3472->regulator.gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0],
 							     "int3472,regulator");
 	if (IS_ERR(int3472->regulator.gpio)) {
-		dev_err(int3472->dev, "Failed to get regulator GPIO line\n");
-		return PTR_ERR(int3472->regulator.gpio);
+		ret = PTR_ERR(int3472->regulator.gpio);
+		int3472->regulator.gpio = NULL;
+
+		return dev_err_probe(int3472->dev, ret,
+				     "failed to get regulator gpio\n");
 	}
 
 	/* Ensure the pin is in output mode and non-active state */
-- 
2.40.1

[PATCH 2/3] platform/x86: int3472: Evaluate device's _DSM method to control imaging clock

[bingbu.cao]

From: Bingbu Cao <bingbu.cao@intel.com>

On some platforms, the imaging clock should be controlled by evaluating
specific clock device's _DSM method instead of setting gpio, so this
change register clock if no gpio based clock and then use the _DSM method
to enable and disable clock.

Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
Signed-off-by: Hao Yao <hao.yao@intel.com>
---
 .../x86/intel/int3472/clk_and_regulator.c     | 81 ++++++++++++++++++-
 drivers/platform/x86/intel/int3472/common.h   |  6 +-
 drivers/platform/x86/intel/int3472/discrete.c |  4 +
 3 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
index d1088be5af78..f0a1d2ef137b 100644
--- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
+++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
@@ -11,6 +11,32 @@
 
 #include "common.h"
 
+static const guid_t img_clk_guid =
+	GUID_INIT(0x82c0d13a, 0x78c5, 0x4244,
+		  0x9b, 0xb1, 0xeb, 0x8b, 0x53, 0x9a, 0x8d, 0x11);
+
+static void skl_int3472_enable_clk_acpi_method(struct int3472_gpio_clock *clk,
+					       bool enable)
+{
+	struct int3472_discrete_device *int3472 = to_int3472_device(clk);
+	union acpi_object args[3];
+	union acpi_object argv4;
+
+	args[0].integer.type = ACPI_TYPE_INTEGER;
+	args[0].integer.value = clk->imgclk_index;
+	args[1].integer.type = ACPI_TYPE_INTEGER;
+	args[1].integer.value = enable ? 1 : 0;
+	args[2].integer.type = ACPI_TYPE_INTEGER;
+	args[2].integer.value = 1;
+
+	argv4.type = ACPI_TYPE_PACKAGE;
+	argv4.package.count = 3;
+	argv4.package.elements = args;
+
+	acpi_evaluate_dsm(acpi_device_handle(int3472->adev), &img_clk_guid,
+			  0, 1, &argv4);
+}
+
 /*
  * The regulators have to have .ops to be valid, but the only ops we actually
  * support are .enable and .disable which are handled via .ena_gpiod. Pass an
@@ -22,7 +48,11 @@ static int skl_int3472_clk_prepare(struct clk_hw *hw)
 {
 	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
 
-	gpiod_set_value_cansleep(clk->ena_gpio, 1);
+	if (clk->ena_gpio)
+		gpiod_set_value_cansleep(clk->ena_gpio, 1);
+	else
+		skl_int3472_enable_clk_acpi_method(clk, 1);
+
 	return 0;
 }
 
@@ -30,7 +60,10 @@ static void skl_int3472_clk_unprepare(struct clk_hw *hw)
 {
 	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
 
-	gpiod_set_value_cansleep(clk->ena_gpio, 0);
+	if (clk->ena_gpio)
+		gpiod_set_value_cansleep(clk->ena_gpio, 0);
+	else
+		skl_int3472_enable_clk_acpi_method(clk, 0);
 }
 
 static int skl_int3472_clk_enable(struct clk_hw *hw)
@@ -86,6 +119,50 @@ static const struct clk_ops skl_int3472_clock_ops = {
 	.recalc_rate = skl_int3472_clk_recalc_rate,
 };
 
+int skl_int3472_register_clock_src(struct int3472_discrete_device *int3472)
+{
+	struct clk_init_data init = {
+		.ops = &skl_int3472_clock_ops,
+		.flags = CLK_GET_RATE_NOCACHE,
+	};
+	int ret;
+
+	if (int3472->clock.cl)
+		return -EBUSY;
+
+	init.name = kasprintf(GFP_KERNEL, "%s-clk",
+			      acpi_dev_name(int3472->adev));
+	if (!init.name)
+		return -ENOMEM;
+
+	int3472->clock.frequency = skl_int3472_get_clk_frequency(int3472);
+	int3472->clock.clk_hw.init = &init;
+	int3472->clock.clk = clk_register(&int3472->adev->dev,
+					  &int3472->clock.clk_hw);
+	if (IS_ERR(int3472->clock.clk)) {
+		ret = PTR_ERR(int3472->clock.clk);
+		goto out_free_init_name;
+	}
+
+	int3472->clock.cl = clkdev_create(int3472->clock.clk, NULL,
+					  int3472->sensor_name);
+	if (!int3472->clock.cl) {
+		ret = -ENOMEM;
+		goto err_unregister_clk;
+	}
+
+	kfree(init.name);
+
+	return 0;
+
+err_unregister_clk:
+	clk_unregister(int3472->clock.clk);
+out_free_init_name:
+	kfree(init.name);
+
+	return ret;
+}
+
 int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
 			       struct acpi_resource_gpio *agpio, u32 polarity)
 {
diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
index 61688e450ce5..036b804e8ea5 100644
--- a/drivers/platform/x86/intel/int3472/common.h
+++ b/drivers/platform/x86/intel/int3472/common.h
@@ -64,7 +64,9 @@ struct int3472_cldb {
 	u8 control_logic_type;
 	u8 control_logic_id;
 	u8 sensor_card_sku;
-	u8 reserved[28];
+	u8 reserved[10];
+	u8 clock_source;
+	u8 reserved2[17];
 };
 
 struct int3472_gpio_function_remap {
@@ -100,6 +102,7 @@ struct int3472_discrete_device {
 		struct clk_lookup *cl;
 		struct gpio_desc *ena_gpio;
 		u32 frequency;
+		u8 imgclk_index;
 	} clock;
 
 	struct int3472_pled {
@@ -123,6 +126,7 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev,
 
 int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
 			       struct acpi_resource_gpio *agpio, u32 polarity);
+int skl_int3472_register_clock_src(struct int3472_discrete_device *int3472);
 void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472);
 
 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index ef020e23e596..d5d5c650d6d2 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -309,6 +309,9 @@ static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
 	if (ret < 0)
 		return ret;
 
+	/* If no gpio based clk registered, try register with clock source */
+	skl_int3472_register_clock_src(int3472);
+
 	acpi_dev_free_resource_list(&resource_list);
 
 	int3472->gpios.dev_id = int3472->sensor_name;
@@ -356,6 +359,7 @@ static int skl_int3472_discrete_probe(struct platform_device *pdev)
 	int3472->adev = adev;
 	int3472->dev = &pdev->dev;
 	platform_set_drvdata(pdev, int3472);
+	int3472->clock.imgclk_index = cldb.clock_source;
 
 	ret = skl_int3472_get_sensor_adev_and_name(&pdev->dev, &int3472->sensor,
 						   &int3472->sensor_name);
-- 
2.40.1

Re: [PATCH 2/3] platform/x86: int3472: Evaluate device's _DSM method to control imaging clock

[Andy Shevchenko]

On Wed, May 24, 2023 at 11:51:34AM +0800, bingbu.cao@intel.com wrote:
> From: Bingbu Cao <bingbu.cao@intel.com>
> 
> On some platforms, the imaging clock should be controlled by evaluating
> specific clock device's _DSM method instead of setting gpio, so this
> change register clock if no gpio based clock and then use the _DSM method
> to enable and disable clock.

...

Add a comment here where you put the GUID in a human-readable format for easy
googling / searching for in the internet / documentation.

> +static const guid_t img_clk_guid =
> +	GUID_INIT(0x82c0d13a, 0x78c5, 0x4244,
> +		  0x9b, 0xb1, 0xeb, 0x8b, 0x53, 0x9a, 0x8d, 0x11);

...

With

	struct acpi_device *adev = ...;

> +	init.name = kasprintf(GFP_KERNEL, "%s-clk",
> +			      acpi_dev_name(int3472->adev));


This become a single line.

> +	if (!init.name)
> +		return -ENOMEM;
> +
> +	int3472->clock.frequency = skl_int3472_get_clk_frequency(int3472);
> +	int3472->clock.clk_hw.init = &init;
> +	int3472->clock.clk = clk_register(&int3472->adev->dev,

And the above can be reused later in this function, like

	int3472->clock.clk = clk_register(&adev->dev, &int3472->clock.clk_hw);

> +					  &int3472->clock.clk_hw);
> +	if (IS_ERR(int3472->clock.clk)) {
> +		ret = PTR_ERR(int3472->clock.clk);
> +		goto out_free_init_name;
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] platform/x86: int3472: Evaluate device's _DSM method to control imaging clock

[Hans de Goede]

Hi,

On 5/24/23 05:51, bingbu.cao@intel.com wrote:
> From: Bingbu Cao <bingbu.cao@intel.com>
> 
> On some platforms, the imaging clock should be controlled by evaluating
> specific clock device's _DSM method instead of setting gpio, so this
> change register clock if no gpio based clock and then use the _DSM method
> to enable and disable clock.
> 
> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> Signed-off-by: Hao Yao <hao.yao@intel.com>

Thank you for this interesting patch.

Besides Andy's comments I believe that this also needs an acpi_check_dsm() call
to see if the DSM functionality is available and the call of the new clk
register function should be error checked.

Since I was curious about this and wanted to test it myself (on a Thinkpad
X1 Yoga Gen 7) I have prepared a v2 addressing all of the above, quoting
from the changelog:

Changes in v2 (Hans de Goede):
- Minor comment / code changes (address Andy's review remarks)
- Add a acpi_check_dsm() call
- Return 0 instead of error if we already have a GPIO clk or if
  acpi_check_dsm() fails
- Rename skl_int3472_register_clock() -> skl_int3472_register_gpio_clock()
  and name the new function: skl_int3472_register_dsm_clock()
- Move the skl_int3472_register_dsm_clock() call to after
  acpi_dev_free_resource_list() and add error checking for it

I'll send out the v2 right after this email. Please give v2 a try
and let me know if it works for you.

Regards,

Hans




> ---
>  .../x86/intel/int3472/clk_and_regulator.c     | 81 ++++++++++++++++++-
>  drivers/platform/x86/intel/int3472/common.h   |  6 +-
>  drivers/platform/x86/intel/int3472/discrete.c |  4 +
>  3 files changed, 88 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> index d1088be5af78..f0a1d2ef137b 100644
> --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> @@ -11,6 +11,32 @@
>  
>  #include "common.h"
>  
> +static const guid_t img_clk_guid =
> +	GUID_INIT(0x82c0d13a, 0x78c5, 0x4244,
> +		  0x9b, 0xb1, 0xeb, 0x8b, 0x53, 0x9a, 0x8d, 0x11);
> +
> +static void skl_int3472_enable_clk_acpi_method(struct int3472_gpio_clock *clk,
> +					       bool enable)
> +{
> +	struct int3472_discrete_device *int3472 = to_int3472_device(clk);
> +	union acpi_object args[3];
> +	union acpi_object argv4;
> +
> +	args[0].integer.type = ACPI_TYPE_INTEGER;
> +	args[0].integer.value = clk->imgclk_index;
> +	args[1].integer.type = ACPI_TYPE_INTEGER;
> +	args[1].integer.value = enable ? 1 : 0;
> +	args[2].integer.type = ACPI_TYPE_INTEGER;
> +	args[2].integer.value = 1;
> +
> +	argv4.type = ACPI_TYPE_PACKAGE;
> +	argv4.package.count = 3;
> +	argv4.package.elements = args;
> +
> +	acpi_evaluate_dsm(acpi_device_handle(int3472->adev), &img_clk_guid,
> +			  0, 1, &argv4);
> +}
> +
>  /*
>   * The regulators have to have .ops to be valid, but the only ops we actually
>   * support are .enable and .disable which are handled via .ena_gpiod. Pass an
> @@ -22,7 +48,11 @@ static int skl_int3472_clk_prepare(struct clk_hw *hw)
>  {
>  	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>  
> -	gpiod_set_value_cansleep(clk->ena_gpio, 1);
> +	if (clk->ena_gpio)
> +		gpiod_set_value_cansleep(clk->ena_gpio, 1);
> +	else
> +		skl_int3472_enable_clk_acpi_method(clk, 1);
> +
>  	return 0;
>  }
>  
> @@ -30,7 +60,10 @@ static void skl_int3472_clk_unprepare(struct clk_hw *hw)
>  {
>  	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>  
> -	gpiod_set_value_cansleep(clk->ena_gpio, 0);
> +	if (clk->ena_gpio)
> +		gpiod_set_value_cansleep(clk->ena_gpio, 0);
> +	else
> +		skl_int3472_enable_clk_acpi_method(clk, 0);
>  }
>  
>  static int skl_int3472_clk_enable(struct clk_hw *hw)
> @@ -86,6 +119,50 @@ static const struct clk_ops skl_int3472_clock_ops = {
>  	.recalc_rate = skl_int3472_clk_recalc_rate,
>  };
>  
> +int skl_int3472_register_clock_src(struct int3472_discrete_device *int3472)
> +{
> +	struct clk_init_data init = {
> +		.ops = &skl_int3472_clock_ops,
> +		.flags = CLK_GET_RATE_NOCACHE,
> +	};
> +	int ret;
> +
> +	if (int3472->clock.cl)
> +		return -EBUSY;
> +
> +	init.name = kasprintf(GFP_KERNEL, "%s-clk",
> +			      acpi_dev_name(int3472->adev));
> +	if (!init.name)
> +		return -ENOMEM;
> +
> +	int3472->clock.frequency = skl_int3472_get_clk_frequency(int3472);
> +	int3472->clock.clk_hw.init = &init;
> +	int3472->clock.clk = clk_register(&int3472->adev->dev,
> +					  &int3472->clock.clk_hw);
> +	if (IS_ERR(int3472->clock.clk)) {
> +		ret = PTR_ERR(int3472->clock.clk);
> +		goto out_free_init_name;
> +	}
> +
> +	int3472->clock.cl = clkdev_create(int3472->clock.clk, NULL,
> +					  int3472->sensor_name);
> +	if (!int3472->clock.cl) {
> +		ret = -ENOMEM;
> +		goto err_unregister_clk;
> +	}
> +
> +	kfree(init.name);
> +
> +	return 0;
> +
> +err_unregister_clk:
> +	clk_unregister(int3472->clock.clk);
> +out_free_init_name:
> +	kfree(init.name);
> +
> +	return ret;
> +}
> +
>  int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
>  			       struct acpi_resource_gpio *agpio, u32 polarity)
>  {
> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
> index 61688e450ce5..036b804e8ea5 100644
> --- a/drivers/platform/x86/intel/int3472/common.h
> +++ b/drivers/platform/x86/intel/int3472/common.h
> @@ -64,7 +64,9 @@ struct int3472_cldb {
>  	u8 control_logic_type;
>  	u8 control_logic_id;
>  	u8 sensor_card_sku;
> -	u8 reserved[28];
> +	u8 reserved[10];
> +	u8 clock_source;
> +	u8 reserved2[17];
>  };
>  
>  struct int3472_gpio_function_remap {
> @@ -100,6 +102,7 @@ struct int3472_discrete_device {
>  		struct clk_lookup *cl;
>  		struct gpio_desc *ena_gpio;
>  		u32 frequency;
> +		u8 imgclk_index;
>  	} clock;
>  
>  	struct int3472_pled {
> @@ -123,6 +126,7 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev,
>  
>  int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
>  			       struct acpi_resource_gpio *agpio, u32 polarity);
> +int skl_int3472_register_clock_src(struct int3472_discrete_device *int3472);
>  void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472);
>  
>  int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index ef020e23e596..d5d5c650d6d2 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -309,6 +309,9 @@ static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
>  	if (ret < 0)
>  		return ret;
>  
> +	/* If no gpio based clk registered, try register with clock source */
> +	skl_int3472_register_clock_src(int3472);
> +
>  	acpi_dev_free_resource_list(&resource_list);
>  
>  	int3472->gpios.dev_id = int3472->sensor_name;
> @@ -356,6 +359,7 @@ static int skl_int3472_discrete_probe(struct platform_device *pdev)
>  	int3472->adev = adev;
>  	int3472->dev = &pdev->dev;
>  	platform_set_drvdata(pdev, int3472);
> +	int3472->clock.imgclk_index = cldb.clock_source;
>  
>  	ret = skl_int3472_get_sensor_adev_and_name(&pdev->dev, &int3472->sensor,
>  						   &int3472->sensor_name);

RE: [PATCH 2/3] platform/x86: int3472: Evaluate device's _DSM method to control imaging clock

[Cao, Bingbu]

Hans,

Thanks for your review and v2 patch. 

Hao, could you help verify the v2 from Hans on your system?

------------------------------------------------------------------------
BRs,  
Bingbu Cao 

>-----Original Message-----
>From: Hans de Goede <hdegoede@redhat.com>
>Sent: Wednesday, May 31, 2023 21:44
>To: Cao, Bingbu <bingbu.cao@intel.com>; djrscally@gmail.com;
>dan.scally@ideasonboard.com; Yao, Hao <hao.yao@intel.com>
>Cc: markgross@kernel.org; linux-media@vger.kernel.org;
>sakari.ailus@linux.intel.com; andriy.shevchenko@linux.intel.com;
>bingbu.cao@linux.intel.com
>Subject: Re: [PATCH 2/3] platform/x86: int3472: Evaluate device's _DSM
>method to control imaging clock
>
>Hi,
>
>On 5/24/23 05:51, bingbu.cao@intel.com wrote:
>> From: Bingbu Cao <bingbu.cao@intel.com>
>>
>> On some platforms, the imaging clock should be controlled by
>> evaluating specific clock device's _DSM method instead of setting
>> gpio, so this change register clock if no gpio based clock and then
>> use the _DSM method to enable and disable clock.
>>
>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
>> Signed-off-by: Hao Yao <hao.yao@intel.com>
>
>Thank you for this interesting patch.
>
>Besides Andy's comments I believe that this also needs an acpi_check_dsm()
>call to see if the DSM functionality is available and the call of the new
>clk register function should be error checked.
>
>Since I was curious about this and wanted to test it myself (on a Thinkpad
>X1 Yoga Gen 7) I have prepared a v2 addressing all of the above, quoting
>from the changelog:
>
>Changes in v2 (Hans de Goede):
>- Minor comment / code changes (address Andy's review remarks)
>- Add a acpi_check_dsm() call
>- Return 0 instead of error if we already have a GPIO clk or if
>  acpi_check_dsm() fails
>- Rename skl_int3472_register_clock() -> skl_int3472_register_gpio_clock()
>  and name the new function: skl_int3472_register_dsm_clock()
>- Move the skl_int3472_register_dsm_clock() call to after
>  acpi_dev_free_resource_list() and add error checking for it
>
>I'll send out the v2 right after this email. Please give v2 a try and let
>me know if it works for you.
>
>Regards,
>
>Hans
>
>
>
>
>> ---
>>  .../x86/intel/int3472/clk_and_regulator.c     | 81 ++++++++++++++++++-
>>  drivers/platform/x86/intel/int3472/common.h   |  6 +-
>>  drivers/platform/x86/intel/int3472/discrete.c |  4 +
>>  3 files changed, 88 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> index d1088be5af78..f0a1d2ef137b 100644
>> --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> @@ -11,6 +11,32 @@
>>
>>  #include "common.h"
>>
>> +static const guid_t img_clk_guid =
>> +	GUID_INIT(0x82c0d13a, 0x78c5, 0x4244,
>> +		  0x9b, 0xb1, 0xeb, 0x8b, 0x53, 0x9a, 0x8d, 0x11);
>> +
>> +static void skl_int3472_enable_clk_acpi_method(struct int3472_gpio_clock
>*clk,
>> +					       bool enable)
>> +{
>> +	struct int3472_discrete_device *int3472 = to_int3472_device(clk);
>> +	union acpi_object args[3];
>> +	union acpi_object argv4;
>> +
>> +	args[0].integer.type = ACPI_TYPE_INTEGER;
>> +	args[0].integer.value = clk->imgclk_index;
>> +	args[1].integer.type = ACPI_TYPE_INTEGER;
>> +	args[1].integer.value = enable ? 1 : 0;
>> +	args[2].integer.type = ACPI_TYPE_INTEGER;
>> +	args[2].integer.value = 1;
>> +
>> +	argv4.type = ACPI_TYPE_PACKAGE;
>> +	argv4.package.count = 3;
>> +	argv4.package.elements = args;
>> +
>> +	acpi_evaluate_dsm(acpi_device_handle(int3472->adev), &img_clk_guid,
>> +			  0, 1, &argv4);
>> +}
>> +
>>  /*
>>   * The regulators have to have .ops to be valid, but the only ops we
>actually
>>   * support are .enable and .disable which are handled via .ena_gpiod.
>> Pass an @@ -22,7 +48,11 @@ static int skl_int3472_clk_prepare(struct
>> clk_hw *hw)  {
>>  	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>>
>> -	gpiod_set_value_cansleep(clk->ena_gpio, 1);
>> +	if (clk->ena_gpio)
>> +		gpiod_set_value_cansleep(clk->ena_gpio, 1);
>> +	else
>> +		skl_int3472_enable_clk_acpi_method(clk, 1);
>> +
>>  	return 0;
>>  }
>>
>> @@ -30,7 +60,10 @@ static void skl_int3472_clk_unprepare(struct clk_hw
>> *hw)  {
>>  	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>>
>> -	gpiod_set_value_cansleep(clk->ena_gpio, 0);
>> +	if (clk->ena_gpio)
>> +		gpiod_set_value_cansleep(clk->ena_gpio, 0);
>> +	else
>> +		skl_int3472_enable_clk_acpi_method(clk, 0);
>>  }
>>
>>  static int skl_int3472_clk_enable(struct clk_hw *hw) @@ -86,6 +119,50
>> @@ static const struct clk_ops skl_int3472_clock_ops = {
>>  	.recalc_rate = skl_int3472_clk_recalc_rate,  };
>>
>> +int skl_int3472_register_clock_src(struct int3472_discrete_device
>> +*int3472) {
>> +	struct clk_init_data init = {
>> +		.ops = &skl_int3472_clock_ops,
>> +		.flags = CLK_GET_RATE_NOCACHE,
>> +	};
>> +	int ret;
>> +
>> +	if (int3472->clock.cl)
>> +		return -EBUSY;
>> +
>> +	init.name = kasprintf(GFP_KERNEL, "%s-clk",
>> +			      acpi_dev_name(int3472->adev));
>> +	if (!init.name)
>> +		return -ENOMEM;
>> +
>> +	int3472->clock.frequency = skl_int3472_get_clk_frequency(int3472);
>> +	int3472->clock.clk_hw.init = &init;
>> +	int3472->clock.clk = clk_register(&int3472->adev->dev,
>> +					  &int3472->clock.clk_hw);
>> +	if (IS_ERR(int3472->clock.clk)) {
>> +		ret = PTR_ERR(int3472->clock.clk);
>> +		goto out_free_init_name;
>> +	}
>> +
>> +	int3472->clock.cl = clkdev_create(int3472->clock.clk, NULL,
>> +					  int3472->sensor_name);
>> +	if (!int3472->clock.cl) {
>> +		ret = -ENOMEM;
>> +		goto err_unregister_clk;
>> +	}
>> +
>> +	kfree(init.name);
>> +
>> +	return 0;
>> +
>> +err_unregister_clk:
>> +	clk_unregister(int3472->clock.clk);
>> +out_free_init_name:
>> +	kfree(init.name);
>> +
>> +	return ret;
>> +}
>> +
>>  int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
>>  			       struct acpi_resource_gpio *agpio, u32 polarity)
>{ diff
>> --git a/drivers/platform/x86/intel/int3472/common.h
>> b/drivers/platform/x86/intel/int3472/common.h
>> index 61688e450ce5..036b804e8ea5 100644
>> --- a/drivers/platform/x86/intel/int3472/common.h
>> +++ b/drivers/platform/x86/intel/int3472/common.h
>> @@ -64,7 +64,9 @@ struct int3472_cldb {
>>  	u8 control_logic_type;
>>  	u8 control_logic_id;
>>  	u8 sensor_card_sku;
>> -	u8 reserved[28];
>> +	u8 reserved[10];
>> +	u8 clock_source;
>> +	u8 reserved2[17];
>>  };
>>
>>  struct int3472_gpio_function_remap {
>> @@ -100,6 +102,7 @@ struct int3472_discrete_device {
>>  		struct clk_lookup *cl;
>>  		struct gpio_desc *ena_gpio;
>>  		u32 frequency;
>> +		u8 imgclk_index;
>>  	} clock;
>>
>>  	struct int3472_pled {
>> @@ -123,6 +126,7 @@ int skl_int3472_get_sensor_adev_and_name(struct
>> device *dev,
>>
>>  int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
>>  			       struct acpi_resource_gpio *agpio, u32 polarity);
>> +int skl_int3472_register_clock_src(struct int3472_discrete_device
>> +*int3472);
>>  void skl_int3472_unregister_clock(struct int3472_discrete_device
>> *int3472);
>>
>>  int skl_int3472_register_regulator(struct int3472_discrete_device
>> *int3472, diff --git a/drivers/platform/x86/intel/int3472/discrete.c
>> b/drivers/platform/x86/intel/int3472/discrete.c
>> index ef020e23e596..d5d5c650d6d2 100644
>> --- a/drivers/platform/x86/intel/int3472/discrete.c
>> +++ b/drivers/platform/x86/intel/int3472/discrete.c
>> @@ -309,6 +309,9 @@ static int skl_int3472_parse_crs(struct
>int3472_discrete_device *int3472)
>>  	if (ret < 0)
>>  		return ret;
>>
>> +	/* If no gpio based clk registered, try register with clock source
>*/
>> +	skl_int3472_register_clock_src(int3472);
>> +
>>  	acpi_dev_free_resource_list(&resource_list);
>>
>>  	int3472->gpios.dev_id = int3472->sensor_name; @@ -356,6 +359,7 @@
>> static int skl_int3472_discrete_probe(struct platform_device *pdev)
>>  	int3472->adev = adev;
>>  	int3472->dev = &pdev->dev;
>>  	platform_set_drvdata(pdev, int3472);
>> +	int3472->clock.imgclk_index = cldb.clock_source;
>>
>>  	ret = skl_int3472_get_sensor_adev_and_name(&pdev->dev, &int3472-
>>sensor,
>>  						   &int3472->sensor_name);

RE: [PATCH 2/3] platform/x86: int3472: Evaluate device's _DSM method to control imaging clock

[Yao, Hao]

Hi Hans,

Thanks for your v2 patch, it is verified working on my system.

Best regards,
Hao Yao

-----Original Message-----
From: Cao, Bingbu <bingbu.cao@intel.com> 
Sent: Thursday, June 1, 2023 2:24 PM
To: Hans de Goede <hdegoede@redhat.com>; djrscally@gmail.com; dan.scally@ideasonboard.com; Yao, Hao <hao.yao@intel.com>
Cc: markgross@kernel.org; linux-media@vger.kernel.org; sakari.ailus@linux.intel.com; andriy.shevchenko@linux.intel.com; bingbu.cao@linux.intel.com
Subject: RE: [PATCH 2/3] platform/x86: int3472: Evaluate device's _DSM method to control imaging clock

Hans,

Thanks for your review and v2 patch. 

Hao, could you help verify the v2 from Hans on your system?

------------------------------------------------------------------------
BRs,
Bingbu Cao 

>-----Original Message-----
>From: Hans de Goede <hdegoede@redhat.com>
>Sent: Wednesday, May 31, 2023 21:44
>To: Cao, Bingbu <bingbu.cao@intel.com>; djrscally@gmail.com; 
>dan.scally@ideasonboard.com; Yao, Hao <hao.yao@intel.com>
>Cc: markgross@kernel.org; linux-media@vger.kernel.org; 
>sakari.ailus@linux.intel.com; andriy.shevchenko@linux.intel.com;
>bingbu.cao@linux.intel.com
>Subject: Re: [PATCH 2/3] platform/x86: int3472: Evaluate device's _DSM 
>method to control imaging clock
>
>Hi,
>
>On 5/24/23 05:51, bingbu.cao@intel.com wrote:
>> From: Bingbu Cao <bingbu.cao@intel.com>
>>
>> On some platforms, the imaging clock should be controlled by 
>> evaluating specific clock device's _DSM method instead of setting 
>> gpio, so this change register clock if no gpio based clock and then 
>> use the _DSM method to enable and disable clock.
>>
>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
>> Signed-off-by: Hao Yao <hao.yao@intel.com>
>
>Thank you for this interesting patch.
>
>Besides Andy's comments I believe that this also needs an 
>acpi_check_dsm() call to see if the DSM functionality is available and 
>the call of the new clk register function should be error checked.
>
>Since I was curious about this and wanted to test it myself (on a 
>Thinkpad
>X1 Yoga Gen 7) I have prepared a v2 addressing all of the above, 
>quoting from the changelog:
>
>Changes in v2 (Hans de Goede):
>- Minor comment / code changes (address Andy's review remarks)
>- Add a acpi_check_dsm() call
>- Return 0 instead of error if we already have a GPIO clk or if
>  acpi_check_dsm() fails
>- Rename skl_int3472_register_clock() -> 
>skl_int3472_register_gpio_clock()
>  and name the new function: skl_int3472_register_dsm_clock()
>- Move the skl_int3472_register_dsm_clock() call to after
>  acpi_dev_free_resource_list() and add error checking for it
>
>I'll send out the v2 right after this email. Please give v2 a try and 
>let me know if it works for you.
>
>Regards,
>
>Hans
>
>
>
>
>> ---
>>  .../x86/intel/int3472/clk_and_regulator.c     | 81 ++++++++++++++++++-
>>  drivers/platform/x86/intel/int3472/common.h   |  6 +-
>>  drivers/platform/x86/intel/int3472/discrete.c |  4 +
>>  3 files changed, 88 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> index d1088be5af78..f0a1d2ef137b 100644
>> --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> @@ -11,6 +11,32 @@
>>
>>  #include "common.h"
>>
>> +static const guid_t img_clk_guid =
>> +	GUID_INIT(0x82c0d13a, 0x78c5, 0x4244,
>> +		  0x9b, 0xb1, 0xeb, 0x8b, 0x53, 0x9a, 0x8d, 0x11);
>> +
>> +static void skl_int3472_enable_clk_acpi_method(struct 
>> +int3472_gpio_clock
>*clk,
>> +					       bool enable)
>> +{
>> +	struct int3472_discrete_device *int3472 = to_int3472_device(clk);
>> +	union acpi_object args[3];
>> +	union acpi_object argv4;
>> +
>> +	args[0].integer.type = ACPI_TYPE_INTEGER;
>> +	args[0].integer.value = clk->imgclk_index;
>> +	args[1].integer.type = ACPI_TYPE_INTEGER;
>> +	args[1].integer.value = enable ? 1 : 0;
>> +	args[2].integer.type = ACPI_TYPE_INTEGER;
>> +	args[2].integer.value = 1;
>> +
>> +	argv4.type = ACPI_TYPE_PACKAGE;
>> +	argv4.package.count = 3;
>> +	argv4.package.elements = args;
>> +
>> +	acpi_evaluate_dsm(acpi_device_handle(int3472->adev), &img_clk_guid,
>> +			  0, 1, &argv4);
>> +}
>> +
>>  /*
>>   * The regulators have to have .ops to be valid, but the only ops we
>actually
>>   * support are .enable and .disable which are handled via .ena_gpiod.
>> Pass an @@ -22,7 +48,11 @@ static int skl_int3472_clk_prepare(struct 
>> clk_hw *hw)  {
>>  	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>>
>> -	gpiod_set_value_cansleep(clk->ena_gpio, 1);
>> +	if (clk->ena_gpio)
>> +		gpiod_set_value_cansleep(clk->ena_gpio, 1);
>> +	else
>> +		skl_int3472_enable_clk_acpi_method(clk, 1);
>> +
>>  	return 0;
>>  }
>>
>> @@ -30,7 +60,10 @@ static void skl_int3472_clk_unprepare(struct 
>> clk_hw
>> *hw)  {
>>  	struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>>
>> -	gpiod_set_value_cansleep(clk->ena_gpio, 0);
>> +	if (clk->ena_gpio)
>> +		gpiod_set_value_cansleep(clk->ena_gpio, 0);
>> +	else
>> +		skl_int3472_enable_clk_acpi_method(clk, 0);
>>  }
>>
>>  static int skl_int3472_clk_enable(struct clk_hw *hw) @@ -86,6 
>> +119,50 @@ static const struct clk_ops skl_int3472_clock_ops = {
>>  	.recalc_rate = skl_int3472_clk_recalc_rate,  };
>>
>> +int skl_int3472_register_clock_src(struct int3472_discrete_device
>> +*int3472) {
>> +	struct clk_init_data init = {
>> +		.ops = &skl_int3472_clock_ops,
>> +		.flags = CLK_GET_RATE_NOCACHE,
>> +	};
>> +	int ret;
>> +
>> +	if (int3472->clock.cl)
>> +		return -EBUSY;
>> +
>> +	init.name = kasprintf(GFP_KERNEL, "%s-clk",
>> +			      acpi_dev_name(int3472->adev));
>> +	if (!init.name)
>> +		return -ENOMEM;
>> +
>> +	int3472->clock.frequency = skl_int3472_get_clk_frequency(int3472);
>> +	int3472->clock.clk_hw.init = &init;
>> +	int3472->clock.clk = clk_register(&int3472->adev->dev,
>> +					  &int3472->clock.clk_hw);
>> +	if (IS_ERR(int3472->clock.clk)) {
>> +		ret = PTR_ERR(int3472->clock.clk);
>> +		goto out_free_init_name;
>> +	}
>> +
>> +	int3472->clock.cl = clkdev_create(int3472->clock.clk, NULL,
>> +					  int3472->sensor_name);
>> +	if (!int3472->clock.cl) {
>> +		ret = -ENOMEM;
>> +		goto err_unregister_clk;
>> +	}
>> +
>> +	kfree(init.name);
>> +
>> +	return 0;
>> +
>> +err_unregister_clk:
>> +	clk_unregister(int3472->clock.clk);
>> +out_free_init_name:
>> +	kfree(init.name);
>> +
>> +	return ret;
>> +}
>> +
>>  int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
>>  			       struct acpi_resource_gpio *agpio, u32 polarity)
>{ diff
>> --git a/drivers/platform/x86/intel/int3472/common.h
>> b/drivers/platform/x86/intel/int3472/common.h
>> index 61688e450ce5..036b804e8ea5 100644
>> --- a/drivers/platform/x86/intel/int3472/common.h
>> +++ b/drivers/platform/x86/intel/int3472/common.h
>> @@ -64,7 +64,9 @@ struct int3472_cldb {
>>  	u8 control_logic_type;
>>  	u8 control_logic_id;
>>  	u8 sensor_card_sku;
>> -	u8 reserved[28];
>> +	u8 reserved[10];
>> +	u8 clock_source;
>> +	u8 reserved2[17];
>>  };
>>
>>  struct int3472_gpio_function_remap { @@ -100,6 +102,7 @@ struct 
>> int3472_discrete_device {
>>  		struct clk_lookup *cl;
>>  		struct gpio_desc *ena_gpio;
>>  		u32 frequency;
>> +		u8 imgclk_index;
>>  	} clock;
>>
>>  	struct int3472_pled {
>> @@ -123,6 +126,7 @@ int skl_int3472_get_sensor_adev_and_name(struct
>> device *dev,
>>
>>  int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
>>  			       struct acpi_resource_gpio *agpio, u32 polarity);
>> +int skl_int3472_register_clock_src(struct int3472_discrete_device 
>> +*int3472);
>>  void skl_int3472_unregister_clock(struct int3472_discrete_device 
>> *int3472);
>>
>>  int skl_int3472_register_regulator(struct int3472_discrete_device 
>> *int3472, diff --git a/drivers/platform/x86/intel/int3472/discrete.c
>> b/drivers/platform/x86/intel/int3472/discrete.c
>> index ef020e23e596..d5d5c650d6d2 100644
>> --- a/drivers/platform/x86/intel/int3472/discrete.c
>> +++ b/drivers/platform/x86/intel/int3472/discrete.c
>> @@ -309,6 +309,9 @@ static int skl_int3472_parse_crs(struct
>int3472_discrete_device *int3472)
>>  	if (ret < 0)
>>  		return ret;
>>
>> +	/* If no gpio based clk registered, try register with clock source
>*/
>> +	skl_int3472_register_clock_src(int3472);
>> +
>>  	acpi_dev_free_resource_list(&resource_list);
>>
>>  	int3472->gpios.dev_id = int3472->sensor_name; @@ -356,6 +359,7 @@ 
>> static int skl_int3472_discrete_probe(struct platform_device *pdev)
>>  	int3472->adev = adev;
>>  	int3472->dev = &pdev->dev;
>>  	platform_set_drvdata(pdev, int3472);
>> +	int3472->clock.imgclk_index = cldb.clock_source;
>>
>>  	ret = skl_int3472_get_sensor_adev_and_name(&pdev->dev, &int3472- 
>>sensor,
>>  						   &int3472->sensor_name);

[PATCH 3/3] platform/x86: int3472: Add ov13b10 (09B13) sensor module support

[bingbu.cao]

From: Hao Yao <hao.yao@intel.com>

Add a new sensor support in INT3472 driver which module name is '09B13'.

Signed-off-by: Hao Yao <hao.yao@intel.com>
Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index d5d5c650d6d2..63acb48bf8df 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -60,6 +60,8 @@ static const struct int3472_sensor_config int3472_sensor_configs[] = {
 	{ "GEFF150023R", REGULATOR_SUPPLY("avdd", NULL), NULL },
 	/* Surface Go 1&2 - OV5693, Front */
 	{ "YHCU", REGULATOR_SUPPLY("avdd", NULL), NULL },
+	/* OV13B10 */
+	{ "09B13", REGULATOR_SUPPLY("vcc", NULL), NULL },
 };
 
 static const struct int3472_sensor_config *
-- 
2.40.1

Re: [PATCH 3/3] platform/x86: int3472: Add ov13b10 (09B13) sensor module support

[Hans de Goede]

Hi all,

On 5/24/23 05:51, bingbu.cao@intel.com wrote:
> From: Hao Yao <hao.yao@intel.com>
> 
> Add a new sensor support in INT3472 driver which module name is '09B13'.
> 
> Signed-off-by: Hao Yao <hao.yao@intel.com>
> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> ---
>  drivers/platform/x86/intel/int3472/discrete.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index d5d5c650d6d2..63acb48bf8df 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -60,6 +60,8 @@ static const struct int3472_sensor_config int3472_sensor_configs[] = {
>  	{ "GEFF150023R", REGULATOR_SUPPLY("avdd", NULL), NULL },
>  	/* Surface Go 1&2 - OV5693, Front */
>  	{ "YHCU", REGULATOR_SUPPLY("avdd", NULL), NULL },
> +	/* OV13B10 */
> +	{ "09B13", REGULATOR_SUPPLY("vcc", NULL), NULL },

"vcc" is not really a useful supply name. All the existing sensor drivers in drivers/media/i2c typically check for both "avdd" and "dvdd". Can you verify if this is supplying digital or analog power using the schematics of the laptop ?

And then use one of the standard "avdd" or "dvdd" supply names ?

I would like to try and get rid of this table and sofar all the sensors which have a regulator GPIO are listed as sing it for "avdd" so I was hoping to be able to just always use "avdd".

At least I would like us to come up with some default fallback for the supply-name in case a sensor-module is not listed in this table and "avdd" seems to be the best fallback.

Note that if your current sensor driver expects "vcc" that that is not a good reason to go with "vcc" here. It would be better to adjust the sensor driver to use the standard "avdd" and "dvdd" supply names (using the bulk regulator api), like all the other sensor drivers do.

Regards,

Hans

Re: [PATCH 3/3] platform/x86: int3472: Add ov13b10 (09B13) sensor module support

[Dan Scally]

Hi both

On 25/05/2023 19:40, Hans de Goede wrote:
> Hi all,
>
> On 5/24/23 05:51, bingbu.cao@intel.com wrote:
>> From: Hao Yao <hao.yao@intel.com>
>>
>> Add a new sensor support in INT3472 driver which module name is '09B13'.
>>
>> Signed-off-by: Hao Yao <hao.yao@intel.com>
>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
>> ---
>>   drivers/platform/x86/intel/int3472/discrete.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
>> index d5d5c650d6d2..63acb48bf8df 100644
>> --- a/drivers/platform/x86/intel/int3472/discrete.c
>> +++ b/drivers/platform/x86/intel/int3472/discrete.c
>> @@ -60,6 +60,8 @@ static const struct int3472_sensor_config int3472_sensor_configs[] = {
>>   	{ "GEFF150023R", REGULATOR_SUPPLY("avdd", NULL), NULL },
>>   	/* Surface Go 1&2 - OV5693, Front */
>>   	{ "YHCU", REGULATOR_SUPPLY("avdd", NULL), NULL },
>> +	/* OV13B10 */
>> +	{ "09B13", REGULATOR_SUPPLY("vcc", NULL), NULL },
> "vcc" is not really a useful supply name. All the existing sensor drivers in drivers/media/i2c typically check for both "avdd" and "dvdd". Can you verify if this is supplying digital or analog power using the schematics of the laptop ?
>
> And then use one of the standard "avdd" or "dvdd" supply names ?
>
> I would like to try and get rid of this table and sofar all the sensors which have a regulator GPIO are listed as sing it for "avdd" so I was hoping to be able to just always use "avdd".


I agree this is quite unsatisfactory in its current form, but I'm hoping to add the ov7251 in the 
near future; the driver for which uses "vdda" instead, so unfortunately not in line with that.

>
> At least I would like us to come up with some default fallback for the supply-name in case a sensor-module is not listed in this table and "avdd" seems to be the best fallback.


I wonder if it'd be better to simply support regulator_get() calls without a supply name in the 
event that the device only has a single supply associated with it? Then we needn't pick a default at 
all.


>
> Note that if your current sensor driver expects "vcc" that that is not a good reason to go with "vcc" here. It would be better to adjust the sensor driver to use the standard "avdd" and "dvdd" supply names (using the bulk regulator api), like all the other sensor drivers do.
>
> Regards,
>
> Hans
>

Re: [PATCH 3/3] platform/x86: int3472: Add ov13b10 (09B13) sensor module support

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 4678 bytes --]

Hi Dan,

On 5/31/23 17:18, Dan Scally wrote:
> Hi both
> 
> On 25/05/2023 19:40, Hans de Goede wrote:
>> Hi all,
>>
>> On 5/24/23 05:51, bingbu.cao@intel.com wrote:
>>> From: Hao Yao <hao.yao@intel.com>
>>>
>>> Add a new sensor support in INT3472 driver which module name is '09B13'.
>>>
>>> Signed-off-by: Hao Yao <hao.yao@intel.com>
>>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
>>> ---
>>>   drivers/platform/x86/intel/int3472/discrete.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
>>> index d5d5c650d6d2..63acb48bf8df 100644
>>> --- a/drivers/platform/x86/intel/int3472/discrete.c
>>> +++ b/drivers/platform/x86/intel/int3472/discrete.c
>>> @@ -60,6 +60,8 @@ static const struct int3472_sensor_config int3472_sensor_configs[] = {
>>>       { "GEFF150023R", REGULATOR_SUPPLY("avdd", NULL), NULL },
>>>       /* Surface Go 1&2 - OV5693, Front */
>>>       { "YHCU", REGULATOR_SUPPLY("avdd", NULL), NULL },
>>> +    /* OV13B10 */
>>> +    { "09B13", REGULATOR_SUPPLY("vcc", NULL), NULL },
>> "vcc" is not really a useful supply name. All the existing sensor drivers in drivers/media/i2c typically check for both "avdd" and "dvdd". Can you verify if this is supplying digital or analog power using the schematics of the laptop ?
>>
>> And then use one of the standard "avdd" or "dvdd" supply names ?
>>
>> I would like to try and get rid of this table and sofar all the sensors which have a regulator GPIO are listed as sing it for "avdd" so I was hoping to be able to just always use "avdd".
> 
> 
> I agree this is quite unsatisfactory in its current form, but I'm hoping to add the ov7251 in the near future; the driver for which uses "vdda" instead, so unfortunately not in line with that.
> 
>>
>> At least I would like us to come up with some default fallback for the supply-name in case a sensor-module is not listed in this table and "avdd" seems to be the best fallback.
> 
> 
> I wonder if it'd be better to simply support regulator_get() calls without a supply name in the event that the device only has a single supply associated with it? Then we needn't pick a default at all.

I do not believe that the regulator subsystem maintainers will accept such a change. They really only want to touch regulators with full constraints to avoid frying things and this would very much go against that.

I think what we need is for the sensor drivers to use standardized supply-names, so pick one of "avdd" or "vdda" and use that everywhere.

This will require some compat code in some of the sensor drivers to try the old supply name if the new standardized supply name fails (easy when using the bulk regulator API, one of the 2 will just become a dummy regulator), but I believe that in the long run this is the best solution.

Like how we also have all the sensor driver standardized on using "powerdown" and "reset" as GPIO con-ids.

Regards,

Hans


p.s.

Talking about this I just (minutes ago) finished writing a patch for the mainline ov2680 driver which allows dropping the sensor specific GPIO remapping in the int3472 driver for the ov2680, see the attached patch (compile tested only so far). The problem is not with the int3472 code, but that the original ov2680 driver is asking for a "reset" GPIO while the pin is named "XSHUTDN" in the datasheet.

Dan, the reason why I'm poking at the mainline ov2680 driver now is because I have the atomisp code at a point where it can work with standard v4l2 sensor drivers without any atomisp specific API between the atomisp code and the sensor driver. So I want to get rid of the atomisp-ov2680.ko private driver. This involves porting recent improvements like selection API (cropping) support from 
atomisp-ov2680.c to the standard driver.

I was sorta hoping to also test this on a miix510, but upon checking I saw that the mainline ov2680.c does not yet work on the miix510. Porting my atomisp-ov2680.c changes should get us close to having the standard ov2680.c work on the miix510 (ACPI enumeration, runtime-pm and selection API support will all be added).

I have recently bought a second hand miix 510. Long story short: Can you give me some quick instructions, or a docs pointer for testing a new sensor with libcamera ?  Or preferably I guess first outside of libcamera just grabbing raw-bayer + some userspace debayer tool for testing and then later on test under libcamera ?


More p.s. :

Dan what about: https://patchwork.kernel.org/project/platform-driver-x86/patch/20230311223019.14123-1-dan.scally@ideasonboard.com/ and my remarks on that patch from you ?


[-- Attachment #2: 0001-media-ov2680-Check-for-powerdown-GPIO-con-id-before-.patch --]
[-- Type: text/x-patch, Size: 4055 bytes --]

From f7a05e2fb19c3bb3bb704d5540746465cc53125d Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Wed, 31 May 2023 16:53:36 +0200
Subject: [PATCH] media: ov2680: Check for "powerdown" GPIO con-id before
 checking for "reset" GPIO con-id

The datasheet of the OV2680 labels the single GPIO to put the sensor in
powersaving mode as XSHUTDN aka shutdown, _not_ reset.

This is important because some boards have standardized sensor connectors
which allow connecting various sensor modules. These connectors have both
reset and powerdown signals and the powerdown signal is routed to
the OV2680's XSHUTDN pin.

On x86/ACPI multiple Bay Trail, Cherry Trail, Sky Lake and Kaby Lake models
have an OV2680 connected to the ISP2 / IPU3. On these devices the GPIOS are
not described in DT instead the GPIOs are described with an Intel specific
DSM which labels them as either powerdown or reset. Often this DSM returns
both reset and powerdown pins even though the OV2680 has only 1 such pin.

For the ov2680 driver to work on these devices it must use the GPIO with
"powerdown" as con-id, matching the XSHUTDN name from the datasheet.

As for why "powerdown" vs say "shutdown" the ACPI DSM -> con-id mapping
code is shared, so we must use standardized names and currently all of
the following sensor drivers already use "powerdown":
adv7180, gc0310, isl7998x, ov02a10, ov2659, ov5640, ov5648, ov5670,
ov5693, ov7670, ov772x, ov7740, ov8858, ov8865 and ov9650 .

Where as the hi846 driver is the lonely standout using "shutdown".

Try the "powerdown" con-id first to make things work, falling back to
"reset" to keep existing DT setups working.

Cc: Dan Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2680.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index b5283082f96d..e5564ef8f443 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -104,7 +104,7 @@ struct ov2680_dev {
 	u32				xvclk_freq;
 	struct regulator_bulk_data	supplies[OV2680_NUM_SUPPLIES];
 
-	struct gpio_desc		*reset_gpio;
+	struct gpio_desc		*pwdn_gpio;
 	struct mutex			lock; /* protect members */
 
 	bool				mode_pending_changes;
@@ -192,19 +192,19 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
 
 static void ov2680_power_up(struct ov2680_dev *sensor)
 {
-	if (!sensor->reset_gpio)
+	if (!sensor->pwdn_gpio)
 		return;
 
-	gpiod_set_value(sensor->reset_gpio, 0);
+	gpiod_set_value(sensor->pwdn_gpio, 0);
 	usleep_range(5000, 10000);
 }
 
 static void ov2680_power_down(struct ov2680_dev *sensor)
 {
-	if (!sensor->reset_gpio)
+	if (!sensor->pwdn_gpio)
 		return;
 
-	gpiod_set_value(sensor->reset_gpio, 1);
+	gpiod_set_value(sensor->pwdn_gpio, 1);
 	usleep_range(5000, 10000);
 }
 
@@ -441,7 +441,7 @@ static int ov2680_power_on(struct ov2680_dev *sensor)
 		return ret;
 	}
 
-	if (!sensor->reset_gpio) {
+	if (!sensor->pwdn_gpio) {
 		ret = cci_write(sensor->regmap, OV2680_REG_SOFT_RESET, 0x01, NULL);
 		if (ret != 0) {
 			dev_err(dev, "sensor soft reset failed\n");
@@ -917,9 +917,17 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
 	struct device *dev = ov2680_to_dev(sensor);
 	int ret;
 
-	sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset",
-						     GPIOD_OUT_HIGH);
-	ret = PTR_ERR_OR_ZERO(sensor->reset_gpio);
+	/*
+	 * The pin we want is named XSHUTDN in the datasheet. Linux sensor
+	 * drivers have standardized on using "powerdown" as con-id name
+	 * for powerdown or shutdown pins. Older DTB files use "reset",
+	 * so fallback to that if there is no "powerdown" pin.
+	 */
+	sensor->pwdn_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_HIGH);
+	if (!sensor->pwdn_gpio)
+		sensor->pwdn_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+
+	ret = PTR_ERR_OR_ZERO(sensor->pwdn_gpio);
 	if (ret < 0) {
 		dev_dbg(dev, "error while getting reset gpio: %d\n", ret);
 		return ret;
-- 
2.40.1

Re: [PATCH 3/3] platform/x86: int3472: Add ov13b10 (09B13) sensor module support

[Hans de Goede]

Hi Dan,

On 5/31/23 21:34, Dan Scally wrote:
> Hi Hans
> 
> On 31/05/2023 16:34, Hans de Goede wrote:

<snip>

>> I think what we need is for the sensor drivers to use standardized supply-names, so pick one of "avdd" or "vdda" and use that everywhere.
>>
>> This will require some compat code in some of the sensor drivers to try the old supply name if the new standardized supply name fails (easy when using the bulk regulator API, one of the 2 will just become a dummy regulator), but I believe that in the long run this is the best solution.
>>
>> Like how we also have all the sensor driver standardized on using "powerdown" and "reset" as GPIO con-ids.
> 
> I agree that this is ideal long term yes - I'll change the ov7251 to that effect.

Ack.

<snip>

>> I was sorta hoping to also test this on a miix510, but upon checking I saw that the mainline ov2680.c does not yet work on the miix510. Porting my atomisp-ov2680.c changes should get us close to having the standard ov2680.c work on the miix510 (ACPI enumeration, runtime-pm and selection API support will all be added).
> 
> 
> That would be nice. The ov2680 on a Miix510 is the sensor I was originally trying to get working, a very long time ago. I had a tree that "worked" in that you could stream with it but it was very ugly, as I had no idea what I was doing at the time and swiftly got the Surface instead and so moved on to that. The references to OV2680 in the CIO2/INT3472 code are hangovers from that early support really.
> 
>>
>> I have recently bought a second hand miix 510. Long story short: Can you give me some quick instructions, or a docs pointer for testing a new sensor with libcamera ?  Or preferably I guess first outside of libcamera just grabbing raw-bayer + some userspace debayer tool for testing and then later on test under libcamera ?
> 
> 
> Outside of libcamera is a case of configuring the formats and then grabbing frames from the CIO2's devnode, then queuing those frames to the IPU3's input devnode. The easiest way to do it is using the ipu3 utils within the libcamera project, which don't actually use the library itself but rather just configure with media-ctl and capture with yavta. See [1] and [2] for example. Testing within libcamera requires that the driver meet libcamera's requirements [3] which mostly means ensuring it has the 5 mandatory controls, plus some targets for the .get_selection() callback - I can't remember exactly which ones but I think it's CROP, NATIVE_SIZE, CROP_BOUNDS and CROP_DEFAULT. And I think it would also need an entry adding right at [4] to describe the sensor for libcamera. After that the cam tool from libcamera should list it with cam -l and capture from it with cam -cN --capture, or alternatively qcam would display the stream.
> 
> 
> [1] https://git.libcamera.org/libcamera/libcamera.git/tree/utils/ipu3/ipu3-capture.sh
> 
> [2] https://git.libcamera.org/libcamera/libcamera.git/tree/utils/ipu3/ipu3-process.sh
> 
> [3] https://git.libcamera.org/libcamera/libcamera.git/tree/Documentation/sensor_driver_requirements.rst
> 
> [4] https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera_sensor_properties.cpp#n140

Thank you for the testing instructions. I will give this a try once I have all the recent atomisp
improvements ported to the main ov2680 driver and I have the main ov2680 driver working with the atomisp.

>> More p.s. :
>>
>> Dan what about: https://patchwork.kernel.org/project/platform-driver-x86/patch/20230311223019.14123-1-dan.scally@ideasonboard.com/ and my remarks on that patch from you ?
> 
> 
> That one completely fell off my radar - sorry about that. I can pick it up again in the morning.

Note I send a another reply in that thread yesterday with an alternative suggestion for fixing this (I would like to not have to add any GPIO remapping stuff in INT3472 when possible, IMHO the sensor drivers should be patched to use the right con-ids where possible).

Regards,

Hans

Re: [PATCH 3/3] platform/x86: int3472: Add ov13b10 (09B13) sensor module support

[Dan Scally]

Morning Hans

On 01/06/2023 09:33, Hans de Goede wrote:
> Hi Dan,
>
> On 5/31/23 21:34, Dan Scally wrote:
>> Hi Hans
>>
>> On 31/05/2023 16:34, Hans de Goede wrote:
> <snip>
>
>>> I think what we need is for the sensor drivers to use standardized supply-names, so pick one of "avdd" or "vdda" and use that everywhere.
>>>
>>> This will require some compat code in some of the sensor drivers to try the old supply name if the new standardized supply name fails (easy when using the bulk regulator API, one of the 2 will just become a dummy regulator), but I believe that in the long run this is the best solution.
>>>
>>> Like how we also have all the sensor driver standardized on using "powerdown" and "reset" as GPIO con-ids.
>> I agree that this is ideal long term yes - I'll change the ov7251 to that effect.
> Ack.
>
> <snip>
>
>>> I was sorta hoping to also test this on a miix510, but upon checking I saw that the mainline ov2680.c does not yet work on the miix510. Porting my atomisp-ov2680.c changes should get us close to having the standard ov2680.c work on the miix510 (ACPI enumeration, runtime-pm and selection API support will all be added).
>>
>> That would be nice. The ov2680 on a Miix510 is the sensor I was originally trying to get working, a very long time ago. I had a tree that "worked" in that you could stream with it but it was very ugly, as I had no idea what I was doing at the time and swiftly got the Surface instead and so moved on to that. The references to OV2680 in the CIO2/INT3472 code are hangovers from that early support really.
>>
>>> I have recently bought a second hand miix 510. Long story short: Can you give me some quick instructions, or a docs pointer for testing a new sensor with libcamera ?  Or preferably I guess first outside of libcamera just grabbing raw-bayer + some userspace debayer tool for testing and then later on test under libcamera ?
>>
>> Outside of libcamera is a case of configuring the formats and then grabbing frames from the CIO2's devnode, then queuing those frames to the IPU3's input devnode. The easiest way to do it is using the ipu3 utils within the libcamera project, which don't actually use the library itself but rather just configure with media-ctl and capture with yavta. See [1] and [2] for example. Testing within libcamera requires that the driver meet libcamera's requirements [3] which mostly means ensuring it has the 5 mandatory controls, plus some targets for the .get_selection() callback - I can't remember exactly which ones but I think it's CROP, NATIVE_SIZE, CROP_BOUNDS and CROP_DEFAULT. And I think it would also need an entry adding right at [4] to describe the sensor for libcamera. After that the cam tool from libcamera should list it with cam -l and capture from it with cam -cN --capture, or alternatively qcam would display the stream.
>>
>>
>> [1] https://git.libcamera.org/libcamera/libcamera.git/tree/utils/ipu3/ipu3-capture.sh
>>
>> [2] https://git.libcamera.org/libcamera/libcamera.git/tree/utils/ipu3/ipu3-process.sh
>>
>> [3] https://git.libcamera.org/libcamera/libcamera.git/tree/Documentation/sensor_driver_requirements.rst
>>
>> [4] https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera_sensor_properties.cpp#n140
> Thank you for the testing instructions. I will give this a try once I have all the recent atomisp
> improvements ported to the main ov2680 driver and I have the main ov2680 driver working with the atomisp.
>
>>> More p.s. :
>>>
>>> Dan what about: https://patchwork.kernel.org/project/platform-driver-x86/patch/20230311223019.14123-1-dan.scally@ideasonboard.com/ and my remarks on that patch from you ?
>>
>> That one completely fell off my radar - sorry about that. I can pick it up again in the morning.
> Note I send a another reply in that thread yesterday with an alternative suggestion for fixing this (I would like to not have to add any GPIO remapping stuff in INT3472 when possible, IMHO the sensor drivers should be patched to use the right con-ids where possible).


Yep, I saw that and agree with it - I'll drop that patch and raise another for the sensor driver.

> Regards,
>
> Hans
>
>

Re: [PATCH 1/3] platform/x86: int3472: Avoid crash in unregistering regulator gpio

[Andy Shevchenko]

On Wed, May 24, 2023 at 11:51:33AM +0800, bingbu.cao@intel.com wrote:
> From: Hao Yao <hao.yao@intel.com>
> 
> When int3472 is loaded before GPIO driver, acpi_get_and_request_gpiod()
> failed but the returned gpio descriptor is not NULL, it will cause panic
> in later gpiod_put(), so set the gpio_desc to NULL in register error
> handling to avoid such crash.

It should return deferred probe and if somebody is asking for troubles with
it...

Probably you need to assign only available GPIOs by introducing a local variable

	struct gpio_desc *gdesc;

	gdesc = ...
	if (IS_ERR(gdesc))
		...

	...->... = gdesc;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] platform/x86: int3472: Avoid crash in unregistering regulator gpio

[Hans de Goede]

Hi,

On 5/24/23 05:51, bingbu.cao@intel.com wrote:
> From: Hao Yao <hao.yao@intel.com>
> 
> When int3472 is loaded before GPIO driver, acpi_get_and_request_gpiod()
> failed but the returned gpio descriptor is not NULL, it will cause panic
> in later gpiod_put(), so set the gpio_desc to NULL in register error
> handling to avoid such crash.
> 
> Signed-off-by: Hao Yao <hao.yao@intel.com>
> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>

For the clk-enable GPIO this is not really necessary:

void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472)
{
        if (!int3472->clock.cl)
                return;

	...
	gpiod_put(int3472->clock.ena_gpio);
}

The int3472->clock.cl check causes the gpiod_put() to never get called.

But setting both GPIOs to NULL when we fail to get them is consistent,
so I have taken this as is:

Thank you for your patch, I've applied this patch to my fixes
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=fixes

I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.

Regards,

Hans






> ---
>  .../x86/intel/int3472/clk_and_regulator.c        | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> index 1086c3d83494..d1088be5af78 100644
> --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> @@ -101,9 +101,12 @@ int skl_int3472_register_clock(struct int3472_discrete_device *int3472,
>  
>  	int3472->clock.ena_gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0],
>  							     "int3472,clk-enable");
> -	if (IS_ERR(int3472->clock.ena_gpio))
> -		return dev_err_probe(int3472->dev, PTR_ERR(int3472->clock.ena_gpio),
> -				     "getting clk-enable GPIO\n");
> +	if (IS_ERR(int3472->clock.ena_gpio)) {
> +		ret = PTR_ERR(int3472->clock.ena_gpio);
> +		int3472->clock.ena_gpio = NULL;
> +		return dev_err_probe(int3472->dev, ret,
> +				     "failed to get gpio for clock\n");
> +	}
>  
>  	if (polarity == GPIO_ACTIVE_LOW)
>  		gpiod_toggle_active_low(int3472->clock.ena_gpio);
> @@ -199,8 +202,11 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
>  	int3472->regulator.gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0],
>  							     "int3472,regulator");
>  	if (IS_ERR(int3472->regulator.gpio)) {
> -		dev_err(int3472->dev, "Failed to get regulator GPIO line\n");
> -		return PTR_ERR(int3472->regulator.gpio);
> +		ret = PTR_ERR(int3472->regulator.gpio);
> +		int3472->regulator.gpio = NULL;
> +
> +		return dev_err_probe(int3472->dev, ret,
> +				     "failed to get regulator gpio\n");
>  	}
>  
>  	/* Ensure the pin is in output mode and non-active state */