On Fri, Nov 20, 2020 at 1:33 PM Andrew Lunn <andrew@xxxxxxx> wrote:
>
> > +static const struct xrs700x_mib xrs700x_mibs[] = {
> > + {XRS_RX_GOOD_OCTETS_L(0), "rx_good_octets"},
> > + {XRS_RX_BAD_OCTETS_L(0), "rx_bad_octets"},
> > + {XRS_RX_UNICAST_L(0), "rx_unicast"},
> > + {XRS_RX_BROADCAST_L(0), "rx_broadcast"},
> > + {XRS_RX_MULTICAST_L(0), "rx_multicast"},
> > + {XRS_RX_UNDERSIZE_L(0), "rx_undersize"},
> > + {XRS_RX_FRAGMENTS_L(0), "rx_fragments"},
> > + {XRS_RX_OVERSIZE_L(0), "rx_oversize"},
> > + {XRS_RX_JABBER_L(0), "rx_jabber"},
> > + {XRS_RX_ERR_L(0), "rx_err"},
> > + {XRS_RX_CRC_L(0), "rx_crc"},
> > + {XRS_RX_64_L(0), "rx_64"},
> > + {XRS_RX_65_127_L(0), "rx_65_127"},
> > + {XRS_RX_128_255_L(0), "rx_128_255"},
> > + {XRS_RX_256_511_L(0), "rx_256_511"},
> > + {XRS_RX_512_1023_L(0), "rx_512_1023"},
> > + {XRS_RX_1024_1536_L(0), "rx_1024_1536"},
> > + {XRS_RX_HSR_PRP_L(0), "rx_hsr_prp"},
> > + {XRS_RX_WRONGLAN_L(0), "rx_wronglan"},
> > + {XRS_RX_DUPLICATE_L(0), "rx_duplicate"},
> > + {XRS_TX_OCTETS_L(0), "tx_octets"},
> > + {XRS_TX_UNICAST_L(0), "tx_unicast"},
> > + {XRS_TX_BROADCAST_L(0), "tx_broadcast"},
> > + {XRS_TX_MULTICAST_L(0), "tx_multicast"},
> > + {XRS_TX_HSR_PRP_L(0), "tx_hsr_prp"},
> > + {XRS_PRIQ_DROP_L(0), "priq_drop"},
> > + {XRS_EARLY_DROP_L(0), "early_drop"},
>
> Can we drop the (0). It does not seem to have any purpose, always
> being 0.
It hurts my OCD when the register macros don't match the same pattern but okay.
>
> > +};
> > +
>
>
> > +static void xrs700x_read_port_counters(struct xrs700x *priv, int port)
> > +{
> > + int i;
> > + struct xrs700x_port *p = &priv->ports[port];
>
> Reverse christmas tree. Please check and fix everywhere.
done. I left the order in xrs700x_setup_regmap_range as is because
they're almost the same length and I want them in the order they're in
in the register. Let me know if that's an issue and I'll change it.
>
> > +static void xrs700x_port_stp_state_set(struct dsa_switch *ds, int port,
> > + u8 state)
> > +{
> > + struct xrs700x *priv = ds->priv;
> > + unsigned int val;
> > +
> > + switch (state) {
> > + case BR_STATE_DISABLED:
> > + val = XRS_PORT_DISABLED;
> > + break;
> > + case BR_STATE_LISTENING:
> > + val = XRS_PORT_DISABLED;
> > + break;
>
> No listening state?
No, just forwarding, learning and disabled. See:
https://www.flexibilis.com/downloads/xrs/SpeedChip_XRS7000_3000_User_Manual.pdf
page 82.
>
> > + case BR_STATE_LEARNING:
> > + val = XRS_PORT_LEARNING;
> > + break;
> > + case BR_STATE_FORWARDING:
> > + val = XRS_PORT_FORWARDING;
> > + break;
> > + case BR_STATE_BLOCKING:
> > + val = XRS_PORT_DISABLED;
> > + break;
>
> Hum. What exactly does XRS_PORT_DISABLED mean? When blocking, it is
> expected you can still send/receive BPDUs.
Datasheet says: "Disabled. Port neither learns MAC addresses nor forwards data."
>
> > +struct xrs700x *xrs700x_switch_alloc(struct device *base, void *priv)
> > +{
> > + struct dsa_switch *ds;
> > + struct xrs700x *dev;
> > +
> > + ds = devm_kzalloc(base, sizeof(*ds), GFP_KERNEL);
> > + if (!ds)
> > + return NULL;
> > +
> > + ds->dev = base;
> > + ds->num_ports = DSA_MAX_PORTS;
>
> Is this needed? detect should fill it in.
Removed. I added this before I added detect and forgot to take it out.
>
> > +int xrs700x_switch_register(struct xrs700x *dev)
> > +{
> > + int ret;
> > + int i;
> > +
> > + ret = xrs700x_detect(dev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = xrs700x_setup_regmap_range(dev);
> > + if (ret)
> > + return ret;
> > +
> > + dev->ports = devm_kzalloc(dev->dev,
> > + sizeof(*dev->ports) * dev->ds->num_ports,
> > + GFP_KERNEL);
> > + if (!dev->ports)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < dev->ds->num_ports; i++) {
> > + ret = xrs700x_alloc_port_mib(dev, i);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + ret = dsa_register_switch(dev->ds);
> > +
> > + if (ret)
> > + cancel_delayed_work_sync(&dev->mib_work);
>
> It would be nice to have to symmetry here. It is not obvious what is
> starting this? It happens in the setup op? So can this be moved
> into the teardown op?
Agreed. Moved to teardown.
>
> > +static int xrs700x_i2c_reg_read(void *context, unsigned int reg,
> > + unsigned int *val)
> > +{
> > + int ret;
> > + unsigned char buf[4];
> > + struct device *dev = context;
> > + struct i2c_client *i2c = to_i2c_client(dev);
> > +
> > + buf[0] = reg >> 23 & 0xff;
> > + buf[1] = reg >> 15 & 0xff;
> > + buf[2] = reg >> 7 & 0xff;
> > + buf[3] = (reg & 0x7f) << 1;
> > +
> > + ret = i2c_master_send(i2c, buf, sizeof(buf));
>
> Are you allowed to perform transfers on stack buffers? I think any I2C
> bus driver using DMA is going to be unhappy.
It should be fine. See the following file, there is a good write up about this:
See Documentation/i2c/dma-considerations.rst
>
> > +static const struct of_device_id xrs700x_i2c_dt_ids[] = {
> > + { .compatible = "arrow,xrs7003" },
> > + { .compatible = "arrow,xrs7004" },
> > + {},
>
> Please validate that the compatible string actually matches the switch
> found. Otherwise we can get into all sorts of horrible backward
> compatibility issues.
Okay. What kind of compatibility issues? Do you have a hypothetical
example? I guess I will just use of_device_is_compatible() to check.
>
> > +static const struct of_device_id xrs700x_mdio_dt_ids[] = {
> > + { .compatible = "arrow,xrs7003" },
> > + { .compatible = "arrow,xrs7004" },
> > + {},
>
> Same here.
>
> Andrew
Thanks
