Re: [PATCH net v2] net: dsa: lan9303: ensure chip reset and wait for READY status

[Vladimir Oltean <olteanv@gmail.com>]

On Wed, Oct 02, 2024 at 07:12:28PM +0200, A. Sverdlin wrote:
> @@ -866,6 +869,29 @@ static int lan9303_check_device(struct lan9303 *chip)
>  	int ret;
>  	u32 reg;
>  
> +	/*
> +	 * In I2C-managed configurations this polling loop will clash with

netdev coding style is with comments like this: /* In I2C managed configurations...

> +	 * switch's reading of EEPROM right after reset and this behaviour is
> +	 * not configurable. While lan9303_read() already has quite long retry
> +	 * timeout, seems not all cases are being detected as arbitration error.

These arbitration errors happen only after reset? So in theory, after
this patch, we could remove the for() loop from lan9303_read()?

> +	 *
> +	 * According to datasheet, EEPROM loader has 30ms timeout (in case of
> +	 * missing EEPROM).
> +	 *
> +	 * Loading of the largest supported EEPROM is expected to take at least
> +	 * 5.9s.
> +	 */
> +	if (read_poll_timeout(lan9303_read, ret, reg & LAN9303_HW_CFG_READY,

Isn't "reg" uninitialized if "ret" is non-zero? So shouldn't be "ret"
also part of the break condition somehow?

> +			      20000, 6000000, false,
> +			      chip->regmap, LAN9303_HW_CFG, &reg)) {
> +		dev_err(chip->dev, "HW_CFG not ready: 0x%08x\n", reg);
> +		return -ENODEV;

What point is there to mangle the return code from read_poll_timeout()
(-ETIMEDOUT) into -ENODEV, instead of just propagating that?

> +	}
> +	if (ret) {
> +		dev_err(chip->dev, "failed to read HW_CFG reg: %d\n", ret);

%pe, ERR_PTR(ret) is nicer for the average, non-expert in errno.h user.
I see this driver isn't using it, so maybe there's an argument about
consistency, but there's a beginning for everything..

> +		return ret;
> +	}
> +
>  	ret = lan9303_read(chip->regmap, LAN9303_CHIP_REV, &reg);
>  	if (ret) {
>  		dev_err(chip->dev, "failed to read chip revision register: %d\n",
> -- 
> 2.46.2
>