Hi Dan, On 5/31/23 16:45, Dan Scally wrote: > Hello Hans and Bingbu (and thanks for both versions of the patch) > > On 31/05/2023 14:44, Hans de Goede 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. > > > Interesting - is that a common thing? Are there other camera-related resources that are controlled in a similar way? I still don't know how to drive the infrared LED on most Surface platforms, so now I'm wondering if they need something similar doing. > > > It does seem a bit strange for this to be a _DSM method against the INT3472 rather than part of _PS0 against the camera itself - isn't that where you'd usually find such things? > >> >> Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx> >> Signed-off-by: Hao Yao <hao.yao@xxxxxxxxx> >> Link: https://lore.kernel.org/r/20230524035135.90315-2-bingbu.cao@xxxxxxxxx >> --- >> 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 think all these changes are good ones. > >> --- >> .../x86/intel/int3472/clk_and_regulator.c | 89 ++++++++++++++++++- >> drivers/platform/x86/intel/int3472/common.h | 10 ++- >> drivers/platform/x86/intel/int3472/discrete.c | 8 +- >> 3 files changed, 99 insertions(+), 8 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..9bcf8c64b8e7 100644 >> --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c >> +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c >> @@ -11,6 +11,37 @@ >> #include "common.h" >> +/* >> + * 82c0d13a-78c5-4244-9bb1-eb8b539a8d11 >> + * This _DSM GUID allows controlling the sensor clk when it is not controlled >> + * through a GPIO. >> + */ >> +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); >> +} > > > I'm not really sure what error modes something like this might have, but acpi_evaluate_dsm() at least can fail - is there no value in error checking here so that it could be returned by skl_int3472_clk_prepare() below? The problem is that acpi_evaluate_dsm() returns an acpi_object * not a status. And it returns NULL on errors as well as if the _DSM method returns nothing (ACPI equivalent of returning void). And these clk-set calls are expected to return void. The good news is that acpi_evaluate_dsm() does log an error one errors. <snip> >> @@ -100,6 +102,7 @@ struct int3472_discrete_device { >> struct clk_lookup *cl; >> struct gpio_desc *ena_gpio; >> u32 frequency; >> + u8 imgclk_index; >> } clock; > > > This struct is called "int3472_gpio_clock" but perhaps now ought to just be "int3472_clock" Ack I've fixed this up while merging the patch. Regards, Hans
