HI, On 5/31/23 19:56, Andy Shevchenko wrote: > On Wed, May 31, 2023 at 4:44 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >> >> From: Bingbu Cao <bingbu.cao@xxxxxxxxx> >> >> On some platforms, the imaging clock should be controlled by evaluating >> specific clock device's _DSM method instead of setting gpio, so this >> change register clock if no gpio based clock and then use the _DSM method >> to enable and disable clock. > > ... > >> + 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. Regards, Hans