Re: [PATCH net-next 11/12] misc: tc956x_pci: add TC956x/QPS615 support

Andrew Lunn
2026-05-01T21:07:43Z

> diff --git a/drivers/misc/tc956x_pci.c b/drivers/misc/tc956x_pci.c

> +static inline void chip_reset_assert(const struct tc956x_chip *chip,
> + enum reset_id id)
> +{
> + tc956x_reset_clock_set(chip, true, true, true, (u8)id);
> +}

This is in drivers/misc, where the rules might be different. But in
netdev, we don't like inline functions in .c files. It is better to
let the compiler decide.

> +static void chip_init_state(struct tc956x_chip *chip)
> +{
> + /* The only IP block we currently use is MSIGEN */
> + chip_reset_assert(chip, RESET_MCU);
> + chip_reset_assert(chip, RESET_MCU1);
> + chip_reset_assert(chip, RESET_INTC);
> + chip_reset_assert(chip, RESET_UART0);
> + chip_clock_disable(chip, CLOCK_MCU);
> + chip_clock_disable(chip, CLOCK_SRAM);
> + chip_clock_disable(chip, CLOCK_PLL);
> + chip_clock_disable(chip, CLOCK_SGMII);

With my networking hat on, this one standard out.

> + chip_clock_disable(chip, CLOCK_REFCLK);

The name REFCLK is sometimes used as for the clock signals for RGMII?

> +static int
> +tc956x_function_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> + struct device *dev = &pdev->dev;
> + struct tc956x_chip *chip;
> + unsigned int msigen_irq;
> + int ret;
> +
> + /* Despite being a PCI device, we require devicetree */
> + if (!dev->of_node)
> + return -EINVAL;

Might be worth a dev_err(), since it is unusual.

Andrew