Re: [PATCH net-next v1 2/2] net: phy: realtek: load firmware for RTL8261C

[Andrew Lunn]

> +struct rtl8261x_fw_header {
> +	__le32 main_magic;	/* Main magic number 0x52544C38 ("RTL8") */
> +	__le32 sub_magic;	/* Sub magic number */
> +	__le16 version_major;	/* Major version */
> +	__le16 version_minor;	/* Minor version */
> +	__le16 num_entries;	/* Number of entries */
> +	__le16 reserved;	/* Reserved */
> +	__le32 crc32;		/* CRC32 checksum */
> +} __packed;
> +
> +struct rtl8261x_fw_entry {
> +	__u8  type;		/* Operation type (OP_*) */
> +	__u8  dev;		/* MMD device */
> +	__le16 addr;		/* Register address */
> +	__u8  msb;		/* MSB bit position */
> +	__u8  lsb;		/* LSB bit position */
> +	__le16 value;		/* Value to write/compare */
> +	__le16 timeout_ms;	/* Poll timeout in milliseconds */
> +	__u8  poll_set;		/* Poll for set (1) or clear (0) */
> +	__u8  reserved;		/* Reserved */
> +} __packed;

It looks like __packed should not be needed in both of
these. Everything is aligned so the compiler will naturally pack
them.

> +	sub_magic = le32_to_cpu(hdr->sub_magic);
> +	if (sub_magic != FW_SUB_MAGIC_8261C && sub_magic != FW_SUB_MAGIC_8261D) {
> +		phydev_err(phydev, "Invalid sub magic: 0x%08x\n", sub_magic);
> +		return -EINVAL;
> +	}

Should there be validation you are not trying to install C firmware on
D hardware? And visa versa?

> +static int rtl_phy_write_mmd_bits(struct phy_device *phydev, int devnum,
> +				  u16 reg, u8 msb, u8 lsb, u16 val)
> +{
> +	int ret;
> +	u32 reg_val;
> +
> +	if (msb > 15 || lsb > msb)
> +		return -EINVAL;
> +
> +	ret = phy_read_mmd(phydev, devnum, reg);
> +	if (ret < 0)
> +		return ret;
> +	reg_val = ret;
> +
> +	reg_val &= ~GENMASK(msb, lsb);
> +	reg_val |= (val << lsb) & GENMASK(msb, lsb);

The logic here is not very obvious. Please could you add some kdoc to
explain what this function does.

	Andrew