From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Wed, 05 Sep 2018 16:39:46 +0300 Subject: [PATCH 02/10] phy: Add configuration interface In-Reply-To: References: Message-ID: <8397722.XVQDA25ZU6@avalon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Maxime, Thank you for the patch. 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 > --- > 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. > + mutex_lock(&phy->mutex); > + ret = phy->ops->configure(phy, mode, opts); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > + > +/** > + * phy_validate() - Checks the phy parameters > + * @phy: the phy returned by phy_get() > + * @mode: phy_mode the configuration is applicable to. > + * @opts: Configuration to check > + * > + * 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 will not change any actual configuration of the > + * PHY, so calling it as many times as deemed fit will have no side > + * effect. > + * > + * Returns: 0 if successful, an negative error code otherwise > + */ > +int phy_validate(struct phy *phy, enum phy_mode mode, > + union phy_configure_opts *opts) > +{ > + int ret; > + > + if (!phy) > + return -EINVAL; > + > + if (!phy->ops->validate) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->validate(phy, mode, opts); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > + > +/** > * _of_phy_get() - lookup and obtain a reference to a phy by phandle > * @np: device_node for which to get the phy > * @index: the index of the phy > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index 9cba7fe16c23..3cc315dcfcd0 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -44,6 +44,12 @@ enum phy_mode { > }; > > /** > + * union phy_configure_opts - Opaque generic phy configuration > + */ > +union phy_configure_opts { > +}; > + > +/** > * struct phy_ops - set of function pointers for performing phy operations > * @init: operation to be performed for initializing phy > * @exit: operation to be performed while exiting > @@ -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. > + /** > + * @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. > + */ > + int (*validate)(struct phy *phy, enum phy_mode mode, > + union phy_configure_opts *opts); > int (*reset)(struct phy *phy); > int (*calibrate)(struct phy *phy); > struct module *owner; > @@ -164,6 +202,10 @@ int phy_exit(struct phy *phy); > int phy_power_on(struct phy *phy); > int phy_power_off(struct phy *phy); > int phy_set_mode(struct phy *phy, enum phy_mode mode); > +int phy_configure(struct phy *phy, enum phy_mode mode, > + union phy_configure_opts *opts); > +int phy_validate(struct phy *phy, enum phy_mode mode, > + union phy_configure_opts *opts); > static inline enum phy_mode phy_get_mode(struct phy *phy) > { > return phy->attrs.mode; -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH 02/10] phy: Add configuration interface Date: Wed, 05 Sep 2018 16:39:46 +0300 Message-ID: <8397722.XVQDA25ZU6@avalon> References: Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by gabe.freedesktop.org (Postfix) with ESMTPS id C62BD89DD3 for ; Wed, 5 Sep 2018 13:39:42 +0000 (UTC) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Maxime Ripard Cc: Krzysztof Witos , Rafal Ciepiela , Boris Brezillon , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Kishon Vijay Abraham I , Chen-Yu Tsai , Thomas Petazzoni , linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org SGkgTWF4aW1lLAoKVGhhbmsgeW91IGZvciB0aGUgcGF0Y2guCgpPbiBXZWRuZXNkYXksIDUgU2Vw dGVtYmVyIDIwMTggMTI6MTY6MzMgRUVTVCBNYXhpbWUgUmlwYXJkIHdyb3RlOgo+IFRoZSBwaHkg ZnJhbWV3b3JrIGlzIG9ubHkgYWxsb3dpbmcgdG8gY29uZmlndXJlIHRoZSBwb3dlciBzdGF0ZSBv ZiB0aGUgUEhZCj4gdXNpbmcgdGhlIGluaXQgYW5kIHBvd2VyX29uIGhvb2tzLCBhbmQgdGhlaXIg cG93ZXJfb2ZmIGFuZCBleGl0Cj4gY291bnRlcnBhcnRzLgo+IAo+IFdoaWxlIGl0IHdvcmtzIGZv ciBtb3N0LCBzaW1wbGUsIFBIWXMgc3VwcG9ydGVkIHNvIGZhciwgc29tZSBtb3JlIGFkdmFuY2Vk Cj4gUEhZcyBuZWVkIHNvbWUgY29uZmlndXJhdGlvbiBkZXBlbmRpbmcgb24gcnVudGltZSBwYXJh bWV0ZXJzLiBUaGVzZSBQSFlzCj4gaGF2ZSBiZWVuIHN1cHBvcnRlZCBieSBhIG51bWJlciBvZiBt ZWFucyBhbHJlYWR5LCBvZnRlbiBieSB1c2luZyBhZC1ob2MKPiBkcml2ZXJzIGluIHRoZWlyIGNv bnN1bWVyIGRyaXZlcnMuCj4gCj4gVGhhdCBkb2Vzbid0IHdvcmsgdG9vIHdlbGwgaG93ZXZlciwg d2hlbiBhIGNvbnN1bWVyIGRldmljZSBuZWVkcyB0byBkZWFsCgpzL2RlYWwvZGVhbCB3aXRoLwoK PiBtdWx0aXBsZSBQSFlzLCBvciB3aGVuIG11bHRpcGxlIGNvbnN1bWVycyBuZWVkIHRvIGRlYWwg d2l0aCB0aGUgc2FtZSBQSFkgKGEKPiBEU0kgZHJpdmVyIGFuZCBhIENTSSBkcml2ZXIgZm9yIGV4 YW1wbGUpLgo+IAo+IFNvIHdlJ2xsIGFkZCBhIG5ldyBpbnRlcmZhY2UsIHRocm91Z2ggdHdvIGZ1 bnRpb25zLCBwaHlfdmFsaWRhdGUgYW5kCj4gcGh5X2NvbmZpZ3VyZS4gVGhlIGZpcnN0IG9uZSB3 aWxsIGFsbG93IHRvIGNoZWNrIHRoYXQgYSBjdXJyZW50Cj4gY29uZmlndXJhdGlvbiwgZm9yIGEg Z2l2ZW4gbW9kZSwgaXMgYXBwbGljYWJsZS4gSXQgd2lsbCBhbHNvIGFsbG93IHRoZSBQSFkKPiBk cml2ZXIgdG8gdHVuZSB0aGUgc2V0dGluZ3MgZ2l2ZW4gYXMgcGFyYW1ldGVycyBhcyBpdCBzZWVz IGZpdC4KPiAKPiBwaHlfY29uZmlndXJlIHdpbGwgYWN0dWFsbHkgYXBwbHkgdGhhdCBjb25maWd1 cmF0aW9uIGluIHRoZSBwaHkgaXRzZWxmLgo+IAo+IFNpZ25lZC1vZmYtYnk6IE1heGltZSBSaXBh cmQgPG1heGltZS5yaXBhcmRAYm9vdGxpbi5jb20+Cj4gLS0tCj4gIGRyaXZlcnMvcGh5L3BoeS1j b3JlLmMgIHwgNjIgKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrLQo+ ICBpbmNsdWRlL2xpbnV4L3BoeS9waHkuaCB8IDQyICsrKysrKysrKysrKysrKysrKysrKysrKysr KystCj4gIDIgZmlsZXMgY2hhbmdlZCwgMTA0IGluc2VydGlvbnMoKykKPiAKPiBkaWZmIC0tZ2l0 IGEvZHJpdmVycy9waHkvcGh5LWNvcmUuYyBiL2RyaXZlcnMvcGh5L3BoeS1jb3JlLmMKPiBpbmRl eCAzNWZkMzhjNWE0YTEuLjZlYWY2NTVlMzcwZiAxMDA2NDQKPiAtLS0gYS9kcml2ZXJzL3BoeS9w aHktY29yZS5jCj4gKysrIGIvZHJpdmVycy9waHkvcGh5LWNvcmUuYwo+IEBAIC00MDgsNiArNDA4 LDY4IEBAIGludCBwaHlfY2FsaWJyYXRlKHN0cnVjdCBwaHkgKnBoeSkKPiAgRVhQT1JUX1NZTUJP TF9HUEwocGh5X2NhbGlicmF0ZSk7Cj4gCj4gIC8qKgo+ICsgKiBwaHlfY29uZmlndXJlKCkgLSBD aGFuZ2VzIHRoZSBwaHkgcGFyYW1ldGVycwo+ICsgKiBAcGh5OiB0aGUgcGh5IHJldHVybmVkIGJ5 IHBoeV9nZXQoKQo+ICsgKiBAbW9kZTogcGh5X21vZGUgdGhlIGNvbmZpZ3VyYXRpb24gaXMgYXBw bGljYWJsZSB0by4KPiArICogQG9wdHM6IE5ldyBjb25maWd1cmF0aW9uIHRvIGFwcGx5Cj4gKyAq Cj4gKyAqIFVzZWQgdG8gY2hhbmdlIHRoZSBQSFkgcGFyYW1ldGVycy4gcGh5X2luaXQoKSBtdXN0 IGhhdmUKPiArICogYmVlbiBjYWxsZWQgb24gdGhlIHBoeS4KPiArICoKPiArICogUmV0dXJuczog MCBpZiBzdWNjZXNzZnVsLCBhbiBuZWdhdGl2ZSBlcnJvciBjb2RlIG90aGVyd2lzZQo+ICsgKi8K PiAraW50IHBoeV9jb25maWd1cmUoc3RydWN0IHBoeSAqcGh5LCBlbnVtIHBoeV9tb2RlIG1vZGUs Cj4gKwkJICB1bmlvbiBwaHlfY29uZmlndXJlX29wdHMgKm9wdHMpCj4gK3sKPiArCWludCByZXQ7 Cj4gKwo+ICsJaWYgKCFwaHkpCj4gKwkJcmV0dXJuIC1FSU5WQUw7Cj4gKwo+ICsJaWYgKCFwaHkt Pm9wcy0+Y29uZmlndXJlKQo+ICsJCXJldHVybiAwOwoKU2hvdWxkbid0IHlvdSByZXBvcnQgYW4g ZXJyb3IgdG8gdGhlIGNhbGxlciA/IElmIGEgY2FsbGVyIGV4cGVjdHMgdGhlIFBIWSB0byAKYmUg Y29uZmlndXJhYmxlLCBJIHdvdWxkIGFzc3VtZSB0aGF0IHNpbGVudGx5IGlnbm9yaW5nIHRoZSBy ZXF1ZXN0ZWQgCmNvbmZpZ3VyYXRpb24gd29uJ3Qgd29yayBncmVhdC4KCj4gKwltdXRleF9sb2Nr KCZwaHktPm11dGV4KTsKPiArCXJldCA9IHBoeS0+b3BzLT5jb25maWd1cmUocGh5LCBtb2RlLCBv cHRzKTsKPiArCW11dGV4X3VubG9jaygmcGh5LT5tdXRleCk7Cj4gKwo+ICsJcmV0dXJuIHJldDsK PiArfQo+ICsKPiArLyoqCj4gKyAqIHBoeV92YWxpZGF0ZSgpIC0gQ2hlY2tzIHRoZSBwaHkgcGFy YW1ldGVycwo+ICsgKiBAcGh5OiB0aGUgcGh5IHJldHVybmVkIGJ5IHBoeV9nZXQoKQo+ICsgKiBA bW9kZTogcGh5X21vZGUgdGhlIGNvbmZpZ3VyYXRpb24gaXMgYXBwbGljYWJsZSB0by4KPiArICog QG9wdHM6IENvbmZpZ3VyYXRpb24gdG8gY2hlY2sKPiArICoKPiArICogVXNlZCB0byBjaGVjayB0 aGF0IHRoZSBjdXJyZW50IHNldCBvZiBwYXJhbWV0ZXJzIGNhbiBiZSBoYW5kbGVkIGJ5Cj4gKyAq IHRoZSBwaHkuIEltcGxlbWVudGF0aW9ucyBhcmUgZnJlZSB0byB0dW5lIHRoZSBwYXJhbWV0ZXJz IHBhc3NlZCBhcwo+ICsgKiBhcmd1bWVudHMgaWYgbmVlZGVkIGJ5IHNvbWUgaW1wbGVtZW50YXRp b24gZGV0YWlsIG9yCj4gKyAqIGNvbnN0cmFpbnRzLiBJdCB3aWxsIG5vdCBjaGFuZ2UgYW55IGFj dHVhbCBjb25maWd1cmF0aW9uIG9mIHRoZQo+ICsgKiBQSFksIHNvIGNhbGxpbmcgaXQgYXMgbWFu eSB0aW1lcyBhcyBkZWVtZWQgZml0IHdpbGwgaGF2ZSBubyBzaWRlCj4gKyAqIGVmZmVjdC4KPiAr ICoKPiArICogUmV0dXJuczogMCBpZiBzdWNjZXNzZnVsLCBhbiBuZWdhdGl2ZSBlcnJvciBjb2Rl IG90aGVyd2lzZQo+ICsgKi8KPiAraW50IHBoeV92YWxpZGF0ZShzdHJ1Y3QgcGh5ICpwaHksIGVu dW0gcGh5X21vZGUgbW9kZSwKPiArCQkgIHVuaW9uIHBoeV9jb25maWd1cmVfb3B0cyAqb3B0cykK PiArewo+ICsJaW50IHJldDsKPiArCj4gKwlpZiAoIXBoeSkKPiArCQlyZXR1cm4gLUVJTlZBTDsK PiArCj4gKwlpZiAoIXBoeS0+b3BzLT52YWxpZGF0ZSkKPiArCQlyZXR1cm4gMDsKPiArCj4gKwlt dXRleF9sb2NrKCZwaHktPm11dGV4KTsKPiArCXJldCA9IHBoeS0+b3BzLT52YWxpZGF0ZShwaHks IG1vZGUsIG9wdHMpOwo+ICsJbXV0ZXhfdW5sb2NrKCZwaHktPm11dGV4KTsKPiArCj4gKwlyZXR1 cm4gcmV0Owo+ICt9Cj4gKwo+ICsvKioKPiAgICogX29mX3BoeV9nZXQoKSAtIGxvb2t1cCBhbmQg b2J0YWluIGEgcmVmZXJlbmNlIHRvIGEgcGh5IGJ5IHBoYW5kbGUKPiAgICogQG5wOiBkZXZpY2Vf bm9kZSBmb3Igd2hpY2ggdG8gZ2V0IHRoZSBwaHkKPiAgICogQGluZGV4OiB0aGUgaW5kZXggb2Yg dGhlIHBoeQo+IGRpZmYgLS1naXQgYS9pbmNsdWRlL2xpbnV4L3BoeS9waHkuaCBiL2luY2x1ZGUv bGludXgvcGh5L3BoeS5oCj4gaW5kZXggOWNiYTdmZTE2YzIzLi4zY2MzMTVkY2ZjZDAgMTAwNjQ0 Cj4gLS0tIGEvaW5jbHVkZS9saW51eC9waHkvcGh5LmgKPiArKysgYi9pbmNsdWRlL2xpbnV4L3Bo eS9waHkuaAo+IEBAIC00NCw2ICs0NCwxMiBAQCBlbnVtIHBoeV9tb2RlIHsKPiAgfTsKPiAKPiAg LyoqCj4gKyAqIHVuaW9uIHBoeV9jb25maWd1cmVfb3B0cyAtIE9wYXF1ZSBnZW5lcmljIHBoeSBj b25maWd1cmF0aW9uCj4gKyAqLwo+ICt1bmlvbiBwaHlfY29uZmlndXJlX29wdHMgewo+ICt9Owo+ ICsKPiArLyoqCj4gICAqIHN0cnVjdCBwaHlfb3BzIC0gc2V0IG9mIGZ1bmN0aW9uIHBvaW50ZXJz IGZvciBwZXJmb3JtaW5nIHBoeSBvcGVyYXRpb25zCj4gICAqIEBpbml0OiBvcGVyYXRpb24gdG8g YmUgcGVyZm9ybWVkIGZvciBpbml0aWFsaXppbmcgcGh5Cj4gICAqIEBleGl0OiBvcGVyYXRpb24g dG8gYmUgcGVyZm9ybWVkIHdoaWxlIGV4aXRpbmcKPiBAQCAtNjAsNiArNjYsMzggQEAgc3RydWN0 IHBoeV9vcHMgewo+ICAJaW50CSgqcG93ZXJfb24pKHN0cnVjdCBwaHkgKnBoeSk7Cj4gIAlpbnQJ KCpwb3dlcl9vZmYpKHN0cnVjdCBwaHkgKnBoeSk7Cj4gIAlpbnQJKCpzZXRfbW9kZSkoc3RydWN0 IHBoeSAqcGh5LCBlbnVtIHBoeV9tb2RlIG1vZGUpOwo+ICsKPiArCS8qKgo+ICsJICogQGNvbmZp Z3VyZToKPiArCSAqCj4gKwkgKiBPcHRpb25hbC4KPiArCSAqCj4gKwkgKiBVc2VkIHRvIGNoYW5n ZSB0aGUgUEhZIHBhcmFtZXRlcnMuIHBoeV9pbml0KCkgbXVzdCBoYXZlCj4gKwkgKiBiZWVuIGNh bGxlZCBvbiB0aGUgcGh5Lgo+ICsJICoKPiArCSAqIFJldHVybnM6IDAgaWYgc3VjY2Vzc2Z1bCwg YW4gbmVnYXRpdmUgZXJyb3IgY29kZSBvdGhlcndpc2UKPiArCSAqLwo+ICsJaW50CSgqY29uZmln dXJlKShzdHJ1Y3QgcGh5ICpwaHksIGVudW0gcGh5X21vZGUgbW9kZSwKPiArCQkJICAgICB1bmlv biBwaHlfY29uZmlndXJlX29wdHMgKm9wdHMpOwoKSXMgdGhpcyBmdW5jdGlvbiBhbGxvd2VkIHRv IG1vZGlmeSBvcHRzID8gSWYgc28sIHRvIHdoYXQgZXh0ZW50ID8gSWYgbm90LCB0aGUgCnBvaW50 ZXIgc2hvdWxkIGJlIG1hZGUgY29uc3QuCgo+ICsJLyoqCj4gKwkgKiBAdmFsaWRhdGU6Cj4gKwkg Kgo+ICsJICogT3B0aW9uYWwuCj4gKwkgKgo+ICsJICogVXNlZCB0byBjaGVjayB0aGF0IHRoZSBj dXJyZW50IHNldCBvZiBwYXJhbWV0ZXJzIGNhbiBiZQo+ICsJICogaGFuZGxlZCBieSB0aGUgcGh5 LiBJbXBsZW1lbnRhdGlvbnMgYXJlIGZyZWUgdG8gdHVuZSB0aGUKPiArCSAqIHBhcmFtZXRlcnMg cGFzc2VkIGFzIGFyZ3VtZW50cyBpZiBuZWVkZWQgYnkgc29tZQo+ICsJICogaW1wbGVtZW50YXRp b24gZGV0YWlsIG9yIGNvbnN0cmFpbnRzLiBJdCBtdXN0IG5vdCBjaGFuZ2UKPiArCSAqIGFueSBh Y3R1YWwgY29uZmlndXJhdGlvbiBvZiB0aGUgUEhZLCBzbyBjYWxsaW5nIGl0IGFzIG1hbnkKPiAr CSAqIHRpbWVzIGFzIGRlZW1lZCBmaXQgYnkgdGhlIGNvbnN1bWVyIG11c3QgaGF2ZSBubyBzaWRl Cj4gKwkgKiBlZmZlY3QuCj4gKwkgKgo+ICsJICogUmV0dXJuczogMCBpZiB0aGUgY29uZmlndXJh dGlvbiBjYW4gYmUgYXBwbGllZCwgYW4gbmVnYXRpdmUKPiArCSAqIGVycm9yIGNvZGUgb3RoZXJ3 aXNlCgpXaGVuIHNob3VsZCB0aGlzIG9wZXJhdGlvbiBtb2RpZnkgdGhlIHBhc3NlZCBwYXJhbWV0 ZXJzLCBhbmQgd2hlbiBzaG91bGQgaXQgCnJldHVybiBhbiBlcnJvciA/IEkgdW5kZXJzdGFuZCB0 aGF0IHlvdXIgZ29hbCBpcyB0byBpbXBsZW1lbnQgYSBuZWdvdGlhdGlvbiAKbWVjaGFuaXNtIGZv ciB0aGUgUEhZIHBhcmFtZXRlcnMsIGFuZCB0byBiZSByZWFsbHkgdXNlZnVsIEkgdGhpbmsgd2Ug bmVlZCB0byAKZG9jdW1lbnQgaXQgbW9yZSBwcmVjaXNlbHkuCgo+ICsJICovCj4gKwlpbnQJKCp2 YWxpZGF0ZSkoc3RydWN0IHBoeSAqcGh5LCBlbnVtIHBoeV9tb2RlIG1vZGUsCj4gKwkJCSAgICB1 bmlvbiBwaHlfY29uZmlndXJlX29wdHMgKm9wdHMpOwo+ICAJaW50CSgqcmVzZXQpKHN0cnVjdCBw aHkgKnBoeSk7Cj4gIAlpbnQJKCpjYWxpYnJhdGUpKHN0cnVjdCBwaHkgKnBoeSk7Cj4gIAlzdHJ1 Y3QgbW9kdWxlICpvd25lcjsKPiBAQCAtMTY0LDYgKzIwMiwxMCBAQCBpbnQgcGh5X2V4aXQoc3Ry dWN0IHBoeSAqcGh5KTsKPiAgaW50IHBoeV9wb3dlcl9vbihzdHJ1Y3QgcGh5ICpwaHkpOwo+ICBp bnQgcGh5X3Bvd2VyX29mZihzdHJ1Y3QgcGh5ICpwaHkpOwo+ICBpbnQgcGh5X3NldF9tb2RlKHN0 cnVjdCBwaHkgKnBoeSwgZW51bSBwaHlfbW9kZSBtb2RlKTsKPiAraW50IHBoeV9jb25maWd1cmUo c3RydWN0IHBoeSAqcGh5LCBlbnVtIHBoeV9tb2RlIG1vZGUsCj4gKwkJICB1bmlvbiBwaHlfY29u ZmlndXJlX29wdHMgKm9wdHMpOwo+ICtpbnQgcGh5X3ZhbGlkYXRlKHN0cnVjdCBwaHkgKnBoeSwg ZW51bSBwaHlfbW9kZSBtb2RlLAo+ICsJCSB1bmlvbiBwaHlfY29uZmlndXJlX29wdHMgKm9wdHMp Owo+ICBzdGF0aWMgaW5saW5lIGVudW0gcGh5X21vZGUgcGh5X2dldF9tb2RlKHN0cnVjdCBwaHkg KnBoeSkKPiAgewo+ICAJcmV0dXJuIHBoeS0+YXR0cnMubW9kZTsKCi0tIApSZWdhcmRzLAoKTGF1 cmVudCBQaW5jaGFydAoKCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3Rv cC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmkt ZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from perceval.ideasonboard.com ([213.167.242.64]:56876 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726071AbeIESKA (ORCPT ); Wed, 5 Sep 2018 14:10:00 -0400 From: Laurent Pinchart To: Maxime Ripard Cc: Kishon Vijay Abraham I , Boris Brezillon , Thomas Petazzoni , linux-media@vger.kernel.org, Archit Taneja , Andrzej Hajda , Chen-Yu Tsai , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, Krzysztof Witos , Rafal Ciepiela Subject: Re: [PATCH 02/10] phy: Add configuration interface Date: Wed, 05 Sep 2018 16:39:46 +0300 Message-ID: <8397722.XVQDA25ZU6@avalon> In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-media-owner@vger.kernel.org List-ID: Hi Maxime, Thank you for the patch. 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 > --- > 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. > + mutex_lock(&phy->mutex); > + ret = phy->ops->configure(phy, mode, opts); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > + > +/** > + * phy_validate() - Checks the phy parameters > + * @phy: the phy returned by phy_get() > + * @mode: phy_mode the configuration is applicable to. > + * @opts: Configuration to check > + * > + * 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 will not change any actual configuration of the > + * PHY, so calling it as many times as deemed fit will have no side > + * effect. > + * > + * Returns: 0 if successful, an negative error code otherwise > + */ > +int phy_validate(struct phy *phy, enum phy_mode mode, > + union phy_configure_opts *opts) > +{ > + int ret; > + > + if (!phy) > + return -EINVAL; > + > + if (!phy->ops->validate) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->validate(phy, mode, opts); > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > + > +/** > * _of_phy_get() - lookup and obtain a reference to a phy by phandle > * @np: device_node for which to get the phy > * @index: the index of the phy > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index 9cba7fe16c23..3cc315dcfcd0 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -44,6 +44,12 @@ enum phy_mode { > }; > > /** > + * union phy_configure_opts - Opaque generic phy configuration > + */ > +union phy_configure_opts { > +}; > + > +/** > * struct phy_ops - set of function pointers for performing phy operations > * @init: operation to be performed for initializing phy > * @exit: operation to be performed while exiting > @@ -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. > + /** > + * @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. > + */ > + int (*validate)(struct phy *phy, enum phy_mode mode, > + union phy_configure_opts *opts); > int (*reset)(struct phy *phy); > int (*calibrate)(struct phy *phy); > struct module *owner; > @@ -164,6 +202,10 @@ int phy_exit(struct phy *phy); > int phy_power_on(struct phy *phy); > int phy_power_off(struct phy *phy); > int phy_set_mode(struct phy *phy, enum phy_mode mode); > +int phy_configure(struct phy *phy, enum phy_mode mode, > + union phy_configure_opts *opts); > +int phy_validate(struct phy *phy, enum phy_mode mode, > + union phy_configure_opts *opts); > static inline enum phy_mode phy_get_mode(struct phy *phy) > { > return phy->attrs.mode; -- Regards, Laurent Pinchart