Re: [PATCH v2] 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 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





[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