Re: [PATCH net-next v4 2/2] net: pse-pd: Add Si3474 PSE controller driver

[Paolo Abeni <pabeni@redhat.com>]

On 6/30/25 4:57 PM, Piotr Kubik wrote:
> +static inline void si3474_get_channels(struct si3474_priv *priv, int id,
> +				       u8 *chan0, u8 *chan1)
> +{

Please don't use 'static inline' in c files. 'static' would do and will
let the compiler do the better choice.

> +	*chan0 = priv->pi[id].chan[0];
> +	*chan1 = priv->pi[id].chan[1];
> +}
> +
> +static inline struct i2c_client *si3474_get_chan_client(struct si3474_priv *priv,
> +							u8 chan)

Same as above.

[...]
> +static int si3474_pi_enable(struct pse_controller_dev *pcdev, int id)
> +{
> +	struct si3474_priv *priv = to_si3474_priv(pcdev);
> +	struct i2c_client *client;
> +	u8 chan0, chan1;
> +	u8 val = 0;
> +	s32 ret;
> +
> +	if (id >= SI3474_MAX_CHANS)
> +		return -ERANGE;
> +
> +	si3474_get_channels(priv, id, &chan0, &chan1);
> +	client = si3474_get_chan_client(priv, chan0);
> +
> +	/* Release PI from shutdown */
> +	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = (u8)ret;
> +	val |= CHAN_MASK(chan0);
> +	val |= CHAN_MASK(chan1);
> +
> +	ret = i2c_smbus_write_byte_data(client, PORT_MODE_REG, val);
> +	if (ret)
> +		return ret;
> +
> +	/* DETECT_CLASS_ENABLE must be set when using AUTO mode,
> +	 * otherwise PI does not power up - datasheet section 2.10.2
> +	 */
> +	val = (CHAN_BIT(chan0) | CHAN_UPPER_BIT(chan0) |
> +	       CHAN_BIT(chan1) | CHAN_UPPER_BIT(chan1));

Minor nit: brackets not needed above.

> +	ret = i2c_smbus_write_byte_data(client, DETECT_CLASS_ENABLE_REG, val);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int si3474_pi_disable(struct pse_controller_dev *pcdev, int id)
> +{
> +	struct si3474_priv *priv = to_si3474_priv(pcdev);
> +	struct i2c_client *client;
> +	u8 chan0, chan1;
> +	u8 val = 0;
> +	s32 ret;
> +
> +	if (id >= SI3474_MAX_CHANS)
> +		return -ERANGE;
> +
> +	si3474_get_channels(priv, id, &chan0, &chan1);
> +	client = si3474_get_chan_client(priv, chan0);
> +
> +	/* Set PI in shutdown mode */
> +	ret = i2c_smbus_read_byte_data(client, PORT_MODE_REG);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = (u8)ret;
> +	val &= ~(CHAN_MASK(chan0));
> +	val &= ~(CHAN_MASK(chan1));

Brackets not needed here, too and adding them makes the code IMHO less
readable.

> +
> +	ret = i2c_smbus_write_byte_data(client, PORT_MODE_REG, val);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int si3474_pi_get_chan_current(struct si3474_priv *priv, u8 chan)
> +{
> +	struct i2c_client *client;
> +	s32 ret;
> +	u8 reg;
> +	u64 tmp_64;

Please respect the reverse christmass tree order in variable
declaration, here and elsewhere.

/P