Re: [PATCH v1 net-next] lan743x: Provide Read/Write Access to on chip OTP

[Andrew Lunn <andrew@lunn.ch>]

> +static int lan743x_otp_write(struct lan743x_adapter *adapter, u32 offset,
> +			     u32 length, u8 *data)
> +{
> +	int ret;
> +	int i;
> +
> +	ret = lan743x_otp_power_up(adapter);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = lan743x_otp_wait_till_not_busy(adapter);
> +	if (ret < 0)
> +		return ret;
> +
>  	/* set to BYTE program mode */
>  	lan743x_csr_write(adapter, OTP_PRGM_MODE, OTP_PRGM_MODE_BYTE_);
>  
>  	for (i = 0; i < length; i++) {

I think you now need to verify length. It might be possible to change
from EEPROM to OTP after lan743x_ethtool_get_eeprom_len() has been
called, so you have been asked to write EEPROM length into the OTP.
The same is also true for read.

> -		lan743x_csr_write(adapter, OTP_ADDR1,
> -				  ((offset + i) >> 8) &
> -				  OTP_ADDR1_15_11_MASK_);
> -		lan743x_csr_write(adapter, OTP_ADDR2,
> -				  ((offset + i) &
> -				  OTP_ADDR2_10_3_MASK_));
> +		lan743x_otp_set_address(adapter, offset + i);
> +
>  		lan743x_csr_write(adapter, OTP_PRGM_DATA, data[i]);
>  		lan743x_csr_write(adapter, OTP_TST_CMD, OTP_TST_CMD_PRGVRFY_);
>  		lan743x_csr_write(adapter, OTP_CMD_GO, OTP_CMD_GO_GO_);
>  
> -		timeout = jiffies + HZ;
> -		do {
> -			udelay(1);
> -			buf = lan743x_csr_read(adapter, OTP_STATUS);
> -			if (time_after(jiffies, timeout)) {
> -				netif_warn(adapter, drv, adapter->netdev,
> -					   "Timeout on OTP_STATUS completion\n");
> -				return -EIO;
> -			}
> -		} while (buf & OTP_STATUS_BUSY_);
> +		ret = lan743x_otp_wait_till_not_busy(adapter);
> +		if (ret < 0)
> +			return ret;
>  	}
>  
> +	lan743x_otp_power_down(adapter);
> +
>  	return 0;
>  }
>  
>  static int lan743x_ethtool_set_eeprom(struct net_device *netdev,
> @@ -224,17 +306,18 @@ static int lan743x_ethtool_set_eeprom(struct net_device *netdev,
>  	struct lan743x_adapter *adapter = netdev_priv(netdev);
>  	int ret = -EINVAL;
>  
> -	if (ee->magic == LAN743X_EEPROM_MAGIC)
> -		ret = lan743x_eeprom_write(adapter, ee->offset, ee->len,
> -					   data);
> -	/* Beware!  OTP is One Time Programming ONLY!
> -	 * So do some strict condition check before messing up
> -	 */
> -	else if ((ee->magic == LAN743X_OTP_MAGIC) &&
> -		 (ee->offset == 0) &&
> -		 (ee->len == MAX_EEPROM_SIZE) &&
> -		 (data[0] == OTP_INDICATOR_1))
> -		ret = lan743x_otp_write(adapter, ee->offset, ee->len, data);
> +	if (adapter->flags & LAN743X_ADAPTER_FLAG_OTP) {
> +		/* Beware!  OTP is One Time Programming ONLY! */
> +		if (ee->magic == LAN743X_OTP_MAGIC) {
> +			ret = lan743x_otp_write(adapter, ee->offset,
> +						ee->len, data);
> +		}
> +	} else {
> +		if (ee->magic == LAN743X_EEPROM_MAGIC) {
> +			ret = lan743x_eeprom_write(adapter, ee->offset,
> +						   ee->len, data);
> +		}
> +	}

This is breaking backwards compatibility. I think you need to respect
the magic value, independent of how adapter->flags.
  
  Andrew