From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey.Brodkin@synopsys.com (Alexey Brodkin) Date: Thu, 9 Feb 2017 19:02:30 +0000 Subject: [PATCH] net: phy: dp83867: Fall-back to default values of clock delay and FIFO depth In-Reply-To: <360cfba0-cc84-4378-025d-34e5eb1fc2c8@gmail.com> References: <20170206192445.35829-1-abrodkin@synopsys.com> <20170208.131519.1313210815958231832.davem@davemloft.net> <360cfba0-cc84-4378-025d-34e5eb1fc2c8@gmail.com> List-ID: Message-ID: <1486666949.2726.99.camel@synopsys.com> To: linux-snps-arc@lists.infradead.org Hi Florian, On Wed, 2017-02-08@10:20 -0800, Florian Fainelli wrote: > On 02/08/2017 10:15 AM, David Miller wrote: > > > > From: Alexey Brodkin > > Date: Mon,??6 Feb 2017 22:24:45 +0300 > > > > > > > > Given there're default values mentioned in the PHY datasheet > > > fall-back gracefully to them instead of silently return an error > > > through the whole call-chain. > > > > > > This allows to use minimalistic description in DT if no special > > > features are required. > > > > > > Signed-off-by: Alexey Brodkin > > > > If these defaults are legitimate to use, then you should probably also > > set them in the non-CONFIG_OF_MDIO case implementation of > > dp83867_of_init(). > > IIRC this is what Alexey is doing already, in case of errors, instead of > returning the error code, he changes the default values and does not > propagate the error code. > > Alexey, you are essentially making dp83867_of_init() impossible to fail > (or nearly) now, even with invalid properties, so I agree with David > here, you should probably have a function that runs after > dp83867_of_init() whose job is to set default values, if none have been > previously set through Device Tree. But why do we need to return error code from?dp83867_of_init()? The point is this function doesn't do any hardware setup as well, in fact it doesn't even reads anything from real hardware. So we may indeed report an error to above callers (which is not super convenient because the error comes many levels above without any error message making understanding of the failure pretty complicated - go trace where it came from) but what would it mean to a user? What do we want our user to do? Set at least something in required properties? Then why don't we do that ourselves right in the driver? Moreover since we're talking about defaults mentioned by the PHY manufacturer we will just use "factory settings". Still allowing others to override defaults with their .dts. I agree with David that it really worth to do the same settings for non_CONFIG_OF_MDIO case. -Alexey From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753928AbdBIU2n (ORCPT ); Thu, 9 Feb 2017 15:28:43 -0500 Received: from smtprelay.synopsys.com ([198.182.60.111]:35639 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752976AbdBIU2l (ORCPT ); Thu, 9 Feb 2017 15:28:41 -0500 From: Alexey Brodkin To: "f.fainelli@gmail.com" CC: "andrew@lunn.ch" , "linux-kernel@vger.kernel.org" , "nsekhar@ti.com" , "m-karicheri2@ti.com" , "mugunthanvnm@ti.com" , "linux-snps-arc@lists.infradead.org" , "grygorii.strashko@ti.com" , "netdev@vger.kernel.org" , "davem@davemloft.net" Subject: Re: [PATCH] net: phy: dp83867: Fall-back to default values of clock delay and FIFO depth Thread-Topic: [PATCH] net: phy: dp83867: Fall-back to default values of clock delay and FIFO depth Thread-Index: AQHSgK6ypwSegJa/ekatqRPhpoTHKqFfXHuAgAABTgCAAZ40gA== Date: Thu, 9 Feb 2017 19:02:30 +0000 Message-ID: <1486666949.2726.99.camel@synopsys.com> References: <20170206192445.35829-1-abrodkin@synopsys.com> <20170208.131519.1313210815958231832.davem@davemloft.net> <360cfba0-cc84-4378-025d-34e5eb1fc2c8@gmail.com> In-Reply-To: <360cfba0-cc84-4378-025d-34e5eb1fc2c8@gmail.com> Accept-Language: en-US, ru-RU Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.121.8.94] Content-Type: text/plain; charset="utf-8" Content-ID: <3C2DD6509A1D804FAF0B1B69CA0DA597@internal.synopsys.com> MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v19KSmKD003966 Hi Florian, On Wed, 2017-02-08 at 10:20 -0800, Florian Fainelli wrote: > On 02/08/2017 10:15 AM, David Miller wrote: > > > > From: Alexey Brodkin > > Date: Mon,  6 Feb 2017 22:24:45 +0300 > > > > > > > > Given there're default values mentioned in the PHY datasheet > > > fall-back gracefully to them instead of silently return an error > > > through the whole call-chain. > > > > > > This allows to use minimalistic description in DT if no special > > > features are required. > > > > > > Signed-off-by: Alexey Brodkin > > > > If these defaults are legitimate to use, then you should probably also > > set them in the non-CONFIG_OF_MDIO case implementation of > > dp83867_of_init(). > > IIRC this is what Alexey is doing already, in case of errors, instead of > returning the error code, he changes the default values and does not > propagate the error code. > > Alexey, you are essentially making dp83867_of_init() impossible to fail > (or nearly) now, even with invalid properties, so I agree with David > here, you should probably have a function that runs after > dp83867_of_init() whose job is to set default values, if none have been > previously set through Device Tree. But why do we need to return error code from dp83867_of_init()? The point is this function doesn't do any hardware setup as well, in fact it doesn't even reads anything from real hardware. So we may indeed report an error to above callers (which is not super convenient because the error comes many levels above without any error message making understanding of the failure pretty complicated - go trace where it came from) but what would it mean to a user? What do we want our user to do? Set at least something in required properties? Then why don't we do that ourselves right in the driver? Moreover since we're talking about defaults mentioned by the PHY manufacturer we will just use "factory settings". Still allowing others to override defaults with their .dts. I agree with David that it really worth to do the same settings for non_CONFIG_OF_MDIO case. -Alexey From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey Brodkin Subject: Re: [PATCH] net: phy: dp83867: Fall-back to default values of clock delay and FIFO depth Date: Thu, 9 Feb 2017 19:02:30 +0000 Message-ID: <1486666949.2726.99.camel@synopsys.com> References: <20170206192445.35829-1-abrodkin@synopsys.com> <20170208.131519.1313210815958231832.davem@davemloft.net> <360cfba0-cc84-4378-025d-34e5eb1fc2c8@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Cc: "andrew@lunn.ch" , "grygorii.strashko@ti.com" , "mugunthanvnm@ti.com" , "netdev@vger.kernel.org" , "nsekhar@ti.com" , "linux-kernel@vger.kernel.org" , "m-karicheri2@ti.com" , "linux-snps-arc@lists.infradead.org" , "davem@davemloft.net" To: "f.fainelli@gmail.com" Return-path: In-Reply-To: <360cfba0-cc84-4378-025d-34e5eb1fc2c8@gmail.com> Content-Language: en-US Content-ID: <3C2DD6509A1D804FAF0B1B69CA0DA597@internal.synopsys.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-snps-arc" Errors-To: linux-snps-arc-bounces+gla-linux-snps-arc=m.gmane.org@lists.infradead.org List-Id: netdev.vger.kernel.org SGkgRmxvcmlhbiwNCg0KT24gV2VkLCAyMDE3LTAyLTA4IGF0IDEwOjIwIC0wODAwLCBGbG9yaWFu IEZhaW5lbGxpIHdyb3RlOg0KPiBPbiAwMi8wOC8yMDE3IDEwOjE1IEFNLCBEYXZpZCBNaWxsZXIg d3JvdGU6DQo+ID4gDQo+ID4gRnJvbTogQWxleGV5IEJyb2RraW4gPEFsZXhleS5Ccm9ka2luQHN5 bm9wc3lzLmNvbT4NCj4gPiBEYXRlOiBNb24swqDCoDYgRmViIDIwMTcgMjI6MjQ6NDUgKzAzMDAN Cj4gPiANCj4gPiA+IA0KPiA+ID4gR2l2ZW4gdGhlcmUncmUgZGVmYXVsdCB2YWx1ZXMgbWVudGlv bmVkIGluIHRoZSBQSFkgZGF0YXNoZWV0DQo+ID4gPiBmYWxsLWJhY2sgZ3JhY2VmdWxseSB0byB0 aGVtIGluc3RlYWQgb2Ygc2lsZW50bHkgcmV0dXJuIGFuIGVycm9yDQo+ID4gPiB0aHJvdWdoIHRo ZSB3aG9sZSBjYWxsLWNoYWluLg0KPiA+ID4gDQo+ID4gPiBUaGlzIGFsbG93cyB0byB1c2UgbWlu aW1hbGlzdGljIGRlc2NyaXB0aW9uIGluIERUIGlmIG5vIHNwZWNpYWwNCj4gPiA+IGZlYXR1cmVz IGFyZSByZXF1aXJlZC4NCj4gPiA+IA0KPiA+ID4gU2lnbmVkLW9mZi1ieTogQWxleGV5IEJyb2Rr aW4gPGFicm9ka2luQHN5bm9wc3lzLmNvbT4NCj4gPiANCj4gPiBJZiB0aGVzZSBkZWZhdWx0cyBh cmUgbGVnaXRpbWF0ZSB0byB1c2UsIHRoZW4geW91IHNob3VsZCBwcm9iYWJseSBhbHNvDQo+ID4g c2V0IHRoZW0gaW4gdGhlIG5vbi1DT05GSUdfT0ZfTURJTyBjYXNlIGltcGxlbWVudGF0aW9uIG9m DQo+ID4gZHA4Mzg2N19vZl9pbml0KCkuDQo+IA0KPiBJSVJDIHRoaXMgaXMgd2hhdCBBbGV4ZXkg aXMgZG9pbmcgYWxyZWFkeSwgaW4gY2FzZSBvZiBlcnJvcnMsIGluc3RlYWQgb2YNCj4gcmV0dXJu aW5nIHRoZSBlcnJvciBjb2RlLCBoZSBjaGFuZ2VzIHRoZSBkZWZhdWx0IHZhbHVlcyBhbmQgZG9l cyBub3QNCj4gcHJvcGFnYXRlIHRoZSBlcnJvciBjb2RlLg0KPiANCj4gQWxleGV5LCB5b3UgYXJl IGVzc2VudGlhbGx5IG1ha2luZyBkcDgzODY3X29mX2luaXQoKSBpbXBvc3NpYmxlIHRvIGZhaWwN Cj4gKG9yIG5lYXJseSkgbm93LCBldmVuIHdpdGggaW52YWxpZCBwcm9wZXJ0aWVzLCBzbyBJIGFn cmVlIHdpdGggRGF2aWQNCj4gaGVyZSwgeW91IHNob3VsZCBwcm9iYWJseSBoYXZlIGEgZnVuY3Rp b24gdGhhdCBydW5zIGFmdGVyDQo+IGRwODM4Njdfb2ZfaW5pdCgpIHdob3NlIGpvYiBpcyB0byBz ZXQgZGVmYXVsdCB2YWx1ZXMsIGlmIG5vbmUgaGF2ZSBiZWVuDQo+IHByZXZpb3VzbHkgc2V0IHRo cm91Z2ggRGV2aWNlIFRyZWUuDQoNCkJ1dCB3aHkgZG8gd2UgbmVlZCB0byByZXR1cm4gZXJyb3Ig Y29kZSBmcm9twqBkcDgzODY3X29mX2luaXQoKT8NClRoZSBwb2ludCBpcyB0aGlzIGZ1bmN0aW9u IGRvZXNuJ3QgZG8gYW55IGhhcmR3YXJlIHNldHVwIGFzIHdlbGwsDQppbiBmYWN0IGl0IGRvZXNu J3QgZXZlbiByZWFkcyBhbnl0aGluZyBmcm9tIHJlYWwgaGFyZHdhcmUuDQoNClNvIHdlIG1heSBp bmRlZWQgcmVwb3J0IGFuIGVycm9yIHRvIGFib3ZlIGNhbGxlcnMgKHdoaWNoIGlzIG5vdCBzdXBl ciBjb252ZW5pZW50DQpiZWNhdXNlIHRoZSBlcnJvciBjb21lcyBtYW55IGxldmVscyBhYm92ZSB3 aXRob3V0IGFueSBlcnJvciBtZXNzYWdlIG1ha2luZw0KdW5kZXJzdGFuZGluZyBvZiB0aGUgZmFp bHVyZSBwcmV0dHkgY29tcGxpY2F0ZWQgLSBnbyB0cmFjZSB3aGVyZSBpdCBjYW1lIGZyb20pDQpi dXQgd2hhdCB3b3VsZCBpdCBtZWFuIHRvIGEgdXNlcj8gV2hhdCBkbyB3ZSB3YW50IG91ciB1c2Vy IHRvIGRvPw0KU2V0IGF0IGxlYXN0IHNvbWV0aGluZyBpbiByZXF1aXJlZCBwcm9wZXJ0aWVzPyBU aGVuIHdoeSBkb24ndCB3ZSBkbyB0aGF0IG91cnNlbHZlcw0KcmlnaHQgaW4gdGhlIGRyaXZlcj8g TW9yZW92ZXIgc2luY2Ugd2UncmUgdGFsa2luZyBhYm91dCBkZWZhdWx0cyBtZW50aW9uZWQgYnkN CnRoZSBQSFkgbWFudWZhY3R1cmVyIHdlIHdpbGwganVzdCB1c2UgImZhY3Rvcnkgc2V0dGluZ3Mi LiBTdGlsbCBhbGxvd2luZyBvdGhlcnMgdG8NCm92ZXJyaWRlIGRlZmF1bHRzIHdpdGggdGhlaXIg LmR0cy4NCg0KSSBhZ3JlZSB3aXRoIERhdmlkIHRoYXQgaXQgcmVhbGx5IHdvcnRoIHRvIGRvIHRo ZSBzYW1lIHNldHRpbmdzIGZvciBub25fQ09ORklHX09GX01ESU8NCmNhc2UuDQoNCi1BbGV4ZXkK X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KbGludXgtc25w cy1hcmMgbWFpbGluZyBsaXN0CmxpbnV4LXNucHMtYXJjQGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0 cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51eC1zbnBzLWFyYw==