Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1728714 > unrolled thread

Re: [PATCH RFC 3/5] Add KSZ8795 switch driver

Started byPavel Machek <pavel@ucw.cz>
First post2017-09-08 11:20 +0200
Last post2017-09-09 17:50 +0200
Articles 7 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RFC 3/5] Add KSZ8795 switch driver Pavel Machek <pavel@ucw.cz> - 2017-09-08 11:20 +0200
    RE: [PATCH RFC 3/5] Add KSZ8795 switch driver <Tristram.Ha@microchip.com> - 2017-09-08 20:00 +0200
      Re: [PATCH RFC 3/5] Add KSZ8795 switch driver Andrew Lunn <andrew@lunn.ch> - 2017-09-08 20:40 +0200
        RE: [PATCH RFC 3/5] Add KSZ8795 switch driver <Woojung.Huh@microchip.com> - 2017-09-08 20:40 +0200
      Re: [PATCH RFC 3/5] Add KSZ8795 switch driver Pavel Machek <pavel@ucw.cz> - 2017-09-09 00:00 +0200
        RE: [PATCH RFC 3/5] Add KSZ8795 switch driver <Tristram.Ha@microchip.com> - 2017-09-09 03:50 +0200
          Re: [PATCH RFC 3/5] Add KSZ8795 switch driver Andrew Lunn <andrew@lunn.ch> - 2017-09-09 17:50 +0200

#1728714 — Re: [PATCH RFC 3/5] Add KSZ8795 switch driver

FromPavel Machek <pavel@ucw.cz>
Date2017-09-08 11:20 +0200
SubjectRe: [PATCH RFC 3/5] Add KSZ8795 switch driver
Message-ID<unnCa-4LB-13@gated-at.bofh.it>

[Multipart message — attachments visible in raw view] — view raw

On Thu 2017-09-07 21:17:16, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Add KSZ8795 switch support with function code.

English? "Add KSZ8795 switch support." ?

> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> ---
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> new file mode 100644
> index 0000000..e4d4e6a
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -0,0 +1,2066 @@
> +/*
> + * Microchip KSZ8795 switch driver
> + *
> + * Copyright (C) 2017 Microchip Technology Inc.
> + *	Tristram Ha <Tristram.Ha@microchip.com>
> + *
> + * Permission to use, copy, modify, and/or distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */

This is not exactly GPL, right? But tagging below says it is
GPL. Please fix one.

> +/**
> + * Some counters do not need to be read too often because they are less likely
> + * to increase much.
> + */

Kerneldoc markup but this does not look like kerneldoc. Use plain /* instead?

> +static void ksz_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set)
> +{
> +	u8 data;
> +
> +	ksz_read8(dev, addr, &data);
> +	if (set)
> +		data |= bits;
> +	else
> +		data &= ~bits;
> +	ksz_write8(dev, addr, data);
> +}
> +
> +static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits,
> +			 bool set)
> +{
> +	u32 addr;
> +	u8 data;
> +
> +	addr = PORT_CTRL_ADDR(port, offset);
> +	ksz_read8(dev, addr, &data);
> +
> +	if (set)
> +		data |= bits;
> +	else
> +		data &= ~bits;
> +
> +	ksz_write8(dev, addr, data);
> +}

Other drivers will need this code, right? Does it make sense to put it
into header?

> +static int chk_last_port(struct ksz_device *dev, int p)
> +{
> +	if (dev->last_port && p == dev->last_port)
> +		p = dev->cpu_port;
> +	return p;
> +}

We often prefix even static functions with common prefix. Up to you, I
guess.

> +static int ksz_reset_switch(struct ksz_device *dev)
> +{
> +	/* reset switch */
> +	ksz_write8(dev, REG_POWER_MANAGEMENT_1,
> +		   SW_SOFTWARE_POWER_DOWN << SW_POWER_MANAGEMENT_MODE_S);
> +	ksz_write8(dev, REG_POWER_MANAGEMENT_1, 0);
> +
> +	return 0;
> +}

This is going to be same in other drivers, right?

> +
> +	for (timeout = 1; timeout > 0; timeout--) {
> +		ksz_read8(dev, REG_IND_MIB_CHECK, &check);
> +
> +		if (check & MIB_COUNTER_VALID) {
> +			ksz_read32(dev, REG_IND_DATA_LO, &data);
> +			if (addr < 2) {
> +				u64 total;
> +
> +				total = check & MIB_TOTAL_BYTES_H;
> +				total <<= 32;
> +				*cnt += total;
> +				*cnt += data;
> +				if (check & MIB_COUNTER_OVERFLOW) {
> +					total = MIB_TOTAL_BYTES_H + 1;
> +					total <<= 32;
> +					*cnt += total;
> +				}
> +			} else {
> +				if (check & MIB_COUNTER_OVERFLOW)
> +					*cnt += MIB_PACKET_DROPPED + 1;
> +				*cnt += data & MIB_PACKET_DROPPED;
> +			}
> +			break;
> +		}
> +	}

Why do you need a loop here? This is quite strange code. (And you have
similar strangeness elsewhere. Please fix.)

> +static int valid_dyn_entry(struct ksz_device *dev, u8 *data)
> +{
> +	int timeout = 100;
> +
> +	do {
> +		ksz_read8(dev, REG_IND_DATA_CHECK, data);
> +		timeout--;
> +	} while ((*data & DYNAMIC_MAC_TABLE_NOT_READY) && timeout);
> +
> +	/* Entry is not ready for accessing. */
> +	if (*data & DYNAMIC_MAC_TABLE_NOT_READY) {
> +		return 1;
> +	/* Entry is ready for accessing. */
> +	} else {
> +		ksz_read8(dev, REG_IND_DATA_8, data);
> +
> +		/* There is no valid entry in the table. */
> +		if (*data & DYNAMIC_MAC_TABLE_MAC_EMPTY)
> +			return 2;
> +	}
> +	return 0;
> +}

Normal calling convention is 0 / -ERROR, not 0,1,2.

> +static void ksz_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
> +{
> +	struct ksz_port *port;
> +	u8 ctrl;
> +	u8 restart;
> +	u8 link;
> +	u8 speed;
> +	u8 force;
> +	u8 p = phy;
> +	u16 data = 0;
> +	int processed = true;
> +
> +	port = &dev->ports[p];
> +	switch (reg) {
> +	case PHY_REG_CTRL:
> +		ksz_pread8(dev, p, P_LOCAL_CTRL, &ctrl);
> +		ksz_pread8(dev, p, P_NEG_RESTART_CTRL, &restart);
> +		ksz_pread8(dev, p, P_SPEED_STATUS, &speed);
> +		ksz_pread8(dev, p, P_FORCE_CTRL, &force);
> +		if (restart & PORT_PHY_LOOPBACK)
> +			data |= PHY_LOOPBACK;
> +		if (force & PORT_FORCE_100_MBIT)
> +			data |= PHY_SPEED_100MBIT;
> +		if (!(force & PORT_AUTO_NEG_DISABLE))
> +			data |= PHY_AUTO_NEG_ENABLE;
> +		if (restart & PORT_POWER_DOWN)
> +			data |= PHY_POWER_DOWN;
> +		if (restart & PORT_AUTO_NEG_RESTART)
> +			data |= PHY_AUTO_NEG_RESTART;
> +		if (force & PORT_FORCE_FULL_DUPLEX)
> +			data |= PHY_FULL_DUPLEX;
> +		if (speed & PORT_HP_MDIX)
> +			data |= PHY_HP_MDIX;
> +		if (restart & PORT_FORCE_MDIX)
> +			data |= PHY_FORCE_MDIX;
> +		if (restart & PORT_AUTO_MDIX_DISABLE)
> +			data |= PHY_AUTO_MDIX_DISABLE;
> +		if (restart & PORT_TX_DISABLE)
> +			data |= PHY_TRANSMIT_DISABLE;
> +		if (restart & PORT_LED_OFF)
> +			data |= PHY_LED_DISABLE;
> +		break;
> +	case PHY_REG_STATUS:
> +		ksz_pread8(dev, p, P_LINK_STATUS, &link);
> +		ksz_pread8(dev, p, P_SPEED_STATUS, &speed);
> +		data = PHY_100BTX_FD_CAPABLE |
> +			PHY_100BTX_CAPABLE |
> +			PHY_10BT_FD_CAPABLE |
> +			PHY_10BT_CAPABLE |
> +			PHY_AUTO_NEG_CAPABLE;
> +		if (link & PORT_AUTO_NEG_COMPLETE)
> +			data |= PHY_AUTO_NEG_ACKNOWLEDGE;
> +		if (link & PORT_STAT_LINK_GOOD) {
> +			data |= PHY_LINK_STATUS;
> +			if (!port->link_up) {
> +				port->link_up = 1;
> +				dev->live_ports |= (1 << phy) & dev->on_ports;
> +			}
> +		} else if (port->link_up) {
> +			port->link_down = 1;
> +			port->link_up = 0;
> +			dev->live_ports &= ~(1 << phy);
> +		}
> +		break;
> +	case PHY_REG_ID_1:
> +		data = KSZ8795_ID_HI;
> +		break;
> +	case PHY_REG_ID_2:
> +		data = KSZ8795_ID_LO;
> +		break;
> +	case PHY_REG_AUTO_NEGOTIATION:
> +		ksz_pread8(dev, p, P_LOCAL_CTRL, &ctrl);
> +		data = PHY_AUTO_NEG_802_3;
> +		if (ctrl & PORT_AUTO_NEG_SYM_PAUSE)
> +			data |= PHY_AUTO_NEG_SYM_PAUSE;
> +		if (ctrl & PORT_AUTO_NEG_100BTX_FD)
> +			data |= PHY_AUTO_NEG_100BTX_FD;
> +		if (ctrl & PORT_AUTO_NEG_100BTX)
> +			data |= PHY_AUTO_NEG_100BTX;
> +		if (ctrl & PORT_AUTO_NEG_10BT_FD)
> +			data |= PHY_AUTO_NEG_10BT_FD;
> +		if (ctrl & PORT_AUTO_NEG_10BT)
> +			data |= PHY_AUTO_NEG_10BT;
> +		break;
> +	case PHY_REG_REMOTE_CAPABILITY:
> +		ksz_pread8(dev, p, P_REMOTE_STATUS, &link);
> +		data = PHY_AUTO_NEG_802_3;
> +		if (link & PORT_REMOTE_SYM_PAUSE)
> +			data |= PHY_AUTO_NEG_SYM_PAUSE;
> +		if (link & PORT_REMOTE_100BTX_FD)
> +			data |= PHY_AUTO_NEG_100BTX_FD;
> +		if (link & PORT_REMOTE_100BTX)
> +			data |= PHY_AUTO_NEG_100BTX;
> +		if (link & PORT_REMOTE_10BT_FD)
> +			data |= PHY_AUTO_NEG_10BT_FD;
> +		if (link & PORT_REMOTE_10BT)
> +			data |= PHY_AUTO_NEG_10BT;
> +		break;
> +	default:
> +		processed = false;
> +		break;
> +	}
> +	if (processed)
> +		*val = data;
> +}

Similar code will be needed by other drivers, right?

> +static int ksz_port_bridge_join(struct dsa_switch *ds, int port,
> +				struct net_device *br)
> +{
> +	struct ksz_device *dev = ds->priv;
> +
> +	dev->br_member |= (1 << port);
> +
> +	/**
> +	 * port_stp_state_set() will be called after to put the port in
> +	 * appropriate state so there is no need to do anything.
> +	 */

/** -> /*. Fix it elsewhere, too. This is not kerneldoc.

> +	/* Topology is changed. */
> +	if (forward != dev->member)

has changed?

Thanks for the patches,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [next] | [standalone]

#1729141

From<Tristram.Ha@microchip.com>
Date2017-09-08 20:00 +0200
Message-ID<unvJo-1M9-17@gated-at.bofh.it>
In reply to#1728714
> -----Original Message-----
> From: Pavel Machek [mailto:pavel@ucw.cz]
> Sent: Friday, September 08, 2017 2:19 AM
> To: Tristram Ha - C24268
> Cc: andrew@lunn.ch; muvarov@gmail.com; nathan.leigh.conrad@gmail.com;
> vivien.didelot@savoirfairelinux.com; f.fainelli@gmail.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Woojung Huh - C21699
> Subject: Re: [PATCH RFC 3/5] Add KSZ8795 switch driver
> 
> On Thu 2017-09-07 21:17:16, Tristram.Ha@microchip.com wrote:
> > From: Tristram Ha <Tristram.Ha@microchip.com>
> >
> > Add KSZ8795 switch support with function code.
> 
> English? "Add KSZ8795 switch support." ?
> 
> > Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> > ---
> > diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> > new file mode 100644
> > index 0000000..e4d4e6a
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/ksz8795.c
> > @@ -0,0 +1,2066 @@
> > +/*
> > + * Microchip KSZ8795 switch driver
> > + *
> > + * Copyright (C) 2017 Microchip Technology Inc.
> > + *	Tristram Ha <Tristram.Ha@microchip.com>
> > + *
> > + * Permission to use, copy, modify, and/or distribute this software for any
> > + * purpose with or without fee is hereby granted, provided that the above
> > + * copyright notice and this permission notice appear in all copies.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL
> WARRANTIES
> > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES
> OF
> > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE
> LIABLE FOR
> > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY
> DAMAGES
> > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER
> IN AN
> > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
> ARISING OUT OF
> > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > + */
> 
> This is not exactly GPL, right? But tagging below says it is
> GPL. Please fix one.
> 

This boilerplate paragraph was copied from the KSZ9477 driver, although I did
wonder why this was used.

> > +static int ksz_reset_switch(struct ksz_device *dev)
> > +{
> > +	/* reset switch */
> > +	ksz_write8(dev, REG_POWER_MANAGEMENT_1,
> > +		   SW_SOFTWARE_POWER_DOWN <<
> SW_POWER_MANAGEMENT_MODE_S);
> > +	ksz_write8(dev, REG_POWER_MANAGEMENT_1, 0);
> > +
> > +	return 0;
> > +}
> 
> This is going to be same in other drivers, right?
> 

The switch reset is different in each chip, although KSZ8795 and
KSZ8895 use the same mechanism.

> > +
> > +	for (timeout = 1; timeout > 0; timeout--) {
> > +		ksz_read8(dev, REG_IND_MIB_CHECK, &check);
> > +
> > +		if (check & MIB_COUNTER_VALID) {
> > +			ksz_read32(dev, REG_IND_DATA_LO, &data);
> > +			if (addr < 2) {
> > +				u64 total;
> > +
> > +				total = check & MIB_TOTAL_BYTES_H;
> > +				total <<= 32;
> > +				*cnt += total;
> > +				*cnt += data;
> > +				if (check & MIB_COUNTER_OVERFLOW) {
> > +					total = MIB_TOTAL_BYTES_H + 1;
> > +					total <<= 32;
> > +					*cnt += total;
> > +				}
> > +			} else {
> > +				if (check & MIB_COUNTER_OVERFLOW)
> > +					*cnt += MIB_PACKET_DROPPED + 1;
> > +				*cnt += data & MIB_PACKET_DROPPED;
> > +			}
> > +			break;
> > +		}
> > +	}
> 
> Why do you need a loop here? This is quite strange code. (And you have
> similar strangeness elsewhere. Please fix.)
> 

The MIB_COUNTER_VALID bit may be invalid on first read, although in slow
SPI speed it never happens.  The timeout value should be increased to 2.

> > +static int valid_dyn_entry(struct ksz_device *dev, u8 *data)
> > +{
> > +	int timeout = 100;
> > +
> > +	do {
> > +		ksz_read8(dev, REG_IND_DATA_CHECK, data);
> > +		timeout--;
> > +	} while ((*data & DYNAMIC_MAC_TABLE_NOT_READY) && timeout);
> > +
> > +	/* Entry is not ready for accessing. */
> > +	if (*data & DYNAMIC_MAC_TABLE_NOT_READY) {
> > +		return 1;
> > +	/* Entry is ready for accessing. */
> > +	} else {
> > +		ksz_read8(dev, REG_IND_DATA_8, data);
> > +
> > +		/* There is no valid entry in the table. */
> > +		if (*data & DYNAMIC_MAC_TABLE_MAC_EMPTY)
> > +			return 2;
> > +	}
> > +	return 0;
> > +}
> 
> Normal calling convention is 0 / -ERROR, not 0,1,2.
>

This is an internal function that is not returning any error.  It just reports
different conditions so the calling function decides what to do.
 
> > +static void ksz_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
> > +{
> > +	struct ksz_port *port;
> > +	u8 ctrl;
> > +	u8 restart;
> > +	u8 link;
> > +	u8 speed;
> > +	u8 force;
> > +	u8 p = phy;
> > +	u16 data = 0;
> > +	int processed = true;
> > +
> > +	port = &dev->ports[p];
> > +	switch (reg) {
> > +	case PHY_REG_CTRL:
> > +		ksz_pread8(dev, p, P_LOCAL_CTRL, &ctrl);
> > +		ksz_pread8(dev, p, P_NEG_RESTART_CTRL, &restart);
> > +		ksz_pread8(dev, p, P_SPEED_STATUS, &speed);
> > +		ksz_pread8(dev, p, P_FORCE_CTRL, &force);
> > +		if (restart & PORT_PHY_LOOPBACK)
> > +			data |= PHY_LOOPBACK;
> > +		if (force & PORT_FORCE_100_MBIT)
> > +			data |= PHY_SPEED_100MBIT;
> > +		if (!(force & PORT_AUTO_NEG_DISABLE))
> > +			data |= PHY_AUTO_NEG_ENABLE;
> > +		if (restart & PORT_POWER_DOWN)
> > +			data |= PHY_POWER_DOWN;
> > +		if (restart & PORT_AUTO_NEG_RESTART)
> > +			data |= PHY_AUTO_NEG_RESTART;
> > +		if (force & PORT_FORCE_FULL_DUPLEX)
> > +			data |= PHY_FULL_DUPLEX;
> > +		if (speed & PORT_HP_MDIX)
> > +			data |= PHY_HP_MDIX;
> > +		if (restart & PORT_FORCE_MDIX)
> > +			data |= PHY_FORCE_MDIX;
> > +		if (restart & PORT_AUTO_MDIX_DISABLE)
> > +			data |= PHY_AUTO_MDIX_DISABLE;
> > +		if (restart & PORT_TX_DISABLE)
> > +			data |= PHY_TRANSMIT_DISABLE;
> > +		if (restart & PORT_LED_OFF)
> > +			data |= PHY_LED_DISABLE;
> > +		break;
> > +	case PHY_REG_STATUS:
> > +		ksz_pread8(dev, p, P_LINK_STATUS, &link);
> > +		ksz_pread8(dev, p, P_SPEED_STATUS, &speed);
> > +		data = PHY_100BTX_FD_CAPABLE |
> > +			PHY_100BTX_CAPABLE |
> > +			PHY_10BT_FD_CAPABLE |
> > +			PHY_10BT_CAPABLE |
> > +			PHY_AUTO_NEG_CAPABLE;
> > +		if (link & PORT_AUTO_NEG_COMPLETE)
> > +			data |= PHY_AUTO_NEG_ACKNOWLEDGE;
> > +		if (link & PORT_STAT_LINK_GOOD) {
> > +			data |= PHY_LINK_STATUS;
> > +			if (!port->link_up) {
> > +				port->link_up = 1;
> > +				dev->live_ports |= (1 << phy) & dev->on_ports;
> > +			}
> > +		} else if (port->link_up) {
> > +			port->link_down = 1;
> > +			port->link_up = 0;
> > +			dev->live_ports &= ~(1 << phy);
> > +		}
> > +		break;
> > +	case PHY_REG_ID_1:
> > +		data = KSZ8795_ID_HI;
> > +		break;
> > +	case PHY_REG_ID_2:
> > +		data = KSZ8795_ID_LO;
> > +		break;
> > +	case PHY_REG_AUTO_NEGOTIATION:
> > +		ksz_pread8(dev, p, P_LOCAL_CTRL, &ctrl);
> > +		data = PHY_AUTO_NEG_802_3;
> > +		if (ctrl & PORT_AUTO_NEG_SYM_PAUSE)
> > +			data |= PHY_AUTO_NEG_SYM_PAUSE;
> > +		if (ctrl & PORT_AUTO_NEG_100BTX_FD)
> > +			data |= PHY_AUTO_NEG_100BTX_FD;
> > +		if (ctrl & PORT_AUTO_NEG_100BTX)
> > +			data |= PHY_AUTO_NEG_100BTX;
> > +		if (ctrl & PORT_AUTO_NEG_10BT_FD)
> > +			data |= PHY_AUTO_NEG_10BT_FD;
> > +		if (ctrl & PORT_AUTO_NEG_10BT)
> > +			data |= PHY_AUTO_NEG_10BT;
> > +		break;
> > +	case PHY_REG_REMOTE_CAPABILITY:
> > +		ksz_pread8(dev, p, P_REMOTE_STATUS, &link);
> > +		data = PHY_AUTO_NEG_802_3;
> > +		if (link & PORT_REMOTE_SYM_PAUSE)
> > +			data |= PHY_AUTO_NEG_SYM_PAUSE;
> > +		if (link & PORT_REMOTE_100BTX_FD)
> > +			data |= PHY_AUTO_NEG_100BTX_FD;
> > +		if (link & PORT_REMOTE_100BTX)
> > +			data |= PHY_AUTO_NEG_100BTX;
> > +		if (link & PORT_REMOTE_10BT_FD)
> > +			data |= PHY_AUTO_NEG_10BT_FD;
> > +		if (link & PORT_REMOTE_10BT)
> > +			data |= PHY_AUTO_NEG_10BT;
> > +		break;
> > +	default:
> > +		processed = false;
> > +		break;
> > +	}
> > +	if (processed)
> > +		*val = data;
> > +}
> 
> Similar code will be needed by other drivers, right?
> 

Although KSZ8795 and KSZ8895 may use the same code, the other
chips will have different code.

[toc] | [prev] | [next] | [standalone]

#1729185

FromAndrew Lunn <andrew@lunn.ch>
Date2017-09-08 20:40 +0200
Message-ID<unwm6-2eW-5@gated-at.bofh.it>
In reply to#1729141
> > > @@ -0,0 +1,2066 @@
> > > +/*
> > > + * Microchip KSZ8795 switch driver
> > > + *
> > > + * Copyright (C) 2017 Microchip Technology Inc.
> > > + *	Tristram Ha <Tristram.Ha@microchip.com>
> > > + *
> > > + * Permission to use, copy, modify, and/or distribute this software for any
> > > + * purpose with or without fee is hereby granted, provided that the above
> > > + * copyright notice and this permission notice appear in all copies.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL
> > WARRANTIES
> > > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES
> > OF
> > > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE
> > LIABLE FOR
> > > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY
> > DAMAGES
> > > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER
> > IN AN
> > > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
> > ARISING OUT OF
> > > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > > + */
> > 
> > This is not exactly GPL, right? But tagging below says it is
> > GPL. Please fix one.
> > 
> 
> This boilerplate paragraph was copied from the KSZ9477 driver, although I did
> wonder why this was used.

Hi Tristram

Please can you talk to your legal people and see if this can be
replaced with the standard GPL text?

> > > +	for (timeout = 1; timeout > 0; timeout--) {
> > > +		ksz_read8(dev, REG_IND_MIB_CHECK, &check);
> > > +
> > > +		if (check & MIB_COUNTER_VALID) {
> > > +			ksz_read32(dev, REG_IND_DATA_LO, &data);
> > > +			if (addr < 2) {
> > > +				u64 total;
> > > +
> > > +				total = check & MIB_TOTAL_BYTES_H;
> > > +				total <<= 32;
> > > +				*cnt += total;
> > > +				*cnt += data;
> > > +				if (check & MIB_COUNTER_OVERFLOW) {
> > > +					total = MIB_TOTAL_BYTES_H + 1;
> > > +					total <<= 32;
> > > +					*cnt += total;
> > > +				}
> > > +			} else {
> > > +				if (check & MIB_COUNTER_OVERFLOW)
> > > +					*cnt += MIB_PACKET_DROPPED + 1;
> > > +				*cnt += data & MIB_PACKET_DROPPED;
> > > +			}
> > > +			break;
> > > +		}
> > > +	}
> > 
> > Why do you need a loop here? This is quite strange code. (And you have
> > similar strangeness elsewhere. Please fix.)
> > 
> 
> The MIB_COUNTER_VALID bit may be invalid on first read, although in slow
> SPI speed it never happens.  The timeout value should be increased to 2.

Maybe timeout is the wrong name? There is nothing to do with time
here.
 
> > > +static int valid_dyn_entry(struct ksz_device *dev, u8 *data)
> > > +{
> > > +	int timeout = 100;
> > > +
> > > +	do {
> > > +		ksz_read8(dev, REG_IND_DATA_CHECK, data);
> > > +		timeout--;
> > > +	} while ((*data & DYNAMIC_MAC_TABLE_NOT_READY) && timeout);
> > > +
> > > +	/* Entry is not ready for accessing. */
> > > +	if (*data & DYNAMIC_MAC_TABLE_NOT_READY) {
> > > +		return 1;
> > > +	/* Entry is ready for accessing. */
> > > +	} else {
> > > +		ksz_read8(dev, REG_IND_DATA_8, data);
> > > +
> > > +		/* There is no valid entry in the table. */
> > > +		if (*data & DYNAMIC_MAC_TABLE_MAC_EMPTY)
> > > +			return 2;
> > > +	}
> > > +	return 0;
> > > +}
> > 
> > Normal calling convention is 0 / -ERROR, not 0,1,2.
> >
> 
> This is an internal function that is not returning any error.  It just reports
> different conditions so the calling function decides what to do.

Still, best practice is to use standard error codes.
  
    Andrew

[toc] | [prev] | [next] | [standalone]

#1729190

From<Woojung.Huh@microchip.com>
Date2017-09-08 20:40 +0200
Message-ID<unwm7-2eW-47@gated-at.bofh.it>
In reply to#1729185
> > > > @@ -0,0 +1,2066 @@
> > > > +/*
> > > > + * Microchip KSZ8795 switch driver
> > > > + *
> > > > + * Copyright (C) 2017 Microchip Technology Inc.
> > > > + *	Tristram Ha <Tristram.Ha@microchip.com>
> > > > + *
> > > > + * Permission to use, copy, modify, and/or distribute this software for
> any
> > > > + * purpose with or without fee is hereby granted, provided that the
> above
> > > > + * copyright notice and this permission notice appear in all copies.
> > > > + *
> > > > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS
> ALL
> > > WARRANTIES
> > > > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED
> WARRANTIES
> > > OF
> > > > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR
> BE
> > > LIABLE FOR
> > > > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR
> ANY
> > > DAMAGES
> > > > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS,
> WHETHER
> > > IN AN
> > > > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
> > > ARISING OUT OF
> > > > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS
> SOFTWARE.
> > > > + */
> > >
> > > This is not exactly GPL, right? But tagging below says it is
> > > GPL. Please fix one.
> > >
> >
> > This boilerplate paragraph was copied from the KSZ9477 driver, although I
> did
> > wonder why this was used.
> 
> Hi Tristram
> 
> Please can you talk to your legal people and see if this can be
> replaced with the standard GPL text?
This should be replaced to GPL. These text copied from drivers/net/dsa/b53/*.
Will submit patches of drivers/net/dsa/microchip/*

- Woojung

[toc] | [prev] | [next] | [standalone]

#1729317

FromPavel Machek <pavel@ucw.cz>
Date2017-09-09 00:00 +0200
Message-ID<unztE-4m3-19@gated-at.bofh.it>
In reply to#1729141

[Multipart message — attachments visible in raw view] — view raw

Hi!

> > > +	default:
> > > +		processed = false;
> > > +		break;
> > > +	}
> > > +	if (processed)
> > > +		*val = data;
> > > +}
> > 
> > Similar code will be needed by other drivers, right?
> 
> Although KSZ8795 and KSZ8895 may use the same code, the other
> chips will have different code.

Ok, please make sure code is shared between these two.

Thansk,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1729368

From<Tristram.Ha@microchip.com>
Date2017-09-09 03:50 +0200
Message-ID<unD4d-6Yq-1@gated-at.bofh.it>
In reply to#1729317
> -----Original Message-----
> From: Pavel Machek [mailto:pavel@ucw.cz]
> Sent: Friday, September 08, 2017 2:58 PM
> To: Tristram Ha - C24268
> Cc: andrew@lunn.ch; muvarov@gmail.com; nathan.leigh.conrad@gmail.com;
> vivien.didelot@savoirfairelinux.com; f.fainelli@gmail.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Woojung Huh -
> C21699
> Subject: Re: [PATCH RFC 3/5] Add KSZ8795 switch driver
> 
> Hi!
> 
> > > > +	default:
> > > > +		processed = false;
> > > > +		break;
> > > > +	}
> > > > +	if (processed)
> > > > +		*val = data;
> > > > +}
> > >
> > > Similar code will be needed by other drivers, right?
> >
> > Although KSZ8795 and KSZ8895 may use the same code, the other
> > chips will have different code.
> 
> Ok, please make sure code is shared between these two.

The exact function probably cannot be shared between KSZ8795
and KSZ8895 drivers because although the register constants look
the same but their exact locations are different in the 2 chips.

[toc] | [prev] | [next] | [standalone]

#1729475

FromAndrew Lunn <andrew@lunn.ch>
Date2017-09-09 17:50 +0200
Message-ID<unQb8-7lh-7@gated-at.bofh.it>
In reply to#1729368
On Sat, Sep 09, 2017 at 01:44:37AM +0000, Tristram.Ha@microchip.com wrote:
> > -----Original Message-----
> > From: Pavel Machek [mailto:pavel@ucw.cz]
> > Sent: Friday, September 08, 2017 2:58 PM
> > To: Tristram Ha - C24268
> > Cc: andrew@lunn.ch; muvarov@gmail.com; nathan.leigh.conrad@gmail.com;
> > vivien.didelot@savoirfairelinux.com; f.fainelli@gmail.com;
> > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Woojung Huh -
> > C21699
> > Subject: Re: [PATCH RFC 3/5] Add KSZ8795 switch driver
> > 
> > Hi!
> > 
> > > > > +	default:
> > > > > +		processed = false;
> > > > > +		break;
> > > > > +	}
> > > > > +	if (processed)
> > > > > +		*val = data;
> > > > > +}
> > > >
> > > > Similar code will be needed by other drivers, right?
> > >
> > > Although KSZ8795 and KSZ8895 may use the same code, the other
> > > chips will have different code.
> > 
> > Ok, please make sure code is shared between these two.
> 
> The exact function probably cannot be shared between KSZ8795
> and KSZ8895 drivers because although the register constants look
> the same but their exact locations are different in the 2 chips.

You need to be careful with such functions. If they look identical,
somebody will try to consolidate them into one. So either you need to
do the consolidation yourself in order to get it right, or you need to
add a comment about how it differs.

    Andrew

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web