On Thu, Jan 22, 2009 at 03:04, <[email protected]> wrote:
there's a few places where you have trailing whitespace ... you can
fix that by running `sed 's:[[:space:]]*$::'` on the files in
question.
> Modified: trunk/drivers/net/bfin_mac.c (6031 => 6032)
>
> +#if defined(CONFIG_NET_DSA_KSZ8893M)
> + extern struct dsa_platform_data ksz8893m_switch_data;
> +#endif
> +
> ...
> +#if defined(CONFIG_NET_DSA_KSZ8893M)
> + ksz8893m_switch_data.mii_bus = &(lp->mii_bus->dev);
> +#endif
shouldnt that layer be generic ? are we now going to have to add
ifdef's for every single switch someone hooks up to our mac ? that's
the point of the phylib layer: the indirection is handled there, not
in the mac drivers.
> Added: trunk/net/dsa/ksz8893m.c (0 => 6032)
>
> +static int switch_read_spi(unsigned char *din, unsigned char reg, int len)
> +{
> + struct spi_message message;
> + unsigned char dout[BUF_LEN];
> + struct spi_transfer *t = &sw.xfer;
> + int i;
> +
> + t->len = len;
> + t->tx_buf = dout;
> + t->rx_buf = din;
> + ((unsigned char *)(t->tx_buf))[0] = SPI_READ;
> + ((unsigned char *)(t->tx_buf))[1] = reg;
why not go through dout instead ? then you can avoid those ugly casts
on the left hand side
> +static int switch_write_spi(unsigned char *dout, unsigned char reg, int
> len)
> +{
> + struct spi_message message;
> + unsigned char din[BUF_LEN];
> + struct spi_transfer *t = &sw.xfer;
> +
> + t->len = len;
> + t->tx_buf = dout;
> + t->rx_buf = din;
> + ((unsigned char *)(t->tx_buf))[0] = SPI_WRITE;
> + ((unsigned char *)(t->tx_buf))[1] = reg;
same here
> +static int ksz8893m_switch_reset(struct dsa_switch *ds)
> +{
> + return 0;
> +}
iirc from reading the datasheet, this part does support software reset
via a spi command
> +static int ksz8893m_setup_global(struct dsa_switch *ds)
> +{
> + unsigned char dout[BUF_LEN];
> + unsigned char din[BUF_LEN];
> +
> + /* Set VLAN VID of port1 */
> + switch_read_spi(din, Port1Control3, 3);
> + din[2] &= 0xf0;
> + dout[2] = (DEFAULT_PORT_VID & 0xfff) >>8 | din[2];
> + dout[3] = DEFAULT_PORT_VID & 0xff;
> + switch_write_spi(dout, Port1Control3, 4);
shouldnt you be checking the return values of every spi read/write here ?
> +static int ksz8893m_setup_port(struct dsa_switch *ds, int p)
> +{
> + int val;
> + val = mdiobus_read(ds->master_mii_bus, p, MII_BMCR);
> + /* bit 12 auto-negotiation enabled */
> + val |= 0x1000;
> + /* bit 13 force 100, bit 8 force full duplex */
> + val |= 0x2100;
> + /* bit 11 power on, bit 3 enable auto MDI-X, bit 2 enble far-end fault
> detection */
> + val &= ~0x80c;
> + mdiobus_write(ds->master_mii_bus, p, MII_BMCR, val);
> +
> + val = mdiobus_read(ds->master_mii_bus, p, MII_ADVERTISE);
> + /* bit 8/7/6/5 advertise 100full/100half/10full/10half ability */
> + val |= 0x1e0;
> + mdiobus_write(ds->master_mii_bus, p, MII_ADVERTISE, val);
> + return 0;
> +}
these magic bit values should be defines in the ksz8893m.h header
also, you should be checking the return values of mdiobus_write() in
case of error
> +static int ksz8893m_setup(struct dsa_switch *ds)
> +{
> + int i;
> + int ret;
> +
> + ret = ksz8893m_switch_reset(ds);
> + if (ret < 0)
> + return ret;
> +
> + ret = ksz8893m_setup_global(ds);
> + if (ret < 0)
> + return ret;
> +
> + for (i = 1; i < KSZ8893M_PORT_NUM; i++) {
> + ret = ksz8893m_setup_port(ds, i);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int ksz8893m_port_to_phy_addr(int port)
> +{
> + if (port >= 1 && port <= KSZ8893M_PORT_NUM)
> + return port;
> +
> + printk(KERN_INFO "ksz8893m: use default phy addr 3\n");
> + return 3;
> +}
shouldnt that be a KERN_WARNING ?
> +void ksz8893m_poll_link(struct dsa_switch *ds)
> +{
> ...
> + /* bit 2 link is up */
> + link = !!(val & 0x04);
> + /* bit 4 far-end fault detected */
> + fefd = !!(val & 0x10);
> + /* bit 5 auto-negotiation complete */
> + anc = !!(val & 0x20);
> + }
> ...
> + speed = 10;
> + duplex = 0;
> + val = mdiobus_read(ds->master_mii_bus, i, MII_BMSR);
> + /* bit 14/13/12/11 capable of 100full/100half/10full/10half */
> + val &= 0x7800;
> + if (val & 0x4000) {
> + speed = 100;
> + duplex = 1;
> + }
> + else if (val & 0x2000) {
> + speed = 100;
> + duplex = 0;
> + }
> + else if (val & 0x1000 ) {
> + speed = 10;
> + duplex = 1;
> + }
these magic bits also belong in the header as defines
> +static int __devinit spi_switch_probe(struct spi_device *spi)
> +{
> + memset(&(sw.xfer), 0, sizeof(sw.xfer));
the parenthesis are not needed to use the &
> + sw.dev = spi;
> + return 0;
> +}
drivers are usually written to support multiple devices. since you've
written this to only support one, you should add a sanity check:
if (sw.dev) {
printk(KERN_ERROR "ksz8893: only one instance supported at a time\n");
return 1;
}
> +static int __devexit spi_switch_remove(struct spi_device *spi)
> +{
> + sw.dev = NULL;
> + printk(KERN_INFO "spi switch exit\n");
> + return 0;
> +}
this printk() seems kind of pointless
> +static struct spi_driver spi_switch_driver = {
> + .driver = {
> + .name = "spi_switch",
"spi_switch" is an awful name. it needs to reflect the actual part in
use. ksz8893 in this case.
> +int __init ksz8893m_init(void)
> +{
> + int ret;
> + ret = spi_register_driver(&spi_switch_driver);
> + if (ret) {
> + printk(KERN_ERR "Can't register spi_switch_driver!\n");
printk's should follow the form:
printk(KERN_XXX "driver: message\n");
> Added: trunk/net/dsa/tag_stpid.c (0 => 6032)
>
> +#include "ksz8893m.h"
does this driver actually rely on ksz8893-specific things ? if so,
that should get fixed. if not, the include should be dropped.
-mike
_______________________________________________
Linux-kernel-commits mailing list
[email protected]
http://blackfin.uclinux.org/mailman/listinfo/linux-kernel-commits
