Re: [PATCH net-next v3 9/9] net: sunhme: Consolidate common probe tasks

[Simon Horman <simon.horman@corigine.com>]

On Mon, Mar 13, 2023 at 08:36:13PM -0400, Sean Anderson wrote:
> Most of the second half of the PCI/SBUS probe functions are the same.
> Consolidate them into a common function.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
> 
> (no changes since v1)
> 
>  drivers/net/ethernet/sun/sunhme.c | 183 ++++++++++++------------------
>  1 file changed, 71 insertions(+), 112 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
> index a59b998062d9..a384b162c46d 100644
> --- a/drivers/net/ethernet/sun/sunhme.c
> +++ b/drivers/net/ethernet/sun/sunhme.c
> @@ -2430,6 +2430,71 @@ static void happy_meal_addr_init(struct happy_meal *hp,
>  	}
>  }
>  
> +static int happy_meal_common_probe(struct happy_meal *hp,
> +				   struct device_node *dp, int minor_rev)
> +{
> +	struct net_device *dev = hp->dev;
> +	int err;
> +
> +#ifdef CONFIG_SPARC
> +	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> +	if (hp->hm_revision == 0xff)
> +		hp->hm_revision = 0xc0 | minor_rev;
> +#else
> +	/* works with this on non-sparc hosts */
> +	hp->hm_revision = 0x20;
> +#endif

...

> +#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
> +	/* Hook up SBUS register/descriptor accessors. */
> +	hp->read_desc32 = sbus_hme_read_desc32;
> +	hp->write_txd = sbus_hme_write_txd;
> +	hp->write_rxd = sbus_hme_write_rxd;
> +	hp->read32 = sbus_hme_read32;
> +	hp->write32 = sbus_hme_write32;
> +#endif

This looks correct for the SBUS case.
But I'm not sure about the PCIE case.

gcc 12 tells me when compiling with sparc allmodconfig that the following
functions are now unused.

  pci_hme_read_desc32
  pci_hme_write_txd
  pci_hme_write_rxd
  pci_hme_read32
  pci_hme_write32

> +
> +	/* Grrr, Happy Meal comes up by default not advertising
> +	 * full duplex 100baseT capabilities, fix this.
b
> +	 */
> +	spin_lock_irq(&hp->happy_lock);
> +	happy_meal_set_initial_advertisement(hp);
> +	spin_unlock_irq(&hp->happy_lock);
> +
> +	err = devm_register_netdev(hp->dma_dev, dev);
> +	if (err)
> +		dev_err(hp->dma_dev, "Cannot register net device, aborting.\n");
> +	return err;
> +}
> +
>  #ifdef CONFIG_SBUS
>  static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
>  {

> @@ -2511,70 +2576,18 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
>  		goto err_out_clear_quattro;
>  	}
>  
> -	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> -	if (hp->hm_revision == 0xff)
> -		hp->hm_revision = 0xa0;

It's not clear to me that the same value will be set by the call to
happy_meal_common_probe(hp, dp, 0); where the logic is:

#ifdef CONFIG_SPARC
	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
	if (hp->hm_revision == 0xff)
		hp->hm_revision = 0xc0 | minor_rev;
#else
	/* works with this on non-sparc hosts */
	hp->hm_revision = 0x20;
#endif

I am assuming that the SPARC logic is run.
But another question: is it strictly true that SBUS means SPARC?

...

> -
> -#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
> -	/* Hook up SBUS register/descriptor accessors. */
> -	hp->read_desc32 = sbus_hme_read_desc32;
> -	hp->write_txd = sbus_hme_write_txd;
> -	hp->write_rxd = sbus_hme_write_rxd;
> -	hp->read32 = sbus_hme_read32;
> -	hp->write32 = sbus_hme_write32;
> -#endif

...

> @@ -2689,21 +2702,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
>  	hp->bigmacregs = (hpreg_base + 0x6000UL);
>  	hp->tcvregs    = (hpreg_base + 0x7000UL);
>  
> -#ifdef CONFIG_SPARC
> -	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> -	if (hp->hm_revision == 0xff)
> -		hp->hm_revision = 0xc0 | (pdev->revision & 0x0f);
> -#else
> -	/* works with this on non-sparc hosts */
> -	hp->hm_revision = 0x20;
> -#endif

...

> -#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
> -	/* Hook up PCI register/descriptor accessors. */
> -	hp->read_desc32 = pci_hme_read_desc32;
> -	hp->write_txd = pci_hme_write_txd;
> -	hp->write_rxd = pci_hme_write_rxd;
> -	hp->read32 = pci_hme_read32;
> -	hp->write32 = pci_hme_write32;
> -#endif

...