Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]

On Thu, May 15, 2014 at 10:49:37AM +0530, Rahul Sharma wrote:
> Hi Thierry,
> 
> On 15 May 2014 03:44, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Thu, May 15, 2014 at 12:47:21AM +0530, Rahul Sharma wrote:
[...]
> >> +#define HDMI_PHY     0
> >> +#define DAC_PHY      1
> >> +#define ADC_PHY      2
> >> +#define PCIE_PHY     3
> >> +#define SATA_PHY     4
> >
> > Perhaps these should be namespaced somehow to avoid potential conflicts
> > with other PHY providers?
> 
> How about XXX_SIMPLE_PHY?

The indices are specific to the Exynos PHY device, so why not
PHY_EXYNOS_SIMPLE_XXX (or any variation thereof)?

> >> +#define PHY_NR       5
> >
> > I'm not sure that this belongs here either. It's not a value that will
> > ever appear in a DT source file.
> 
> I want it to grow along with new additions in the phy list else
> catastrophic. This will look unrelated in driver.

But this is in no way growing automatically as it is. Whoever adds a new
type of PHY will need to manually increment this define. Furthermore the
driver will need to be updated to cope with this anyway.

I think this is even a reason to have this only in the driver. Otherwise
you could have a situation where somebody upgrades the binding (and this
file along with it) but not update the driver with the necessary support.
But the driver will still pick up the PHY_NR change and will accept the
new PHY type as valid, even though it has no code to handle it. If you
have this in the driver, however, then as long as the driver has not yet
been updated it will reject requests for the new PHY type. And that's
exactly what it should be doing since it doesn't know how to handle it.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]