On Thu, Sep 06, 2018 at 07:51:05PM +0300, Laurent Pinchart wrote:
> On Thursday, 6 September 2018 17:48:07 EEST Maxime Ripard wrote:
> > On Wed, Sep 05, 2018 at 04:39:46PM +0300, Laurent Pinchart wrote:
> > > On Wednesday, 5 September 2018 12:16:33 EEST Maxime Ripard wrote:
> > >> The phy framework is only allowing to configure the power state of the
> > >> PHY using the init and power_on hooks, and their power_off and exit
> > >> counterparts.
> > >>
> > >> While it works for most, simple, PHYs supported so far, some more
> > >> advanced PHYs need some configuration depending on runtime parameters.
> > >> These PHYs have been supported by a number of means already, often by
> > >> using ad-hoc drivers in their consumer drivers.
> > >>
> > >> That doesn't work too well however, when a consumer device needs to deal
> > >
> > > s/deal/deal with/
> > >
> > >> multiple PHYs, or when multiple consumers need to deal with the same PHY
> > >> (a DSI driver and a CSI driver for example).
> > >>
> > >> So we'll add a new interface, through two funtions, phy_validate and
> > >> phy_configure. The first one will allow to check that a current
> > >> configuration, for a given mode, is applicable. It will also allow the
> > >> PHY driver to tune the settings given as parameters as it sees fit.
> > >>
> > >> phy_configure will actually apply that configuration in the phy itself.
> > >>
> > >> Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
> > >> ---
> > >>
> > >> drivers/phy/phy-core.c | 62 +++++++++++++++++++++++++++++++++++++++++-
> > >> include/linux/phy/phy.h | 42 ++++++++++++++++++++++++++++-
> > >> 2 files changed, 104 insertions(+)
> > >>
> > >> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> > >> index 35fd38c5a4a1..6eaf655e370f 100644
> > >> --- a/drivers/phy/phy-core.c
> > >> +++ b/drivers/phy/phy-core.c
> > >> @@ -408,6 +408,68 @@ int phy_calibrate(struct phy *phy)
> > >> EXPORT_SYMBOL_GPL(phy_calibrate);
> > >>
> > >> /**
> > >> + * phy_configure() - Changes the phy parameters
> > >> + * @phy: the phy returned by phy_get()
> > >> + * @mode: phy_mode the configuration is applicable to.
> > >> + * @opts: New configuration to apply
> > >> + *
> > >> + * Used to change the PHY parameters. phy_init() must have
> > >> + * been called on the phy.
> > >> + *
> > >> + * Returns: 0 if successful, an negative error code otherwise
> > >> + */
> > >> +int phy_configure(struct phy *phy, enum phy_mode mode,
> > >> + union phy_configure_opts *opts)
> > >> +{
> > >> + int ret;
> > >> +
> > >> + if (!phy)
> > >> + return -EINVAL;
> > >> +
> > >> + if (!phy->ops->configure)
> > >> + return 0;
> > >
> > > Shouldn't you report an error to the caller ? If a caller expects the PHY
> > > to be configurable, I would assume that silently ignoring the requested
> > > configuration won't work great.
> >
> > I'm not sure. I also expect a device having to interact with multiple
> > PHYs, some of them needing some configuration while some other do
> > not. In that scenario, returning 0 seems to be the right thing to do.
>
> It could be up to the caller to decide whether to ignore the error or not when
> the operation isn't implemented. I expect that a call requiring specific
> configuration parameters for a given PHY might want to bail out if the
> configuration can't be applied. On the other hand that should never happen
> when the system is designed correctly, as vendors are not supposed to ship
> kernels that would be broken by design (as in requiring a configure operation
> but not providing it).
I'll do as Andrew (and you) suggested then.
> > >> @@ -60,6 +66,38 @@ struct phy_ops {
> > >> int (*power_on)(struct phy *phy);
> > >> int (*power_off)(struct phy *phy);
> > >> int (*set_mode)(struct phy *phy, enum phy_mode mode);
> > >> +
> > >> + /**
> > >> + * @configure:
> > >> + *
> > >> + * Optional.
> > >> + *
> > >> + * Used to change the PHY parameters. phy_init() must have
> > >> + * been called on the phy.
> > >> + *
> > >> + * Returns: 0 if successful, an negative error code otherwise
> > >> + */
> > >> + int (*configure)(struct phy *phy, enum phy_mode mode,
> > >> + union phy_configure_opts *opts);
> > >
> > > Is this function allowed to modify opts ? If so, to what extent ? If not,
> > > the pointer should be made const.
> >
> > That's a pretty good question. I guess it could modify it to the same
> > extent than validate could. Would that make sense?
>
> It would, or we could say that PHY users are required to call the validate
> function first, and the the configure function will return an error if the
> passed configuration isn't valid. That would avoid double-validation when the
> PHY user uses .validate().
I usually prefer to have a function being able to check its input on
its own. Especially, the sole use case we have right now is DRM, and
DRM would typically call phy_validate X+1 times (X being the number of
modes), once for each mode in mode_valid and once in atomic_check.
> > >> + /**
> > >> + * @validate:
> > >> + *
> > >> + * Optional.
> > >> + *
> > >> + * Used to check that the current set of parameters can be
> > >> + * handled by the phy. Implementations are free to tune the
> > >> + * parameters passed as arguments if needed by some
> > >> + * implementation detail or constraints. It must not change
> > >> + * any actual configuration of the PHY, so calling it as many
> > >> + * times as deemed fit by the consumer must have no side
> > >> + * effect.
> > >> + *
> > >> + * Returns: 0 if the configuration can be applied, an negative
> > >> + * error code otherwise
> > >
> > > When should this operation modify the passed parameters, and when should
> > > it return an error ? I understand that your goal is to implement a
> > > negotiation mechanism for the PHY parameters, and to be really useful I
> > > think we need to document it more precisely.
> >
> > My initial idea was to reject a configuration that wouldn't be
> > achievable by the PHY, ie you're asking something that is outside of
> > the operating boundaries, while you would be able to change settings
> > that would be operational, but sub-optimal.
>
> I'm fine with that, let's document it explicitly.
ACK.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Attachment:
signature.asc
Description: PGP signature
