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