From: Hans de Goede <hdegoede@redhat.com>
To: bingbu.cao@intel.com, djrscally@gmail.com,
dan.scally@ideasonboard.com, 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
Date: Wed, 31 May 2023 15:44:26 +0200 [thread overview]
Message-ID: <6baeafb2-29bf-ab70-2e4e-eea55d6af440@redhat.com> (raw)
In-Reply-To: <20230524035135.90315-2-bingbu.cao@intel.com>
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);
next prev parent reply other threads:[~2023-05-31 14:27 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-24 3:51 [PATCH 1/3] platform/x86: int3472: Avoid crash in unregistering regulator gpio bingbu.cao
2023-05-24 3:51 ` [PATCH 2/3] platform/x86: int3472: Evaluate device's _DSM method to control imaging clock bingbu.cao
2023-05-24 10:31 ` Andy Shevchenko
2023-05-31 13:44 ` Hans de Goede [this message]
2023-06-01 6:24 ` Cao, Bingbu
2023-06-02 4:15 ` Yao, Hao
2023-05-24 3:51 ` [PATCH 3/3] platform/x86: int3472: Add ov13b10 (09B13) sensor module support bingbu.cao
2023-05-25 18:40 ` Hans de Goede
2023-05-31 15:18 ` Dan Scally
2023-05-31 15:34 ` Hans de Goede
[not found] ` <8183d5d6-8707-1ebb-4c47-49f35483fdc5@ideasonboard.com>
2023-06-01 8:33 ` Hans de Goede
2023-06-01 8:41 ` Dan Scally
2023-05-24 10:26 ` [PATCH 1/3] platform/x86: int3472: Avoid crash in unregistering regulator gpio Andy Shevchenko
2023-05-30 10:12 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6baeafb2-29bf-ab70-2e4e-eea55d6af440@redhat.com \
--to=hdegoede@redhat.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=bingbu.cao@intel.com \
--cc=bingbu.cao@linux.intel.com \
--cc=dan.scally@ideasonboard.com \
--cc=djrscally@gmail.com \
--cc=hao.yao@intel.com \
--cc=linux-media@vger.kernel.org \
--cc=markgross@kernel.org \
--cc=sakari.ailus@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).