mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Sascha Hauer <sha@pengutronix.de>
To: Trent Piepho <trent.piepho@igorinstitute.com>
Cc: "Daniel Brát" <danek.brat@gmail.com>, barebox@lists.infradead.org
Subject: Re: [PATCH] phy: core: Make 'phy_optional_get' return NULL when not implemented
Date: Thu, 4 Nov 2021 21:02:22 +0100	[thread overview]
Message-ID: <20211104200222.GE25698@pengutronix.de> (raw)
In-Reply-To: <CAMHeXxNbHxrEyzsYaXaaaFx38iKt+LjAkC-EcVUXt96yZ3XBBw@mail.gmail.com>

On Wed, Nov 03, 2021 at 03:35:08PM -0700, Trent Piepho wrote:
> On Tue, Nov 2, 2021 at 12:40 AM Sascha Hauer <sha@pengutronix.de> wrote:
> >
> > On Mon, Nov 01, 2021 at 11:33:07AM -0700, Trent Piepho wrote:
> > >    On Mon, Nov 1, 2021 at 2:01 AM Sascha Hauer <[1]sha@pengutronix.de> wrote:
>  >    >
> > >    > 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?
> >
> > We need to distinguish the cases "There is a phy specified, but the
> > reset defaults are good enough to go without a driver" and "There is a
> > phy specified and we need driver support for it". barebox can't know
> > this.
> 
> Could we say that compiling barebox without CONFIG_GENERIC_PHY means
> the driver is not needed and compiling it with the driver means that
> is it?

Please no. Enabling Kconfig options ideally gives you additional
features, but it should not break anything. Consider the case when you
need to enable CONFIG_GENERIC_PHY for something else like an LVDS phy or
whatever, you don't want to end up with broken USB support then and have
to choose whether USB or LVDS is working.

> 
> One can have the defconfig include drivers that might be needed, but
> at some point an engineer ought to be able to
> 
> > Returning NULL from the phy_optional_get() stub has another problem:
> > Some devices might work with CONFIG_GENERIC_PHY disabled, but stop
> > to work when CONFIG_GENERIC_PHY gets enabled, because we might have
> > no driver for the phy. Having boards that only work with CONFIG_GENERIC_PHY
> > disabled is rather unexpected as well.
> 
> Return NULL if there is no driver.
> 
> > >    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;
> > >    }
> >
> > I am fine with this approach. We could add a check if the phy node has a
> > status = "disabled" property. That would allow a board to explicitly
> > state that we do not need to support a phy. We could then do the
> > right thing no matter if CONFIG_GENERIC_PHY is enabled or not.
> >
> > Thinking about it I would prefer something like barebox,status = "disabled"
> > to prevent the property from leaking into Linux.
> 
> Wouldn't it be easier to just delete the phy-names property to disable the phy?

My point is that you sometimes start Linux with the barebox internal
device tree. We should then pass a device tree Linux can handle
properly. A barebox,status would just be ignored by Linux whereas a
status = "disabled" property or deleted phy-names property would disable
the phy for Linux as well.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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


  reply	other threads:[~2021-11-04 20:03 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-20  8:39 Daniel Brát
2021-11-01  9:00 ` Sascha Hauer
     [not found]   ` <CAMHeXxMJYC2q4Hs7zd9A=5-KsXyFUtTshMVoZZC2nJUah=LtjA@mail.gmail.com>
2021-11-01 19:04     ` Trent Piepho
2021-11-02  7:40     ` Sascha Hauer
2021-11-03 22:35       ` Trent Piepho
2021-11-04 20:02         ` Sascha Hauer [this message]
2021-11-04 21:40           ` Trent Piepho
2021-11-08  9:10             ` Sascha Hauer
2021-11-08 22:01               ` Trent Piepho
2021-11-11 13:09                 ` Sascha Hauer
2022-03-03  9:53 ` Sascha Hauer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211104200222.GE25698@pengutronix.de \
    --to=sha@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --cc=danek.brat@gmail.com \
    --cc=trent.piepho@igorinstitute.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox