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. -- With Best Regards, Andy Shevchenko
