LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Trent Piepho <tpiepho@impinj.com>
To: "hkallweit1@gmail.com" <hkallweit1@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Cc: "f.fainelli@gmail.com" <f.fainelli@gmail.com>,
	"andrew@lunn.ch" <andrew@lunn.ch>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: Re: [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties
Date: Mon, 13 May 2019 19:58:17 +0000	[thread overview]
Message-ID: <1557777496.4229.13.camel@impinj.com> (raw)
In-Reply-To: <3a42c0cc-4a4b-e168-c03e-1cc13bd2f5d4@gmail.com>

On Sat, 2019-05-11 at 14:32 +0200, Heiner Kallweit wrote:
> On 11.05.2019 12:41, Heiner Kallweit wrote:
> > On 10.05.2019 23:46, Trent Piepho wrote:
> > > The variables used to store u32 DT properties were signed
> > > ints.  This
> > > doesn't work properly if the value of the property were to
> > > overflow.
> > > Use unsigned variables so this doesn't happen.
> > > 
> > 
> > In patch 3 you added a check for DT properties being out of range.
> > I think this would be good also for the three properties here.
> > The delay values are only 4 bits wide, so you might also consider
> > to switch to u8 or u16.
> > 
> 
> I briefly looked over the rest of the driver. What is plain wrong
> is to allocate memory for the private data structure in the
> config_init callback. This has to be done in the probe callback.
> An example is marvell_probe(). As you seem to work on this driver,
> can you provide a patch for this?

It only allocates the data once, so it is not a memory leak.  But yes,
totally wrong place to do it.  I can fix this.  It also does not set
the signal line impedance from DT value unless unless also configuring
clock skew, even though they are orthogonal concepts.  And fails to
verify anything read from the DT.

Perhaps you could tell me if the approach I've taken in patch 3, 
"Add ability to disable output clock", and patch 4, "Disable tx/rx
delay when not configured", are considered acceptable?  I can conceive
of arguments for alternate approaches.  I would like to add support for
 these into u-boot too, but typically u-boot follows the kernel DT
bindings, so I want to finalize the kernel DT semantics before sending
patches to u-boot.

> > Please note that net-next is closed currently. Please resubmit the
> > patches once it's open again, and please annotate them properly
> > with net-next.

Sorry, didn't know about this policy.  Been years since I've submitted
net patches.  Is there a description somewhere of how this is done? 
Googling net-next wasn't helpful.  I gather new patches are only
allowed when the kernel merge window is open?  And they can't be queued
on patchwork or a topic branch until this happens?


  reply	other threads:[~2019-05-13 19:58 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-10 21:46 [PATCH 1/5] dt-bindings: phy: dp83867: Describe how driver behaves w.r.t rgmii delay Trent Piepho
2019-05-10 21:46 ` [PATCH 2/5] dt-bindings: phy: dp83867: Add documentation for disabling clock output Trent Piepho
2019-05-10 21:46 ` [PATCH 3/5] net: phy: dp83867: Add ability to disable output clock Trent Piepho
2019-05-10 21:46 ` [PATCH 4/5] net: phy: dp83867: Disable tx/rx delay when not configured Trent Piepho
2019-05-10 21:46 ` [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties Trent Piepho
2019-05-11 10:41   ` Heiner Kallweit
2019-05-11 12:32     ` Heiner Kallweit
2019-05-13 19:58       ` Trent Piepho [this message]
2019-05-13 20:12         ` Heiner Kallweit
2019-05-13 20:46           ` Andrew Lunn
2019-05-13 21:26             ` Trent Piepho
2019-05-13 21:43               ` Andrew Lunn
2019-05-14 15:37                 ` Trent Piepho

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=1557777496.4229.13.camel@impinj.com \
    --to=tpiepho@impinj.com \
    --cc=andrew@lunn.ch \
    --cc=devicetree@vger.kernel.org \
    --cc=f.fainelli@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --subject='Re: [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties' \
    /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

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).