| Message ID | 20230524035135.90315-2-bingbu.cao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] platform/x86: int3472: Avoid crash in unregistering regulator gpio | expand |
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; > + }
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);
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);
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);
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);