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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Tue, Jun 06, 2023 at 03:13:47PM +0200, Hans de Goede wrote:
> 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.

I'm talking about enable method when we actually change the clock state.

-- 
With Best Regards,
Andy Shevchenko