Re: [PATCH net-next v10 02/11] net: ti: prueth: Adds ICSSM Ethernet driver

[Jakub Kicinski <kuba@kernel.org>]

On Wed,  2 Jul 2025 19:36:24 +0530 Parvathi Pudi wrote:
> +		if (ret < 0) {
> +			dev_err(dev, "%pOF error reading port_id %d\n",
> +				eth_node, ret);
> +		}

unnecessary parenthesis, but also did you mean to error out here?

> +			dev_err(dev, "port reg should be 0 or 1\n");
> +			of_node_put(eth_node);

this error will also trigger if same port is specified multiple times

+			ret = PTR_ERR(prueth->pru1);
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "unable to get PRU1: %d\n", ret);
+			goto put_pru;

dev_err_probe() ?

> +/**
> + * struct prueth_private_data - PRU Ethernet private data
> + * @fw_pru: firmware names to be used for PRUSS ethernet usecases
> + * @support_lre: boolean to indicate if lre is enabled
> + * @support_switch: boolean to indicate if switch is enabled

Please improve or remove this, adding kdoc which doesn't explain
anything is discouraged per kernel coding style.

This one is actually more confusing than helpful the fields are
called "support" but kdoc says "enabled". Maybe name the fields
'enabled' ?

> + */
> +struct prueth_private_data {
> +	const struct prueth_firmware fw_pru[PRUSS_NUM_PRUS];
> +	bool support_lre;
> +	bool support_switch;
> +};
> +
> +/* data for each emac port */
> +struct prueth_emac {
> +	struct prueth *prueth;
> +	struct net_device *ndev;
> +
> +	struct rproc *pru;
> +	struct phy_device *phydev;
> +
> +	int link;
> +	int speed;
> +	int duplex;
> +
> +	enum prueth_port port_id;
> +	const char *phy_id;
> +	u8 mac_addr[6];
> +	phy_interface_t phy_if;
> +	spinlock_t lock;	/* serialize access */

'serialize access' to what? Which fields does it protect?
-- 
pw-bot: cr