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

Alex Elder
2026-05-03T02:06:33Z

On 5/1/26 4:07 PM, Andrew Lunn wrote:
>> 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.

That was a mistake. I agree with that perspective. These functions
were moved out of the header file because they were only used here.
And in the process, I neglected to drop the inline. Will fix.

>> +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?

You're saying that the REFCLK disable stood out, and you want to
understand what "REFCLK" actually represents?

I believe this is an *output* reference clock signal generated by the
TC9564. Looking at the schematic for the RB3gen2 it leads only to
a test point.

However I want to compare notes with Daniel on Monday about this.

Would it draw less attention if it were named "REFCLKO"?

In any case we can add some reassuring comments.

>
>> +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.

Good suggestion. I'll add that.

Thanks a lot for your review.

-Alex

>
> Andrew