On 1/22/24 22:36, Andy Shevchenko wrote: > On Mon, Jan 22, 2024 at 5:30 PM Thomas Richard > <thomas.richard@xxxxxxxxxxx> wrote: >> On 1/15/24 21:13, Andy Shevchenko wrote: >>> On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard >>> <thomas.richard@xxxxxxxxxxx> wrote: > > ... > >>>> + if (pcie->reset_gpio) >>> >>> Dup, why? >> >> This pcie->reset_gpio corresponds to PERST# of PCIe endpoints. >> I assert it during suspend, because I have to deassert it (with a delay) >> during resume stage [1]. > > Ah, sorry for being unclear, I meant that gpiod_set_value*() already > has that check, you don't need it here. ok understood, check is useless. > >>>> + gpiod_set_value_cansleep(pcie->reset_gpio, 0); > > ... > >>>> + if (pcie->reset_gpio) { >>>> + usleep_range(100, 200); >>> >>> fsleep() ? >>> Btw, why is it needed here, perhaps a comment? >> >> The comment should be the same than in the probe [1]. >> Should I copy it? Or should I just add a reference to the probe? >> >> [1] >> https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/pci/controller/cadence/pci-j721e.c#L535 > > Either way works for me. > >>>> + gpiod_set_value_cansleep(pcie->reset_gpio, 1); >>>> + } > > ... > >>>> +#define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie) >>> >>> Is container_of.h included in this file? >> >> linux/container_of.h is included in linux/kernel.h. >> And linux/kernel.h is included in pcie-cadence.h >> (https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/pci/controller/cadence/pcie-cadence.h#L9). > > Okay, so, try to clean up pcie-cadence.h so it won't use "proxy" headers. > There is an IWYU (include what you use) principle, please follow it. In fact, as cdns_pcie_to_rc is only used in pci-j721e.c, no need to have it in a header file. I prefer to move cdns_pcie_to_rc definition in pci-j721e.c. As I don't modify pcie-cadence.h, the cleanup to not use proxy headers is something that it can be done outside this series. -- Thomas Richard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com