Re: (subset) [PATCH v12 0/9] Add STM32MP25 PCIe drivers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 6/25/25 06:00, Manivannan Sadhasivam wrote:
On Tue, Jun 24, 2025 at 05:22:06PM -0500, Bjorn Helgaas wrote:
On Mon, Jun 23, 2025 at 06:13:07AM -0600, Manivannan Sadhasivam wrote:
On Tue, 10 Jun 2025 11:07:05 +0200, Christian Bruel wrote:
Changes in v12;
    Fix warning reported by kernel test robot <lkp@xxxxxxxxx>

Changes in v11;
    Address comments from Manivanna:
    - RC driver: Do not call pm_runtime_get_noresume in probe
                 More uses of dev_err_probe
    - EP driver: Use level triggered PERST# irq

[...]

Applied, thanks!

[1/9] dt-bindings: PCI: Add STM32MP25 PCIe Root Complex bindings
       commit: 41d5cfbdda7a61c5d646a54035b697205cff1cf0
[2/9] PCI: stm32: Add PCIe host support for STM32MP25
       commit: f6111bc2d8fe6ffc741661126a2174523124dc11
[3/9] dt-bindings: PCI: Add STM32MP25 PCIe Endpoint bindings
       commit: 203cfc4a23506ffb9c48d1300348c290dbf9368e
[4/9] PCI: stm32: Add PCIe Endpoint support for STM32MP25
       commit: 8869fb36a107a9ff18dab8c224de6afff1e81dec
[5/9] MAINTAINERS: add entry for ST STM32MP25 PCIe drivers
       commit: 003902ed7778d62083120253cd282a9112674986

This doesn't build for me with the attached config:

   $ make drivers/pci/controller/dwc/pcie-stm32.o
     CALL    scripts/checksyscalls.sh
     DESCEND objtool
     INSTALL libsubcmd_headers
     CC      drivers/pci/controller/dwc/pcie-stm32.o
   drivers/pci/controller/dwc/pcie-stm32.c: In function ‘stm32_pcie_suspend_noirq’:
   drivers/pci/controller/dwc/pcie-stm32.c:83:16: error: implicit declaration of function ‘pinctrl_pm_select_sleep_state’ [-Werror=implicit-function-declaration]
      83 |         return pinctrl_pm_select_sleep_state(dev);
	|                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/controller/dwc/pcie-stm32.c: In function ‘stm32_pcie_resume_noirq’:
   drivers/pci/controller/dwc/pcie-stm32.c:96:24: error: ‘structdevice’ has no member named ‘pins’
      96 |         if (!IS_ERR(dev->pins->init_state))
	|                        ^~
   drivers/pci/controller/dwc/pcie-stm32.c:97:23: error: implicit declaration of function ‘pinctrl_select_state’ [-Werror=implicit-function-declaration]
      97 |                 ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
	|                       ^~~~~~~~~~~~~~~~~~~~
   drivers/pci/controller/dwc/pcie-stm32.c:97:47: error: ‘structdevice’ has no member named ‘pins’
      97 |                 ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
	|                                               ^~
   drivers/pci/controller/dwc/pcie-stm32.c:97:61: error: ‘structdevice’ has no member named ‘pins’
      97 |                 ret = pinctrl_select_state(dev->pins->p, dev->pins->init_state);
	|                                                             ^~
   drivers/pci/controller/dwc/pcie-stm32.c:99:23: error: implicit declaration of function ‘pinctrl_pm_select_default_state’ [-Werror=implicit-function-declaration]
      99 |                 ret = pinctrl_pm_select_default_state(dev);
	|                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Hmm... I see two issues here. First is, wrong pinctrl header used. The correct
one is:

#include <linux/pinctrl/consumer.h>

ah yes, the missing pinctrl_pm_select_default_state() should indeed be fixed by using the correct header.


Second issue is the driver accessing "struct device::pins" directly. The "pins"
member won't be available if CONFIG_PINCTRL is not set (which is what your
.config has). So either the member should not be accessed directly or the
driver has to depend on CONFIG_PINCTRL. The latter one is not acceptable.It
also looks weird that only this driver is accessing the "pins" member directly
apart from the pinctrl core. So I think this part needs a revisit.

Christian?
The pinctrl "init" and "default" configurations are managed effectively by the probing code. The same approach is required in stm32_pcie_resume_noirq().

In this case, would introducing a new helper function, pinctrl_pm_select_init_state(), be preferable, even if we are the only consumer?

Thank you



- Mani





[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux