> > > @@ -0,0 +1,2066 @@
> > > +/*
> > > + * Microchip KSZ8795 switch driver
> > > + *
> > > + * Copyright (C) 2017 Microchip Technology Inc.
> > > + * Tristram Ha <tristram...@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
