Hi,
On 6/6/23 13:26, Andy Shevchenko wrote:
> On Tue, Jun 6, 2023 at 12:23 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>> On 5/31/23 19:56, Andy Shevchenko wrote:
>>> On Wed, May 31, 2023 at 4:44 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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
