Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi,

On Thu, Feb 12, 2015 at 05:52:42PM -0800, David Cohen wrote:
> Hi Heikki,
> 
> Sorry I am starting a new branch on this thread.
> I need to go back to another topic on this same patch.
> 
> On Fri, Jan 23, 2015 at 05:12:58PM +0200, Heikki Krogerus wrote:
> > TUSB1210 ULPI PHY has vendor specific register for eye
> > diagram tuning. On some platforms the system firmware has
> > set optimized value to it. In order to not loose the
> > optimized value, the driver stores it during probe and
> > restores it every time the PHY is powered back on.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > ---
> 
> [snip]
> 
> > +	/* Store initial eye diagram optimisation value */
> > +	ret = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
> 
> We can't rely on eye diagram optimization value here. It's possible the
> phy went through reset (e.g. module unloading/loading).
> We need a more consistent method. Could we use _DSD instead? That would
> be more compatible with DT too.

I'm don't have any problem with getting the value from a property.
Sounds like the correct thing to do. Are you thinking about putting
that _DSD method to the ACPI device object for the dwc3? The correct
place would probable be separate device object for the PHY, but if we
do that we need to consider how that object is bound to the ulpi
device. There are few ways we can do that, so it requires some
thinking.

In any case this driver we can already implement so that it expects to
get the value from property. We just need the name for it.

The register has actually separate fields for High speed output
impedance configuration and High speed output driver strength
configuration for eye diagram tuning, plus control bit for DP/DM swap.

Maybe we should actually get them from three separate properties:

        device_property_read_u8(dev, "datapolarity", &val);
        ...
        device_property_read_u8(dev, "zhsdrv", &val);
        ...
        device_property_read_u8(dev, "ihstx", &val);
        ...

What do you think?


Cheers,

-- 
heikki