Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB

[Uwe Kleine-König]

Hello Sebastian,

On Mon, Mar 21, 2016 at 01:48:45PM +0100, Sebastian Frias wrote:
> On 03/18/2016 08:12 PM, Uwe Kleine-König wrote:
> > On Fri, Mar 18, 2016 at 04:56:21PM +0100, Sebastian Frias wrote:
> >> On 03/18/2016 01:54 PM, Uwe Kleine-König wrote:
> >>> From a driver perspecitive, it would be nice if devm_gpiod_get_optional
> >>> returned NULL iff the respective gpio isn't specified even with
> >>> GPIOLIB=n, but this isn't sensible either because it would result in
> >>> quite some gpiolib code to not being conditionally compiled on
> >>> CONFIG_GPIOLIB any more.
> >>
> >> Let's say that was the case, what would the PHY code do?
> > 
> > With reset gpios it might not be that critical, but consider an optional
> > enable gpio. (Optional in the sense, that some device have it and others
> > don't, e.g. because the pin is pulled into active level by hardware.)
> > 
> > Now you do:
> > 
> > 	gpiod = gpiod_get_optional("enable");
> > 
> > and if gpiod now is an error pointer, you must assume that you cannot
> > operate the device. And even with GPIOLIB=n (and gpiod = ERR_PTR(-ENOSYS))
> > you cannot ignore the error. 
> 
> Two things:
> - I suppose that in such hypothetical case the dependency on GPIOLIB
>   would be required and thus be there

I don't agree. There are bugs out there, and maybe the reason is as
simple as "the implementor of the reset-gpio handling had GPIOLIB on and
didn't test without GPIOLIB".

> - We'd also need to check that 'gpiod' is not NULL

That is the optional part. If gpiod is NULL, you have one of the devices
that don't need to handle the enable line.

> In the scenario at hand only some devices require a hack and
> gpiod_reset=NULL is valid (ie: device will not fail probing)

With your patch and GPIOLIB=n you bind the driver even for the devices
that need the hack. This is broken!

> >For consistency I'd recommend to do the
> > same for reset even though there is a chance to get a working device.
> 
> I'm not so sure, because then information would be lost, handling both
> ("enable" and "reset") the same way aliases different intents and
> requirements.
> Indeed, only "enable" would be really mandatory, "reset" is essentially
> optional (only 1 of 3 devices of the family require the hack).
> 
> Besides, if "reset" was really mandatory, we would also fail the probe
> if the pointer for the reset GPIO is NULL.

No, if reset was mandatory you'd use gpiod_get instead of
gpiod_get_optional and fail iff that call fails, too.

> Indeed, even with GPIOLIB=y the pointer can be NULL if the "reset" (or
> "enable" in your scenario) is not present.
> From a functionality perspective, a NULL pointer for "reset" will result
> in the same behaviour as GPIOLIB=n, ie: not being able to reset the PHY.

Right, but you only get reset=NULL iff the device tree (or whatever is
the source of information for gpiod_get) doesn't specify a reset gpio
which then means "This device doesn't need a reset gpio.". This is
different from the GPIOLIB=n case, where the return code signals "It's
unknown if the device needs a reset gpio.".

> So, the current code (your commit) and previous code (Daniel's commit)
> clearly points to "reset" being optional.
> 
> Furthermore, I think we should not even register the
> "link_change_notify" callback when dealing with AT803x PHYs that do not
> require the hack.
> Another solution (considering the callback is statically registered in
> the "phy_driver" structs for all AT803x PHYs) would be for the callback
> to disable itself.
> Once it detects that the PHY does not require the hack, it could just
> perform:
> 
>     phydev->drv->link_change_notify = NULL;
> 
> or call a new generic function to do the same if encapsulation is required.
> 
> If everybody agrees I can make such change as well, but I really think
> Daniel should give his opinion first.
> 
> > 
> >> What would you think of making at803x_link_change_notify() print a
> >> message every time it should do a reset but does not has a way to do it?
> > 
> > Then this question is obsolete because the device doesn't probe.
> 
> I think you assume that "reset" is mandatory for all AT803x devices, but
> that's not what the code says.

No, you're wrong here. I'm aware that the code supports devices without
reset.

> As such, my proposal was to:
> - keep my proposed patch

I don't agree.

> - make another patch to print a warning when gpiod_reset is NULL (which
>   can happen, even without my patch)

This only happens if no reset gpio is needed and in this case the driver
does the right thing. So if you ask me, there is no need to modify the
driver. Better add a dependency to GPIOLIB. This is necessary even for
devices which don't need a reset gpio to answer the question "does this
driver need a reset gpio?" correctly.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |