Re: [PATCH v2] platform/x86: int3472: Evaluate device's _DSM method to control imaging clock

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 6/6/23 13:26, Andy Shevchenko wrote:
> On Tue, Jun 6, 2023 at 12:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 5/31/23 19:56, Andy Shevchenko wrote:
>>> On Wed, May 31, 2023 at 4:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> ...
> 
>>>> +       if (clk->ena_gpio)
>>>> +               gpiod_set_value_cansleep(clk->ena_gpio, 1);
>>>> +       else
>>>> +               skl_int3472_enable_clk_acpi_method(clk, 1);
>>>
>>> Looking at this, can we avoid duplicative validation of the GPIO?
>>> Perhaps skl_int3472_enable_clk_acpi_method() can have embedded another
>>> check so they won't be called together?
>>>
>>> ...
>>>
>>>> +       if (clk->ena_gpio)
>>>> +               gpiod_set_value_cansleep(clk->ena_gpio, 0);
>>>> +       else
>>>> +               skl_int3472_enable_clk_acpi_method(clk, 0);
>>>
>>> Ditto.
>>
>> Ack, I've squashed a fix for this into this patch while merging it into
>> my review-hans branch.
> 
> Now you have two different checks for the same, I would suggest that
> you switch to check clk->cl in the skl_int3472_enable_clk()
> as it's done in the skl_int3472_register_dsm_clock() instead of GPIO.
> Other way around is also possible but it seems to me that checking for
> clock existence has better guarantees than just checking for GPIO
> availability.

Hmm, you mean the:

        if (int3472->clock.cl)
                return 0; /* A GPIO controlled clk has already been registered */

Check in skl_int3472_register_dsm_clock() ? That matches with / aligns with
this check:

        if (int3472->clock.cl)
                return -EBUSY;

in skl_int3472_register_gpio_clock().

To me it seems sensible to align the checks for "a clk has already been
registered" up this way.

Regards,

Hans