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

[Alex Elder]

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