netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: David Miller <davem@davemloft.net>
To: richardcochran@gmail.com
Cc: afleming@gmail.com, netdev@vger.kernel.org
Subject: Re: [PATCH] phylib: Add support for the LXT973 phy.
Date: Mon, 07 Jun 2010 01:18:35 -0700 (PDT)	[thread overview]
Message-ID: <20100607.011835.55846752.davem@davemloft.net> (raw)
In-Reply-To: <20100605140017.GA2750@riccoc20.at.omicron.at>

From: Richard Cochran <richardcochran@gmail.com>
Date: Sat, 5 Jun 2010 16:00:17 +0200

> On Wed, Jun 02, 2010 at 02:32:11PM -0500, Andy Fleming wrote:
>> Yeah, I was clearly not thinking clearly.  dev_flags will be
>> overwritten, and is not meant for this.  I believe, what we should do
>> is add a "port" field to the PHY device, and if PCR_FIBER_SELECT is
>> set, then set the port field to PORT_FIBRE.  I'm not entirely clear on
>> the semantics of that field in the ethtool cmd, but it seems like the
>> right idea.
> 
> Here is another try. Is that more like it?

I think this is overkill.

One, and only one, PHY driver wants to maintain a boolean state and
we're adding a full 32-bit flags member to the generic PHY device
struct to accomodate this?

Andy if you have ideas to use that state via ethtool or whatever in
the future, you come back to us when the future arrives and you've
implemented the code in the generic PHY lib code to do that stuff.

As it stands right now, that code doesn't exist so accomodating it is
just silly.

I also think worrying about the phy_dev->priv pointer being misused in
the future inside of this driver is a completely bogus argument by any
measure.

We have many cases all over the kernel tree, in drivers and elsewhere,
where we encode integer states in what are normally pointer values
when we need to maintain a small piece of state and don't need to do a
full blown memory allocation to allocate a piece of extra memory to
hold that state.

Richard, please just resubmit your original patch and I will apply it.

Thanks.


  reply	other threads:[~2010-06-07  8:18 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-05-31 13:09 [PATCH] phylib: Add support for the LXT973 phy Richard Cochran
2010-06-01 22:39 ` Andy Fleming
2010-06-02 12:55   ` Richard Cochran
2010-06-02 13:07     ` Richard Cochran
2010-06-02 13:50     ` David Miller
2010-06-02 15:08       ` Richard Cochran
2010-06-02 15:15         ` David Miller
2010-06-03 11:28           ` Richard Cochran
2010-06-02 19:32       ` Andy Fleming
2010-06-05 14:00         ` Richard Cochran
2010-06-07  8:18           ` David Miller [this message]
2010-06-07 15:39             ` Andy Fleming
2010-06-07 15:39             ` Richard Cochran
2010-06-09 23:17               ` David Miller
2010-06-22 12:38 ` [PATCH] phylib: Add autoload " David Woodhouse
2010-06-23  5:37   ` Richard Cochran
2010-06-23  9:00     ` David Woodhouse
2010-06-27  5:16   ` David Miller

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=20100607.011835.55846752.davem@davemloft.net \
    --to=davem@davemloft.net \
    --cc=afleming@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=richardcochran@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).