From: Andrew Lunn <andrew@lunn•ch>
To: Tristram.Ha@microchip•com
Cc: pavel@ucw•cz, 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@microchip•com
Subject: Re: [PATCH RFC 3/5] Add KSZ8795 switch driver
Date: Fri, 8 Sep 2017 20:32:27 +0200 [thread overview]
Message-ID: <20170908183227.GJ25219@lunn.ch> (raw)
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD41121E3D@CHN-SV-EXMX02.mchp-main.com>
> > > @@ -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
next prev parent reply other threads:[~2017-09-08 18:32 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-07 21:17 [PATCH RFC 3/5] Add KSZ8795 switch driver Tristram.Ha
2017-09-07 22:36 ` Andrew Lunn
2017-09-18 20:27 ` Tristram.Ha
2017-09-28 18:40 ` Pavel Machek
2017-09-28 18:45 ` Florian Fainelli
2017-09-29 18:56 ` Tristram.Ha
2017-09-28 19:34 ` Andrew Lunn
2017-09-29 9:14 ` David Laight
2017-09-29 12:12 ` Andrew Lunn
2017-09-29 18:24 ` Tristram.Ha
2017-09-29 18:53 ` Andrew Lunn
2017-09-29 19:19 ` Tristram.Ha
2017-09-29 20:39 ` Andrew Lunn
2017-09-08 9:18 ` Pavel Machek
2017-09-08 17:54 ` Tristram.Ha
2017-09-08 18:32 ` Andrew Lunn [this message]
2017-09-08 18:35 ` Woojung.Huh
2017-09-08 21:57 ` Pavel Machek
2017-09-09 1:44 ` Tristram.Ha
2017-09-09 15:45 ` Andrew Lunn
2017-09-28 15:24 ` Pavel Machek
2017-09-29 18:45 ` Tristram.Ha
2017-10-01 7:21 ` Pavel Machek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170908183227.GJ25219@lunn.ch \
--to=andrew@lunn$(echo .)ch \
--cc=Tristram.Ha@microchip$(echo .)com \
--cc=Woojung.Huh@microchip$(echo .)com \
--cc=f.fainelli@gmail$(echo .)com \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=muvarov@gmail$(echo .)com \
--cc=nathan.leigh.conrad@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pavel@ucw$(echo .)cz \
--cc=vivien.didelot@savoirfairelinux$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox