From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Thu, 06 Sep 2018 19:51:05 +0300 Subject: [PATCH 02/10] phy: Add configuration interface In-Reply-To: <20180906144807.pn753tgfyovvheil@flea> References: <8397722.XVQDA25ZU6@avalon> <20180906144807.pn753tgfyovvheil@flea> Message-ID: <2403687.Gdit31W5bd@avalon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Maxime, 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 > >> --- > >> > >> 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). > >> + mutex_lock(&phy->mutex); > >> + ret = phy->ops->configure(phy, mode, opts); > >> + mutex_unlock(&phy->mutex); > >> + > >> + return ret; > >> +} [snip] > >> 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 [snip] > >> @@ -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(). > >> + /** > >> + * @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. -- 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: Thu, 06 Sep 2018 19:51:05 +0300 Message-ID: <2403687.Gdit31W5bd@avalon> References: <8397722.XVQDA25ZU6@avalon> <20180906144807.pn753tgfyovvheil@flea> 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 37FB8894A7 for ; Thu, 6 Sep 2018 16:51:00 +0000 (UTC) In-Reply-To: <20180906144807.pn753tgfyovvheil@flea> 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 SGkgTWF4aW1lLAoKT24gVGh1cnNkYXksIDYgU2VwdGVtYmVyIDIwMTggMTc6NDg6MDcgRUVTVCBN YXhpbWUgUmlwYXJkIHdyb3RlOgo+IE9uIFdlZCwgU2VwIDA1LCAyMDE4IGF0IDA0OjM5OjQ2UE0g KzAzMDAsIExhdXJlbnQgUGluY2hhcnQgd3JvdGU6Cj4gPiBPbiBXZWRuZXNkYXksIDUgU2VwdGVt YmVyIDIwMTggMTI6MTY6MzMgRUVTVCBNYXhpbWUgUmlwYXJkIHdyb3RlOgo+ID4+IFRoZSBwaHkg ZnJhbWV3b3JrIGlzIG9ubHkgYWxsb3dpbmcgdG8gY29uZmlndXJlIHRoZSBwb3dlciBzdGF0ZSBv ZiB0aGUKPiA+PiBQSFkgdXNpbmcgdGhlIGluaXQgYW5kIHBvd2VyX29uIGhvb2tzLCBhbmQgdGhl aXIgcG93ZXJfb2ZmIGFuZCBleGl0Cj4gPj4gY291bnRlcnBhcnRzLgo+ID4+IAo+ID4+IFdoaWxl IGl0IHdvcmtzIGZvciBtb3N0LCBzaW1wbGUsIFBIWXMgc3VwcG9ydGVkIHNvIGZhciwgc29tZSBt b3JlCj4gPj4gYWR2YW5jZWQgUEhZcyBuZWVkIHNvbWUgY29uZmlndXJhdGlvbiBkZXBlbmRpbmcg b24gcnVudGltZSBwYXJhbWV0ZXJzLgo+ID4+IFRoZXNlIFBIWXMgaGF2ZSBiZWVuIHN1cHBvcnRl ZCBieSBhIG51bWJlciBvZiBtZWFucyBhbHJlYWR5LCBvZnRlbiBieQo+ID4+IHVzaW5nIGFkLWhv YyBkcml2ZXJzIGluIHRoZWlyIGNvbnN1bWVyIGRyaXZlcnMuCj4gPj4gCj4gPj4gVGhhdCBkb2Vz bid0IHdvcmsgdG9vIHdlbGwgaG93ZXZlciwgd2hlbiBhIGNvbnN1bWVyIGRldmljZSBuZWVkcyB0 byBkZWFsCj4gPiAKPiA+IHMvZGVhbC9kZWFsIHdpdGgvCj4gPiAKPiA+PiBtdWx0aXBsZSBQSFlz LCBvciB3aGVuIG11bHRpcGxlIGNvbnN1bWVycyBuZWVkIHRvIGRlYWwgd2l0aCB0aGUgc2FtZSBQ SFkKPiA+PiAoYSBEU0kgZHJpdmVyIGFuZCBhIENTSSBkcml2ZXIgZm9yIGV4YW1wbGUpLgo+ID4+ IAo+ID4+IFNvIHdlJ2xsIGFkZCBhIG5ldyBpbnRlcmZhY2UsIHRocm91Z2ggdHdvIGZ1bnRpb25z LCBwaHlfdmFsaWRhdGUgYW5kCj4gPj4gcGh5X2NvbmZpZ3VyZS4gVGhlIGZpcnN0IG9uZSB3aWxs IGFsbG93IHRvIGNoZWNrIHRoYXQgYSBjdXJyZW50Cj4gPj4gY29uZmlndXJhdGlvbiwgZm9yIGEg Z2l2ZW4gbW9kZSwgaXMgYXBwbGljYWJsZS4gSXQgd2lsbCBhbHNvIGFsbG93IHRoZQo+ID4+IFBI WSBkcml2ZXIgdG8gdHVuZSB0aGUgc2V0dGluZ3MgZ2l2ZW4gYXMgcGFyYW1ldGVycyBhcyBpdCBz ZWVzIGZpdC4KPiA+PiAKPiA+PiBwaHlfY29uZmlndXJlIHdpbGwgYWN0dWFsbHkgYXBwbHkgdGhh dCBjb25maWd1cmF0aW9uIGluIHRoZSBwaHkgaXRzZWxmLgo+ID4+IAo+ID4+IFNpZ25lZC1vZmYt Ynk6IE1heGltZSBSaXBhcmQgPG1heGltZS5yaXBhcmRAYm9vdGxpbi5jb20+Cj4gPj4gLS0tCj4g Pj4gCj4gPj4gIGRyaXZlcnMvcGh5L3BoeS1jb3JlLmMgIHwgNjIgKysrKysrKysrKysrKysrKysr KysrKysrKysrKysrKysrKysrKysrKystCj4gPj4gIGluY2x1ZGUvbGludXgvcGh5L3BoeS5oIHwg NDIgKysrKysrKysrKysrKysrKysrKysrKysrKysrKy0KPiA+PiAgMiBmaWxlcyBjaGFuZ2VkLCAx MDQgaW5zZXJ0aW9ucygrKQo+ID4+IAo+ID4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL3BoeS9waHkt Y29yZS5jIGIvZHJpdmVycy9waHkvcGh5LWNvcmUuYwo+ID4+IGluZGV4IDM1ZmQzOGM1YTRhMS4u NmVhZjY1NWUzNzBmIDEwMDY0NAo+ID4+IC0tLSBhL2RyaXZlcnMvcGh5L3BoeS1jb3JlLmMKPiA+ PiArKysgYi9kcml2ZXJzL3BoeS9waHktY29yZS5jCj4gPj4gQEAgLTQwOCw2ICs0MDgsNjggQEAg aW50IHBoeV9jYWxpYnJhdGUoc3RydWN0IHBoeSAqcGh5KQo+ID4+ICBFWFBPUlRfU1lNQk9MX0dQ TChwaHlfY2FsaWJyYXRlKTsKPiA+PiAgCj4gPj4gIC8qKgo+ID4+ICsgKiBwaHlfY29uZmlndXJl KCkgLSBDaGFuZ2VzIHRoZSBwaHkgcGFyYW1ldGVycwo+ID4+ICsgKiBAcGh5OiB0aGUgcGh5IHJl dHVybmVkIGJ5IHBoeV9nZXQoKQo+ID4+ICsgKiBAbW9kZTogcGh5X21vZGUgdGhlIGNvbmZpZ3Vy YXRpb24gaXMgYXBwbGljYWJsZSB0by4KPiA+PiArICogQG9wdHM6IE5ldyBjb25maWd1cmF0aW9u IHRvIGFwcGx5Cj4gPj4gKyAqCj4gPj4gKyAqIFVzZWQgdG8gY2hhbmdlIHRoZSBQSFkgcGFyYW1l dGVycy4gcGh5X2luaXQoKSBtdXN0IGhhdmUKPiA+PiArICogYmVlbiBjYWxsZWQgb24gdGhlIHBo eS4KPiA+PiArICoKPiA+PiArICogUmV0dXJuczogMCBpZiBzdWNjZXNzZnVsLCBhbiBuZWdhdGl2 ZSBlcnJvciBjb2RlIG90aGVyd2lzZQo+ID4+ICsgKi8KPiA+PiAraW50IHBoeV9jb25maWd1cmUo c3RydWN0IHBoeSAqcGh5LCBlbnVtIHBoeV9tb2RlIG1vZGUsCj4gPj4gKwkJICB1bmlvbiBwaHlf Y29uZmlndXJlX29wdHMgKm9wdHMpCj4gPj4gK3sKPiA+PiArCWludCByZXQ7Cj4gPj4gKwo+ID4+ ICsJaWYgKCFwaHkpCj4gPj4gKwkJcmV0dXJuIC1FSU5WQUw7Cj4gPj4gKwo+ID4+ICsJaWYgKCFw aHktPm9wcy0+Y29uZmlndXJlKQo+ID4+ICsJCXJldHVybiAwOwo+ID4gCj4gPiBTaG91bGRuJ3Qg eW91IHJlcG9ydCBhbiBlcnJvciB0byB0aGUgY2FsbGVyID8gSWYgYSBjYWxsZXIgZXhwZWN0cyB0 aGUgUEhZCj4gPiB0byBiZSBjb25maWd1cmFibGUsIEkgd291bGQgYXNzdW1lIHRoYXQgc2lsZW50 bHkgaWdub3JpbmcgdGhlIHJlcXVlc3RlZAo+ID4gY29uZmlndXJhdGlvbiB3b24ndCB3b3JrIGdy ZWF0Lgo+IAo+IEknbSBub3Qgc3VyZS4gSSBhbHNvIGV4cGVjdCBhIGRldmljZSBoYXZpbmcgdG8g aW50ZXJhY3Qgd2l0aCBtdWx0aXBsZQo+IFBIWXMsIHNvbWUgb2YgdGhlbSBuZWVkaW5nIHNvbWUg Y29uZmlndXJhdGlvbiB3aGlsZSBzb21lIG90aGVyIGRvCj4gbm90LiBJbiB0aGF0IHNjZW5hcmlv LCByZXR1cm5pbmcgMCBzZWVtcyB0byBiZSB0aGUgcmlnaHQgdGhpbmcgdG8gZG8uCgpJdCBjb3Vs ZCBiZSB1cCB0byB0aGUgY2FsbGVyIHRvIGRlY2lkZSB3aGV0aGVyIHRvIGlnbm9yZSB0aGUgZXJy b3Igb3Igbm90IHdoZW4gCnRoZSBvcGVyYXRpb24gaXNuJ3QgaW1wbGVtZW50ZWQuIEkgZXhwZWN0 IHRoYXQgYSBjYWxsIHJlcXVpcmluZyBzcGVjaWZpYyAKY29uZmlndXJhdGlvbiBwYXJhbWV0ZXJz IGZvciBhIGdpdmVuIFBIWSBtaWdodCB3YW50IHRvIGJhaWwgb3V0IGlmIHRoZSAKY29uZmlndXJh dGlvbiBjYW4ndCBiZSBhcHBsaWVkLiBPbiB0aGUgb3RoZXIgaGFuZCB0aGF0IHNob3VsZCBuZXZl ciBoYXBwZW4gCndoZW4gdGhlIHN5c3RlbSBpcyBkZXNpZ25lZCBjb3JyZWN0bHksIGFzIHZlbmRv cnMgYXJlIG5vdCBzdXBwb3NlZCB0byBzaGlwIAprZXJuZWxzIHRoYXQgd291bGQgYmUgYnJva2Vu IGJ5IGRlc2lnbiAoYXMgaW4gcmVxdWlyaW5nIGEgY29uZmlndXJlIG9wZXJhdGlvbiAKYnV0IG5v dCBwcm92aWRpbmcgaXQpLgoKPiA+PiArCW11dGV4X2xvY2soJnBoeS0+bXV0ZXgpOwo+ID4+ICsJ cmV0ID0gcGh5LT5vcHMtPmNvbmZpZ3VyZShwaHksIG1vZGUsIG9wdHMpOwo+ID4+ICsJbXV0ZXhf dW5sb2NrKCZwaHktPm11dGV4KTsKPiA+PiArCj4gPj4gKwlyZXR1cm4gcmV0Owo+ID4+ICt9Cgpb c25pcF0KCj4gPj4gZGlmZiAtLWdpdCBhL2luY2x1ZGUvbGludXgvcGh5L3BoeS5oIGIvaW5jbHVk ZS9saW51eC9waHkvcGh5LmgKPiA+PiBpbmRleCA5Y2JhN2ZlMTZjMjMuLjNjYzMxNWRjZmNkMCAx MDA2NDQKPiA+PiAtLS0gYS9pbmNsdWRlL2xpbnV4L3BoeS9waHkuaAo+ID4+ICsrKyBiL2luY2x1 ZGUvbGludXgvcGh5L3BoeS5oCgpbc25pcF0KCj4gPj4gQEAgLTYwLDYgKzY2LDM4IEBAIHN0cnVj dCBwaHlfb3BzIHsKPiA+PiAgCWludAkoKnBvd2VyX29uKShzdHJ1Y3QgcGh5ICpwaHkpOwo+ID4+ ICAJaW50CSgqcG93ZXJfb2ZmKShzdHJ1Y3QgcGh5ICpwaHkpOwo+ID4+ICAJaW50CSgqc2V0X21v ZGUpKHN0cnVjdCBwaHkgKnBoeSwgZW51bSBwaHlfbW9kZSBtb2RlKTsKPiA+PiArCj4gPj4gKwkv KioKPiA+PiArCSAqIEBjb25maWd1cmU6Cj4gPj4gKwkgKgo+ID4+ICsJICogT3B0aW9uYWwuCj4g Pj4gKwkgKgo+ID4+ICsJICogVXNlZCB0byBjaGFuZ2UgdGhlIFBIWSBwYXJhbWV0ZXJzLiBwaHlf aW5pdCgpIG11c3QgaGF2ZQo+ID4+ICsJICogYmVlbiBjYWxsZWQgb24gdGhlIHBoeS4KPiA+PiAr CSAqCj4gPj4gKwkgKiBSZXR1cm5zOiAwIGlmIHN1Y2Nlc3NmdWwsIGFuIG5lZ2F0aXZlIGVycm9y IGNvZGUgb3RoZXJ3aXNlCj4gPj4gKwkgKi8KPiA+PiArCWludAkoKmNvbmZpZ3VyZSkoc3RydWN0 IHBoeSAqcGh5LCBlbnVtIHBoeV9tb2RlIG1vZGUsCj4gPj4gKwkJCSAgICAgdW5pb24gcGh5X2Nv bmZpZ3VyZV9vcHRzICpvcHRzKTsKPiA+IAo+ID4gSXMgdGhpcyBmdW5jdGlvbiBhbGxvd2VkIHRv IG1vZGlmeSBvcHRzID8gSWYgc28sIHRvIHdoYXQgZXh0ZW50ID8gSWYgbm90LAo+ID4gdGhlIHBv aW50ZXIgc2hvdWxkIGJlIG1hZGUgY29uc3QuCj4gCj4gVGhhdCdzIGEgcHJldHR5IGdvb2QgcXVl c3Rpb24uIEkgZ3Vlc3MgaXQgY291bGQgbW9kaWZ5IGl0IHRvIHRoZSBzYW1lCj4gZXh0ZW50IHRo YW4gdmFsaWRhdGUgY291bGQuIFdvdWxkIHRoYXQgbWFrZSBzZW5zZT8KCkl0IHdvdWxkLCBvciB3 ZSBjb3VsZCBzYXkgdGhhdCBQSFkgdXNlcnMgYXJlIHJlcXVpcmVkIHRvIGNhbGwgdGhlIHZhbGlk YXRlIApmdW5jdGlvbiBmaXJzdCwgYW5kIHRoZSB0aGUgY29uZmlndXJlIGZ1bmN0aW9uIHdpbGwg cmV0dXJuIGFuIGVycm9yIGlmIHRoZSAKcGFzc2VkIGNvbmZpZ3VyYXRpb24gaXNuJ3QgdmFsaWQu IFRoYXQgd291bGQgYXZvaWQgZG91YmxlLXZhbGlkYXRpb24gd2hlbiB0aGUgClBIWSB1c2VyIHVz ZXMgLnZhbGlkYXRlKCkuCgo+ID4+ICsJLyoqCj4gPj4gKwkgKiBAdmFsaWRhdGU6Cj4gPj4gKwkg Kgo+ID4+ICsJICogT3B0aW9uYWwuCj4gPj4gKwkgKgo+ID4+ICsJICogVXNlZCB0byBjaGVjayB0 aGF0IHRoZSBjdXJyZW50IHNldCBvZiBwYXJhbWV0ZXJzIGNhbiBiZQo+ID4+ICsJICogaGFuZGxl ZCBieSB0aGUgcGh5LiBJbXBsZW1lbnRhdGlvbnMgYXJlIGZyZWUgdG8gdHVuZSB0aGUKPiA+PiAr CSAqIHBhcmFtZXRlcnMgcGFzc2VkIGFzIGFyZ3VtZW50cyBpZiBuZWVkZWQgYnkgc29tZQo+ID4+ ICsJICogaW1wbGVtZW50YXRpb24gZGV0YWlsIG9yIGNvbnN0cmFpbnRzLiBJdCBtdXN0IG5vdCBj aGFuZ2UKPiA+PiArCSAqIGFueSBhY3R1YWwgY29uZmlndXJhdGlvbiBvZiB0aGUgUEhZLCBzbyBj YWxsaW5nIGl0IGFzIG1hbnkKPiA+PiArCSAqIHRpbWVzIGFzIGRlZW1lZCBmaXQgYnkgdGhlIGNv bnN1bWVyIG11c3QgaGF2ZSBubyBzaWRlCj4gPj4gKwkgKiBlZmZlY3QuCj4gPj4gKwkgKgo+ID4+ ICsJICogUmV0dXJuczogMCBpZiB0aGUgY29uZmlndXJhdGlvbiBjYW4gYmUgYXBwbGllZCwgYW4g bmVnYXRpdmUKPiA+PiArCSAqIGVycm9yIGNvZGUgb3RoZXJ3aXNlCj4gPiAKPiA+IFdoZW4gc2hv dWxkIHRoaXMgb3BlcmF0aW9uIG1vZGlmeSB0aGUgcGFzc2VkIHBhcmFtZXRlcnMsIGFuZCB3aGVu IHNob3VsZAo+ID4gaXQgcmV0dXJuIGFuIGVycm9yID8gSSB1bmRlcnN0YW5kIHRoYXQgeW91ciBn b2FsIGlzIHRvIGltcGxlbWVudCBhCj4gPiBuZWdvdGlhdGlvbiBtZWNoYW5pc20gZm9yIHRoZSBQ SFkgcGFyYW1ldGVycywgYW5kIHRvIGJlIHJlYWxseSB1c2VmdWwgSQo+ID4gdGhpbmsgd2UgbmVl ZCB0byBkb2N1bWVudCBpdCBtb3JlIHByZWNpc2VseS4KPiAKPiBNeSBpbml0aWFsIGlkZWEgd2Fz IHRvIHJlamVjdCBhIGNvbmZpZ3VyYXRpb24gdGhhdCB3b3VsZG4ndCBiZQo+IGFjaGlldmFibGUg YnkgdGhlIFBIWSwgaWUgeW91J3JlIGFza2luZyBzb21ldGhpbmcgdGhhdCBpcyBvdXRzaWRlIG9m Cj4gdGhlIG9wZXJhdGluZyBib3VuZGFyaWVzLCB3aGlsZSB5b3Ugd291bGQgYmUgYWJsZSB0byBj aGFuZ2Ugc2V0dGluZ3MKPiB0aGF0IHdvdWxkIGJlIG9wZXJhdGlvbmFsLCBidXQgc3ViLW9wdGlt YWwuCgpJJ20gZmluZSB3aXRoIHRoYXQsIGxldCdzIGRvY3VtZW50IGl0IGV4cGxpY2l0bHkuCgot LSAKUmVnYXJkcywKCkxhdXJlbnQgUGluY2hhcnQKCgoKX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxA bGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxt YW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from perceval.ideasonboard.com ([213.167.242.64]:45354 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726541AbeIFV1V (ORCPT ); Thu, 6 Sep 2018 17:27:21 -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: Thu, 06 Sep 2018 19:51:05 +0300 Message-ID: <2403687.Gdit31W5bd@avalon> In-Reply-To: <20180906144807.pn753tgfyovvheil@flea> References: <8397722.XVQDA25ZU6@avalon> <20180906144807.pn753tgfyovvheil@flea> 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, 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 > >> --- > >> > >> 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). > >> + mutex_lock(&phy->mutex); > >> + ret = phy->ops->configure(phy, mode, opts); > >> + mutex_unlock(&phy->mutex); > >> + > >> + return ret; > >> +} [snip] > >> 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 [snip] > >> @@ -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(). > >> + /** > >> + * @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. -- Regards, Laurent Pinchart