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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx> 
Sent: Thursday, June 1, 2023 2:24 PM
To: Hans de Goede <hdegoede@xxxxxxxxxx>; djrscally@xxxxxxxxx; dan.scally@xxxxxxxxxxxxxxxx; Yao, Hao <hao.yao@xxxxxxxxx>
Cc: markgross@xxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx; andriy.shevchenko@xxxxxxxxxxxxxxx; bingbu.cao@xxxxxxxxxxxxxxx
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@xxxxxxxxxx>
>Sent: Wednesday, May 31, 2023 21:44
>To: Cao, Bingbu <bingbu.cao@xxxxxxxxx>; djrscally@xxxxxxxxx; 
>dan.scally@xxxxxxxxxxxxxxxx; Yao, Hao <hao.yao@xxxxxxxxx>
>Cc: markgross@xxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx; 
>sakari.ailus@xxxxxxxxxxxxxxx; andriy.shevchenko@xxxxxxxxxxxxxxx;
>bingbu.cao@xxxxxxxxxxxxxxx
>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@xxxxxxxxx wrote:
>> From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> Signed-off-by: Hao Yao <hao.yao@xxxxxxxxx>
>
>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);





[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux