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@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. I'm talking about enable method when we actually change the clock state. -- With Best Regards, Andy Shevchenko
