Re: [PATCH net-next v6 17/17] net: pse-pd: Add TI TPS23881 PSE controller driver

From: Andrew Lunn
Date: Thu Mar 28 2024 - 12:18:23 EST


> +static int
> +tps23881_get_unused_chan(struct tps23881_port_matrix port_matrix[TPS23881_MAX_CHANS],
> + int port_cnt)
> +{
> + bool used;
> + int i, j;
> +
> + for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> + used = false;
> +
> + for (j = 0; j < port_cnt; j++) {
> + if (port_matrix[j].hw_chan[0] == i) {
> + used = true;
> + break;
> + }
> +
> + if (port_matrix[j].is_4p &&
> + port_matrix[j].hw_chan[1] == i) {
> + used = true;
> + break;
> + }
> + }
> +
> + if (!used)
> + return i;
> + }
> +
> + return -1;

nitpick: Return -ENODEV.

> + while (cnt_4ch_grp1 < 4) {
> + ret = tps23881_get_unused_chan(tmp_port_matrix, port_cnt);
> + if (ret < 0) {
> + pr_err("tps23881: port matrix issue, no chan available\n");
> + return -ENODEV;

and then just returns ret.

> +static int
> +tps23881_set_ports_conf(struct tps23881_priv *priv,
> + struct tps23881_port_matrix port_matrix[TPS23881_MAX_CHANS])
> +{
> + struct i2c_client *client = priv->client;
> + int i, ret;
> + u16 val;
> +
> + /* Set operating mode */
> + ret = i2c_smbus_write_word_data(client, TPS23881_REG_OP_MODE, 0xaaaa);

Could you add some #defines here? This is semiauto i think?

> + if (ret)
> + return ret;
> +
> + /* Disable DC disconnect */
> + ret = i2c_smbus_write_word_data(client, TPS23881_REG_DIS_EN, 0x0);
> + if (ret)
> + return ret;
> +
> + /* Set port power allocation */
> + val = 0;
> + for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> + if (!port_matrix[i].exist)
> + continue;
> +
> + if (port_matrix[i].is_4p)
> + val |= 0xf << ((port_matrix[i].lgcl_chan[0] / 2) * 4);
> + else
> + val |= 0x3 << ((port_matrix[i].lgcl_chan[0] / 2) * 4);
> + }
> + ret = i2c_smbus_write_word_data(client, TPS23881_REG_PORT_POWER, val);
> + if (ret)
> + return ret;
> +
> + /* Enable detection and classification */
> + val = 0;
> + for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> + if (!port_matrix[i].exist)
> + continue;
> +
> + val |= BIT(port_matrix[i].lgcl_chan[0]) |
> + BIT(port_matrix[i].lgcl_chan[0] + 4);
> + if (port_matrix[i].is_4p)
> + val |= BIT(port_matrix[i].lgcl_chan[1]) |
> + BIT(port_matrix[i].lgcl_chan[1] + 4);
> + }
> + ret = i2c_smbus_write_word_data(client, TPS23881_REG_DET_CLA_EN, 0xffff);

This looks odd. You calculate val, and then don't use it?

Andrew