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:

quoted

On Wed, Sep 05, 2018 at 04:39:46PM +0300, Laurent Pinchart wrote:

quoted

On Wednesday, 5 September 2018 12:16:33 EEST Maxime Ripard wrote:

quoted

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/

quoted

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 <redacted>
---

 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.

quoted

quoted

quoted

@@ -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.

quoted

quoted

quoted

+	/**
+	 * @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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180907/53c56627/attachment-0001.sig>