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);
