Re: [PATCH] phy: core: Make 'phy_optional_get' return NULL when not implemented

[Trent Piepho <trent.piepho@igorinstitute.com>]

On Mon, Nov 1, 2021 at 2:01 AM Sascha Hauer <sha@pengutronix.de> wrote:
> On Wed, Oct 20, 2021 at 10:39:54AM +0200, Daniel Brát wrote:
> > Make 'phy_optional_get' return NULL instead of ERR_PTR(-ENOSYS) when the
> > CONFIG_GENERIC_PHY is not enabled. It makes more sense to return NULL instead
> > of straight up throwing a error since the function has 'optional' in its name.
> > This also fixes dwc2 usb driver which would previously fail inside its probe
> > function despite being able to function without a phy just fine.
>
> The phy is only optional as long as none is specified in the device
> tree. When there is one specified then it's no longer optional. We can't
> do the right thing here without checking the device tree. Given that
> it's simple to enable CONFIG_GENERIC_PHY I think this is the way to go.

But this force enables GENERIC_PHY when it's not needed.

There are commonly many device nodes in Linux dts files that are not
used by an enabled Barebox driver.  It's normal to turn off a driver
that might be or could be used.  Is it necessarily an error if a phy
is present in the dts but we don't wish to include support for it?

In this Barebox version of this code, I think phy_optional_get() only
returns NULL if there is some error finding the phy OF node, such as
there being none specified or some error in the validity of the dts
data.  Otherwise it's PROBE_DEFER.

In the Linux version, there are other ways to get NULL, such as the
phy being disabled:
        if (!of_device_is_available(args.np)) {
               dev_warn(phy_provider->dev, "Requested PHY is disabled\n");
               phy = ERR_PTR(-ENODEV);  // phy_optional_get -> NULL

So it seems like the phy being optional, even if defined in this dts,
might be the intended behavior.

The stub version could check if a phy is in the dts and generate a
warning, if the goal is to reduce problems with people creating broken
configurations and then not understanding why their configuration does
not work.

struct phy *phy_optional_get(struct device_d *dev, const char *string)
{
        if (dev->device_node &&
            !of_parse_phandle_with_args(dev->device_node, "phys",
"#phy-cells", of_property_match_string(dev->device_node, "phy-names",
string), NULL))
                dev_warn(dev, "%s phy specified in device tree but
CONFIG_GENERIC_PHY support is not enabled", string);
        return NULL;
}

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox