Re: [PATCH 5/5] net: phy: dp83867: Use unsigned variables to store unsigned properties

[Trent Piepho]

On Mon, 2019-05-13 at 22:46 +0200, Andrew Lunn wrote:
> > > 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.
> > > 
> > 
> > I lack experience with these TI PHY's. Maybe Andrew or Florian can advise.
> 
> Hi Trent
> 
> I already deleted the patches. For patch 3:
> 
> + 	  if (dp83867->clk_output_sel > DP83867_CLK_O_SEL_REF_CLK &&
> +	         dp83867->clk_output_sel != DP83867_CLK_O_SEL_OFF) {
> +		 	phydev_err(phydev, "ti,clk-output-sel value %u out of range\n",
> +				   dp83867->clk_output_sel);
> +			return -EINVAL;
> +		      }
> 
> This last bit looks odd. If it is not OFF, it is invalid?

The valid values are in the range 0 to DP83867_CLK_O_SEL_REF_CLK and
also DP83867_CLK_O_SEL_OFF.  Thus invalid values are those greater than
DP83867_CLK_O_SEL_REF_CLK which are not DP83867_CLK_O_SEL_OFF.

> 
> Are there any in tree users of DP83867_CLK_O_SEL_REF_CLK? We have to
> be careful changing its meaning. But if nobody is actually using it...

Nope.  I doubt this will affect anyone.  They'd need to strap the phy
to get a different configuration, and the explicitly add a property,
which isn't in the example DTS files, to change the configuration to
something they didn't want, and then depend on a driver bug ignoring
the erroneous setting they added.

> Patch 4:
> 
> This is harder. Ideally we want to fix this. At some point, somebody
> is going to want 'rgmii' to actually mean 'rgmii', because that is
> what their hardware needs.
> 
> Could you add a WARN_ON() for 'rgmii' but the PHY is actually adding a
> delay? And add a comment about setting the correct thing in device
> tree?  Hopefully we will then get patches correcting DT blobs. And if
> we later do need to fix 'rgmii', we will break less board.

Yes I can do this.  Should it warn on any use of "rgmii"?  If so, how
would someone make the warning go away if they actually want rgmii mode
with no delay?

I suspect hsdk.dts is an example of an in-tree broken board that uses
"rgmii" would it should have used "rgmii-id".  Unfortunately, phy dts
nodes don't usually provide a compatible that identifies the phy, using
instead run-time auto-detection based on PHY id registers, so there's
no way to tell from the dts files what boards use this phy unless they
also specify one of the phy specific properties.  Which is how I found
hsdk.dts (and no other boards).