Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1203338 > unrolled thread

Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code Florian Fainelli <f.fainelli@gmail.com> - 2015-08-08 19:40 +0200
    Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code Stas Sergeev <stsp@list.ru> - 2015-08-11 18:10 +0200
      RE: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code Madalin-Cristian Bucur <madalin.bucur@freescale.com> - 2015-08-11 18:40 +0200
        Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code Stas Sergeev <stsp@list.ru> - 2015-08-11 19:00 +0200
          RE: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code Madalin-Cristian Bucur <madalin.bucur@freescale.com> - 2015-08-12 15:30 +0200
            Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code Stas Sergeev <stsp@list.ru> - 2015-08-12 16:00 +0200
              RE: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code Madalin-Cristian Bucur <madalin.bucur@freescale.com> - 2015-08-12 16:50 +0200
                Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code Stas Sergeev <stsp@list.ru> - 2015-08-12 17:20 +0200
                  RE: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code Madalin-Cristian Bucur <madalin.bucur@freescale.com> - 2015-08-12 17:30 +0200
                    Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code Stas Sergeev <stsp@list.ru> - 2015-08-12 18:10 +0200

#1203338 — Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

CC'ing Stas,

Le 08/05/15 07:42, Madalin Bucur a écrit :
> The FMan MAC configuration code needs the speed and duplex information
> for fixed-link interfaces that is parsed now by the of function
> of_phy_register_fixed_link(). This parses the fixed-link parameters but
> does not expose to the caller neither the phy_device pointer nor the
> status struct where it loads the fixed-link params. By extracting the
> fixed-link parsing code from of_phy_register_fixed_link() into a
> separate function the parsed values are made available without changing
> the existing API. This change also removes a small redundancy in the
> previous code calling fixed_phy_register().

I will look into this shortly, sorry for the delay.

> 
> The FMan patch relies on the latest FMan driver v4 submission by Igal Liberman:
> https://patchwork.ozlabs.org/project/netdev/list/?submitter=Igal.Liberman&state=*&q=v4
> 
> Madalin Bucur (2):
>   of: separate fixed link parsing from registration
>   fsl_fman: use fixed_phy_status for MEMAC
> 
>  .../ethernet/freescale/fman/flib/fsl_fman_memac.h  |  6 ++-
>  drivers/net/ethernet/freescale/fman/inc/mac.h      |  2 +-
>  drivers/net/ethernet/freescale/fman/mac/fm_memac.c | 42 ++++++++++++-----
>  drivers/net/ethernet/freescale/fman/mac/fm_memac.h |  3 +-
>  drivers/net/ethernet/freescale/fman/mac/mac.c      | 18 ++++++--
>  drivers/of/of_mdio.c                               | 52 ++++++++++++++--------
>  include/linux/of_mdio.h                            |  9 ++++
>  7 files changed, 94 insertions(+), 38 deletions(-)
> 


-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1205251

08.08.2015 20:32, Florian Fainelli пишет:
> CC'ing Stas,
Hi.

> Le 08/05/15 07:42, Madalin Bucur a écrit :
>> The FMan MAC configuration code needs the speed and duplex information
>> for fixed-link interfaces that is parsed now by the of function
>> of_phy_register_fixed_link(). This parses the fixed-link parameters but
>> does not expose to the caller neither the phy_device pointer nor the
>> status struct where it loads the fixed-link params.
I have only barely touched that code, but IMO both things
are by design. There are some API deficiencies, and so, many
drivers still use of_phy_find_device() to circumvent the encapsulation
and get the phy_device pointer, but this is unlikely a good thing
to do. I even proposed some API extensions, but there was no
interest.

>>   By extracting the
>> fixed-link parsing code from of_phy_register_fixed_link() into a
>> separate function the parsed values are made available without changing
>> the existing API. This change also removes a small redundancy in the
>> previous code calling fixed_phy_register().
Today, the fixed_link is not always fixed.
See for example this patch (already mainlined):
https://lkml.org/lkml/2015/7/20/711
of_phy_is_fixed_link() returns 'true' if you have
managed="in-band-status", and so the SGMII in-band status
can update fixed-link params.

So my question is: why do you even need to know whether
the link is fixed or not? IIRC you can check the phy_device
pointer in the adjust_link callback of of_phy_connect() to get
the current link status values. Why is this not enough for your
task? Maybe the patch description should be updated to include
why the current technique is bad, what is actually fixed by the
change.
I think using the fixed-link DT values directly is not something
to be done. The encapsulation is there for a reason, so maybe
instead we can see what API additions do we need to avoid the
current limitations that force people to use of_phy_find_device()
and other work-arounds.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1205268

KyBKb2FraW0sIFNoYW9odWkNCg0KPiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9t
OiBTdGFzIFNlcmdlZXYgW21haWx0bzpzdHNwQGxpc3QucnVdDQo+IA0KPiAwOC4wOC4yMDE1IDIw
OjMyLCBGbG9yaWFuIEZhaW5lbGxpINC/0LjRiNC10YI6DQo+ID4gQ0MnaW5nIFN0YXMsDQo+IEhp
Lg0KPiANCj4gPiBMZSAwOC8wNS8xNSAwNzo0MiwgTWFkYWxpbiBCdWN1ciBhIMOpY3JpdCA6DQo+
ID4+IFRoZSBGTWFuIE1BQyBjb25maWd1cmF0aW9uIGNvZGUgbmVlZHMgdGhlIHNwZWVkIGFuZCBk
dXBsZXgNCj4gaW5mb3JtYXRpb24NCj4gPj4gZm9yIGZpeGVkLWxpbmsgaW50ZXJmYWNlcyB0aGF0
IGlzIHBhcnNlZCBub3cgYnkgdGhlIG9mIGZ1bmN0aW9uDQo+ID4+IG9mX3BoeV9yZWdpc3Rlcl9m
aXhlZF9saW5rKCkuIFRoaXMgcGFyc2VzIHRoZSBmaXhlZC1saW5rIHBhcmFtZXRlcnMgYnV0DQo+
ID4+IGRvZXMgbm90IGV4cG9zZSB0byB0aGUgY2FsbGVyIG5laXRoZXIgdGhlIHBoeV9kZXZpY2Ug
cG9pbnRlciBub3IgdGhlDQo+ID4+IHN0YXR1cyBzdHJ1Y3Qgd2hlcmUgaXQgbG9hZHMgdGhlIGZp
eGVkLWxpbmsgcGFyYW1zLg0KPiBJIGhhdmUgb25seSBiYXJlbHkgdG91Y2hlZCB0aGF0IGNvZGUs
IGJ1dCBJTU8gYm90aCB0aGluZ3MNCj4gYXJlIGJ5IGRlc2lnbi4gVGhlcmUgYXJlIHNvbWUgQVBJ
IGRlZmljaWVuY2llcywgYW5kIHNvLCBtYW55DQo+IGRyaXZlcnMgc3RpbGwgdXNlIG9mX3BoeV9m
aW5kX2RldmljZSgpIHRvIGNpcmN1bXZlbnQgdGhlIGVuY2Fwc3VsYXRpb24NCj4gYW5kIGdldCB0
aGUgcGh5X2RldmljZSBwb2ludGVyLCBidXQgdGhpcyBpcyB1bmxpa2VseSBhIGdvb2QgdGhpbmcN
Cj4gdG8gZG8uIEkgZXZlbiBwcm9wb3NlZCBzb21lIEFQSSBleHRlbnNpb25zLCBidXQgdGhlcmUg
d2FzIG5vDQo+IGludGVyZXN0Lg0KPiANCj4gPj4gICBCeSBleHRyYWN0aW5nIHRoZQ0KPiA+PiBm
aXhlZC1saW5rIHBhcnNpbmcgY29kZSBmcm9tIG9mX3BoeV9yZWdpc3Rlcl9maXhlZF9saW5rKCkg
aW50byBhDQo+ID4+IHNlcGFyYXRlIGZ1bmN0aW9uIHRoZSBwYXJzZWQgdmFsdWVzIGFyZSBtYWRl
IGF2YWlsYWJsZSB3aXRob3V0IGNoYW5naW5nDQo+ID4+IHRoZSBleGlzdGluZyBBUEkuIFRoaXMg
Y2hhbmdlIGFsc28gcmVtb3ZlcyBhIHNtYWxsIHJlZHVuZGFuY3kgaW4gdGhlDQo+ID4+IHByZXZp
b3VzIGNvZGUgY2FsbGluZyBmaXhlZF9waHlfcmVnaXN0ZXIoKS4NCj4gVG9kYXksIHRoZSBmaXhl
ZF9saW5rIGlzIG5vdCBhbHdheXMgZml4ZWQuDQo+IFNlZSBmb3IgZXhhbXBsZSB0aGlzIHBhdGNo
IChhbHJlYWR5IG1haW5saW5lZCk6DQo+IGh0dHBzOi8vbGttbC5vcmcvbGttbC8yMDE1LzcvMjAv
NzExDQo+IG9mX3BoeV9pc19maXhlZF9saW5rKCkgcmV0dXJucyAndHJ1ZScgaWYgeW91IGhhdmUN
Cj4gbWFuYWdlZD0iaW4tYmFuZC1zdGF0dXMiLCBhbmQgc28gdGhlIFNHTUlJIGluLWJhbmQgc3Rh
dHVzDQo+IGNhbiB1cGRhdGUgZml4ZWQtbGluayBwYXJhbXMuDQo+IA0KPiBTbyBteSBxdWVzdGlv
biBpczogd2h5IGRvIHlvdSBldmVuIG5lZWQgdG8ga25vdyB3aGV0aGVyDQo+IHRoZSBsaW5rIGlz
IGZpeGVkIG9yIG5vdD8gSUlSQyB5b3UgY2FuIGNoZWNrIHRoZSBwaHlfZGV2aWNlDQo+IHBvaW50
ZXIgaW4gdGhlIGFkanVzdF9saW5rIGNhbGxiYWNrIG9mIG9mX3BoeV9jb25uZWN0KCkgdG8gZ2V0
DQo+IHRoZSBjdXJyZW50IGxpbmsgc3RhdHVzIHZhbHVlcy4gV2h5IGlzIHRoaXMgbm90IGVub3Vn
aCBmb3IgeW91cg0KPiB0YXNrPyBNYXliZSB0aGUgcGF0Y2ggZGVzY3JpcHRpb24gc2hvdWxkIGJl
IHVwZGF0ZWQgdG8gaW5jbHVkZQ0KPiB3aHkgdGhlIGN1cnJlbnQgdGVjaG5pcXVlIGlzIGJhZCwg
d2hhdCBpcyBhY3R1YWxseSBmaXhlZCBieSB0aGUNCj4gY2hhbmdlLg0KPiBJIHRoaW5rIHVzaW5n
IHRoZSBmaXhlZC1saW5rIERUIHZhbHVlcyBkaXJlY3RseSBpcyBub3Qgc29tZXRoaW5nDQo+IHRv
IGJlIGRvbmUuIFRoZSBlbmNhcHN1bGF0aW9uIGlzIHRoZXJlIGZvciBhIHJlYXNvbiwgc28gbWF5
YmUNCj4gaW5zdGVhZCB3ZSBjYW4gc2VlIHdoYXQgQVBJIGFkZGl0aW9ucyBkbyB3ZSBuZWVkIHRv
IGF2b2lkIHRoZQ0KPiBjdXJyZW50IGxpbWl0YXRpb25zIHRoYXQgZm9yY2UgcGVvcGxlIHRvIHVz
ZSBvZl9waHlfZmluZF9kZXZpY2UoKQ0KPiBhbmQgb3RoZXIgd29yay1hcm91bmRzLg0KDQpJIG5l
ZWQgdG8gYmUgYWJsZSB0byBkZXRlcm1pbmUgdGhlIGltcG9zZWQgc3BlZWQgYW5kIGR1cGxleCBm
b3IgZml4ZWQgbGluaw0KZXh0ZXJuYWwgUEhZcyBiZWNhdXNlIEkgbmVlZCB0byBjb25maWd1cmUg
dGhlIGludGVybmFsIFBIWSB3aXRoIG1hdGNoaW5nDQp2YWx1ZXMuIElmIEkgZG8gbm90IHNldCB0
aGUgc2FtZSBzcGVlZCwgZ2l2ZW4gdGhlIGZhY3QgdGhhdCBBTiBuZWVkcyB0byBiZSBvZmYsDQp0
aGVyZSB3aWxsIGJlIG5vIGxpbmsgYW5kIG5vIGFkanVzdCBsaW5rIHRvIGZpeCB0aGluZ3MgbGF0
ZXIgKGFuZCB0aGUgaW50ZXJuYWwgUEhZIGlzDQpub3QgdXBkYXRlZCBieSBhZGp1c3QgbGluayBh
bnl3YXkpLiBJIGRvIG5vdCBoYXZlIGFjY2VzcyBhdCB0aGUgcGh5IHBvaW50ZXIgYXQNCnRoZSB0
aW1lIEkgbmVlZCB0aGUgc3BlZWQgYW5kIGR1cGxleCwgdG8gcmV0cmlldmUgdGhlIGRlZmF1bHRz
IGZyb20gdGhlcmUgYW5kDQpJJ3ZlIHRyaWVkIHRvIG1ha2UgdGhlIHNtYWxsZXN0IGNoYW5nZXMg
dGhhdCBhbGxvdyBtZSB0byByZXRyaWV2ZSB0aG9zZSB3aXRob3V0DQptb2RpZnlpbmcgZXhpc3Rp
bmcgQVBJLg0KV2h5IGlzIGl0IGltcG9ydGFudCB0byBoaWRlIHRoZSBkZWZhdWx0IHZhbHVlcyBm
cm9tIHRoZSBNQUMgZHJpdmVyPw0KDQpUaGFuayB5b3UsDQpNYWRhbGluDQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1205276

11.08.2015 19:33, Madalin-Cristian Bucur пишет:
> + Joakim, Shaohui
>
>> -----Original Message-----
>> From: Stas Sergeev [mailto:stsp@list.ru]
>>
>> 08.08.2015 20:32, Florian Fainelli пишет:
>>> CC'ing Stas,
>> Hi.
>>
>>> Le 08/05/15 07:42, Madalin Bucur a écrit :
>>>> The FMan MAC configuration code needs the speed and duplex
>> information
>>>> for fixed-link interfaces that is parsed now by the of function
>>>> of_phy_register_fixed_link(). This parses the fixed-link parameters but
>>>> does not expose to the caller neither the phy_device pointer nor the
>>>> status struct where it loads the fixed-link params.
>> I have only barely touched that code, but IMO both things
>> are by design. There are some API deficiencies, and so, many
>> drivers still use of_phy_find_device() to circumvent the encapsulation
>> and get the phy_device pointer, but this is unlikely a good thing
>> to do. I even proposed some API extensions, but there was no
>> interest.
>>
>>>>    By extracting the
>>>> fixed-link parsing code from of_phy_register_fixed_link() into a
>>>> separate function the parsed values are made available without changing
>>>> the existing API. This change also removes a small redundancy in the
>>>> previous code calling fixed_phy_register().
>> Today, the fixed_link is not always fixed.
>> See for example this patch (already mainlined):
>> https://lkml.org/lkml/2015/7/20/711
>> of_phy_is_fixed_link() returns 'true' if you have
>> managed="in-band-status", and so the SGMII in-band status
>> can update fixed-link params.
>>
>> So my question is: why do you even need to know whether
>> the link is fixed or not? IIRC you can check the phy_device
>> pointer in the adjust_link callback of of_phy_connect() to get
>> the current link status values. Why is this not enough for your
>> task? Maybe the patch description should be updated to include
>> why the current technique is bad, what is actually fixed by the
>> change.
>> I think using the fixed-link DT values directly is not something
>> to be done. The encapsulation is there for a reason, so maybe
>> instead we can see what API additions do we need to avoid the
>> current limitations that force people to use of_phy_find_device()
>> and other work-arounds.
> I need to be able to determine the imposed speed and duplex for fixed link
> external PHYs because I need to configure the internal PHY with matching
> values. If I do not set the same speed, given the fact that AN needs to be off,
> there will be no link and no adjust link to fix things later (and the internal PHY is
> not updated by adjust link anyway). I do not have access at the phy pointer at
> the time I need the speed and duplex, to retrieve the defaults from there and
> I've tried to make the smallest changes that allow me to retrieve those without
> modifying existing API.
> Why is it important to hide the default values from the MAC driver?
My worry is that the fixed values are not really fixed, and
therefore are not always useful to access directly. It is likely
not a problem for your use-case, as, as you say, the AN is
disabled, but this is probably not the best to do in general.
And also you do:
---

-		err = of_phy_register_fixed_link(mac_node);
-		if (err)
+		struct phy_device *phy;
+
+		mac_dev->fixed_link = kzalloc(sizeof(*mac_dev->fixed_link),
+					      GFP_KERNEL);
+		if (of_phy_parse_fixed_link(mac_node, mac_dev->fixed_link))
+			goto _return_dev_set_drvdata;
+
+		phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link,
+					 mac_node);

---

which means you really want to circumvent the current OF
api quite a lot, without saying why in the patch description.
As such, it may be difficult to review. Could you please write
a more complete description to the patch?

As to your problem: would it be possible to set speed & duplex
after you do of_phy_connect()? It returns the phy_device
pointer, and perhaps you can look into phydev->speed and
phydev->duplex at that point?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1206085

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBTdGFzIFNlcmdlZXYgW21haWx0
bzpzdHNwQGxpc3QucnVdDQo+IA0KPiAxMS4wOC4yMDE1IDE5OjMzLCBNYWRhbGluLUNyaXN0aWFu
IEJ1Y3VyINC/0LjRiNC10YI6DQo+ID4gKyBKb2FraW0sIFNoYW9odWkNCj4gPg0KPiA+PiAtLS0t
LU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiA+PiBGcm9tOiBTdGFzIFNlcmdlZXYgW21haWx0bzpz
dHNwQGxpc3QucnVdDQo+ID4+DQo+ID4+IDA4LjA4LjIwMTUgMjA6MzIsIEZsb3JpYW4gRmFpbmVs
bGkg0L/QuNGI0LXRgjoNCj4gPj4+IENDJ2luZyBTdGFzLA0KPiA+PiBIaS4NCj4gPj4NCj4gPj4+
IExlIDA4LzA1LzE1IDA3OjQyLCBNYWRhbGluIEJ1Y3VyIGEgw6ljcml0IDoNCj4gPj4+PiBUaGUg
Rk1hbiBNQUMgY29uZmlndXJhdGlvbiBjb2RlIG5lZWRzIHRoZSBzcGVlZCBhbmQgZHVwbGV4DQo+
ID4+IGluZm9ybWF0aW9uDQo+ID4+Pj4gZm9yIGZpeGVkLWxpbmsgaW50ZXJmYWNlcyB0aGF0IGlz
IHBhcnNlZCBub3cgYnkgdGhlIG9mIGZ1bmN0aW9uDQo+ID4+Pj4gb2ZfcGh5X3JlZ2lzdGVyX2Zp
eGVkX2xpbmsoKS4gVGhpcyBwYXJzZXMgdGhlIGZpeGVkLWxpbmsgcGFyYW1ldGVycyBidXQNCj4g
Pj4+PiBkb2VzIG5vdCBleHBvc2UgdG8gdGhlIGNhbGxlciBuZWl0aGVyIHRoZSBwaHlfZGV2aWNl
IHBvaW50ZXIgbm9yIHRoZQ0KPiA+Pj4+IHN0YXR1cyBzdHJ1Y3Qgd2hlcmUgaXQgbG9hZHMgdGhl
IGZpeGVkLWxpbmsgcGFyYW1zLg0KPHNuaXA+DQoNCj4gPiBJJ3ZlIHRyaWVkIHRvIG1ha2UgdGhl
IHNtYWxsZXN0IGNoYW5nZXMgdGhhdCBhbGxvdyBtZSB0byByZXRyaWV2ZSB0aG9zZQ0KPiA+IHdp
dGhvdXQgbW9kaWZ5aW5nIGV4aXN0aW5nIEFQSS4NCj4gPiBXaHkgaXMgaXQgaW1wb3J0YW50IHRv
IGhpZGUgdGhlIGRlZmF1bHQgdmFsdWVzIGZyb20gdGhlIE1BQyBkcml2ZXI/DQo+IE15IHdvcnJ5
IGlzIHRoYXQgdGhlIGZpeGVkIHZhbHVlcyBhcmUgbm90IHJlYWxseSBmaXhlZCwgYW5kDQo+IHRo
ZXJlZm9yZSBhcmUgbm90IGFsd2F5cyB1c2VmdWwgdG8gYWNjZXNzIGRpcmVjdGx5LiBJdCBpcyBs
aWtlbHkNCj4gbm90IGEgcHJvYmxlbSBmb3IgeW91ciB1c2UtY2FzZSwgYXMsIGFzIHlvdSBzYXks
IHRoZSBBTiBpcw0KPiBkaXNhYmxlZCwgYnV0IHRoaXMgaXMgcHJvYmFibHkgbm90IHRoZSBiZXN0
IHRvIGRvIGluIGdlbmVyYWwuDQoNClllcywgbm90IGEgcHJvYmxlbSBpbiBteSBjYXNlLg0KDQo+
IEFuZCBhbHNvIHlvdSBkbzoNCj4gLS0tDQo+IA0KPiAtCQllcnIgPSBvZl9waHlfcmVnaXN0ZXJf
Zml4ZWRfbGluayhtYWNfbm9kZSk7DQo+IC0JCWlmIChlcnIpDQo+ICsJCXN0cnVjdCBwaHlfZGV2
aWNlICpwaHk7DQo+ICsNCj4gKwkJbWFjX2Rldi0+Zml4ZWRfbGluayA9IGt6YWxsb2Moc2l6ZW9m
KCptYWNfZGV2LQ0KPiA+Zml4ZWRfbGluayksDQo+ICsJCQkJCSAgICAgIEdGUF9LRVJORUwpOw0K
PiArCQlpZiAob2ZfcGh5X3BhcnNlX2ZpeGVkX2xpbmsobWFjX25vZGUsIG1hY19kZXYtDQo+ID5m
aXhlZF9saW5rKSkNCj4gKwkJCWdvdG8gX3JldHVybl9kZXZfc2V0X2RydmRhdGE7DQo+ICsNCj4g
KwkJcGh5ID0gZml4ZWRfcGh5X3JlZ2lzdGVyKFBIWV9QT0xMLCBtYWNfZGV2LT5maXhlZF9saW5r
LA0KPiArCQkJCQkgbWFjX25vZGUpOw0KPiANCj4gLS0tDQo+IA0KPiB3aGljaCBtZWFucyB5b3Ug
cmVhbGx5IHdhbnQgdG8gY2lyY3VtdmVudCB0aGUgY3VycmVudCBPRg0KPiBhcGkgcXVpdGUgYSBs
b3QsIHdpdGhvdXQgc2F5aW5nIHdoeSBpbiB0aGUgcGF0Y2ggZGVzY3JpcHRpb24uDQoNCkkgY2ly
Y3VtdmVudCB0aGUgQVBJIGJlY2F1c2UgSSBkaW4gbm90IHdhbnQgdG8gY2hhbmdlIGV4aXN0aW5n
IEFQSS4NCklmIEkgY291bGQgZ2V0IGEgcmVmZXJlbmNlIHRvIHRoZSBzdGF0dXMgc3RydWN0IHdp
dGhvdXQgY2hhbmdpbmcgYW55IGNvZGUgDQpvciB3aXRob3V0IGJlaW5nIHJlcXVpcmVkIHRvIGNh
bGwgYnkgbXlzZWxmIGZpeGVkX3BoeV9yZWdpc3RlcigpLCBJDQp3b3VsZCBvZiBkb25lIHRoYXQu
IEdpdmVuIHRoZSBleGlzdGluZyBjb2RlIGluIG9mX3BoeV9yZWdpc3Rlcl9maXhlZF9saW5rKCks
DQp0aGlzIHdhcyBteSBvbmx5IG9wdGlvbi4gSSBjb3VsZCBoYXZlIGJyb2tlbiBvZl9waHlfcmVn
aXN0ZXJfZml4ZWRfbGluaygpDQppbiB0d28gZnVuY3Rpb25zOg0KDQpvZl9waHlfcGFyc2VfZml4
ZWRfbGluaygpIGFuZCBvZl9waHlfcmVnaXN0ZXJfZml4ZWRfbGluaygpLCB0aGUgbGF0dGVyIGRv
aW5nIG9ubHkNCnRoZSBjYWxsIHRvIGZpeGVkX3BoeV9yZWdpc3RlcigpDQoNCnRoYXQgd291bGQg
YWxsb3cgdG8ga2VlcCBvZl9waHlfcmVnaXN0ZXJfZml4ZWRfbGluaygpIGFzIGl0IGlzLCBicm9r
ZW4gaW4gdHdvIHN0YWdlczoNCg0KLSBwYXJzaW5nDQotIHJlZ2lzdGVyaW5nDQoNCnRoYW4gY2Fu
IGJlIHVzZWQgYnkgb3RoZXIgZHJpdmVycyBpbiBvcmRlciB0byBnZXQgdGhlIHN0YXR1cyBidXQg
SSB0aGluayBpdCdzIG92ZXJraWxsLg0KDQo+IEFzIHN1Y2gsIGl0IG1heSBiZSBkaWZmaWN1bHQg
dG8gcmV2aWV3LiBDb3VsZCB5b3UgcGxlYXNlIHdyaXRlDQo+IGEgbW9yZSBjb21wbGV0ZSBkZXNj
cmlwdGlvbiB0byB0aGUgcGF0Y2g/DQoNClRvIGJldHRlciB1bmRlcnN0YW5kIHRoaXMgcGF0Y2gs
IHRoaW5rIG9mIGl0IGFzIGp1c3QgYSByZWZhY3RvcmluZyBvZiB0aGUNCm9mX3BoeV9yZWdpc3Rl
cl9maXhlZF9saW5rKCkgdGhhdCBkb2VzIHR3byB0aGluZ3MgaW5zaWRlOg0KDQotIHBhcnNpbmcg
b2YgZml4ZWQgbGluayBub2RlICgyIGJpbmRpbmdzIHN1cHBvcnRlZCkNCi0gcmVnaXN0ZXIgcGh5
IGJ5IGNhbGxpbmcgZml4ZWRfcGh5X3JlZ2lzdGVyKCkgaW4gdGhlIHNhbWUgd2F5LCBpbiB0aGUg
c2FtZSBjb2RlYmFzZQ0KDQpJJ3ZlIGV4dHJhY3RlZCB0aGUgcGFyc2luZyBpbiBhIHNlcGFyYXRl
IGZ1bmN0aW9uICggZm9sbG93aW5nIHRoZSAib25lIGZ1bmN0aW9uIHNob3VsZA0KZG8gb25lIHRo
aW5nIiBydWxlKS4NCg0KVGhlbiBJJ3ZlIGV4cG9ydGVkIHRoaXMgZnVuY3Rpb24gdG8gbWFrZSBz
dGF0dXMgYXZhaWxhYmxlIHRvIGNhbGxlcnMuDQoNCj4gQXMgdG8geW91ciBwcm9ibGVtOiB3b3Vs
ZCBpdCBiZSBwb3NzaWJsZSB0byBzZXQgc3BlZWQgJiBkdXBsZXgNCj4gYWZ0ZXIgeW91IGRvIG9m
X3BoeV9jb25uZWN0KCk/IEl0IHJldHVybnMgdGhlIHBoeV9kZXZpY2UNCj4gcG9pbnRlciwgYW5k
IHBlcmhhcHMgeW91IGNhbiBsb29rIGludG8gcGh5ZGV2LT5zcGVlZCBhbmQNCj4gcGh5ZGV2LT5k
dXBsZXggYXQgdGhhdCBwb2ludD8NCg0KSXQgd291bGQgYmUgcG9zc2libGUgYnV0IHVuLW5hdHVy
YWwgYXMgSSdkIGhhdmUgcHJvYmluZyBpbmZvcm1hdGlvbiBvbmx5IGF2YWlsYWJsZSBhdA0KcnVu
dGltZS4gVGhhdCB3b3VsZCBqdXN0IGNvbXBsaWNhdGUgbWF0dGVycyBmb3IgbXkgcGFydGljdWxh
ciBjYXNlIGFucyBJIHN1c3BlY3QgdGhlcmUNCndpbGwgYmUgb3RoZXIgZHJpdmVycyB0aGF0IGdl
dCBpbnRvIHRoaXMgc2l0dWF0aW9uLiBZb3UgYXJlIGNvbmNlcm5lZCBhYm91dCBwZW9wbGUNCmFi
dXNpbmcgdGhpcyBBUEkgdG8gcmVhZCBmaXhlZCBsaW5rIHN0YXR1cyB3aGVuIHRoZSBsaW5rIGlz
IG5vdCByZWFsbHkgZml4ZWQsIEknbSBjb25jZXJuZWQNCmFib3V0IGRlY2xhcmluZyB0aGUgbGlu
ayBhcyBmaXhlZC1saW5rIHdoZW4gaXQncyBub3QuIE1heWJlIHRoZSBuYW1pbmcvYmluZGluZyBu
ZWVkcyB0byBiZQ0KcmV2aXNlZCB0byBjb3ZlciB0aGUgY2FzZSB3aGVuIGFsbCBpcyBmaXhlZCBi
dXQgdGhlIGxpbmsuDQoNCg==
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1206133

12.08.2015 16:26, Madalin-Cristian Bucur пишет:
>>> I've tried to make the smallest changes that allow me to retrieve those
>>> without modifying existing API.
>>> Why is it important to hide the default values from the MAC driver?
>> My worry is that the fixed values are not really fixed, and
>> therefore are not always useful to access directly. It is likely
>> not a problem for your use-case, as, as you say, the AN is
>> disabled, but this is probably not the best to do in general.
> Yes, not a problem in my case.
>
>> And also you do:
>> ---
>>
>> -		err = of_phy_register_fixed_link(mac_node);
>> -		if (err)
>> +		struct phy_device *phy;
>> +
>> +		mac_dev->fixed_link = kzalloc(sizeof(*mac_dev-
>>> fixed_link),
>> +					      GFP_KERNEL);
>> +		if (of_phy_parse_fixed_link(mac_node, mac_dev-
>>> fixed_link))
>> +			goto _return_dev_set_drvdata;
>> +
>> +		phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link,
>> +					 mac_node);
>>
>> ---
>>
>> which means you really want to circumvent the current OF
>> api quite a lot, without saying why in the patch description.
> I circumvent the API because I din not want to change existing API.
> If I could get a reference to the status struct without changing any code
> or without being required to call by myself fixed_phy_register(), I
> would of done that. Given the existing code in of_phy_register_fixed_link(),
> this was my only option. I could have broken of_phy_register_fixed_link()
> in two functions:
>
> of_phy_parse_fixed_link() and of_phy_register_fixed_link(), the latter doing only
> the call to fixed_phy_register()
>
> that would allow to keep of_phy_register_fixed_link() as it is, broken in two stages:
>
> - parsing
> - registering
>
> than can be used by other drivers in order to get the status but I think it's overkill.
What I referred to as "circumventing an API" is that you do
phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link, + mac_node);
by hands, instead of letting the of_phy_register_fixed_link() doing so.

How about a smaller circumvention, like this for instance:
---
err = of_phy_register_fixed_link(mac_node);
phy = of_phy_find_device(dn);
status = fixed_phy_get_link_status(phy);    // no such func, to be coded up
---

Or even like this:
---
err = of_phy_register_fixed_link(mac_node);
phy = of_phy_find_device(dn);
set_speed_and_duplex(phy->speed, phy->duplex);    // not sure if these 
values are available that early
---

Also I meant the description should have been in the patch,
not in the e-mail. :) You only wrote _what_ the patch does
(which is of course obvious from the code itself), but not
_why_ and _what was fixed_ (what didn't work).

>> As to your problem: would it be possible to set speed & duplex
>> after you do of_phy_connect()? It returns the phy_device
>> pointer, and perhaps you can look into phydev->speed and
>> phydev->duplex at that point?
> It would be possible but un-natural as I'd have probing information only available at
> runtime.
This is un-natural only if you deal just with a fixed case.
If your driver can deal also with the non-fixed cases
(either AN or MDIO), then this looks more natural as the
non-fixed management should be done at any point of time,
and certainly _after_ of_phy_connect(). So if your driver is
universal, this look like the natural choise to me, but if it is
limited to the fixed case, then, as a simplification, you move
that to the init time.
But I am not argueing what is more natural. Maybe the
above approaches with of_phy_find_device() can be used
in init time?

>   That would just complicate matters for my particular case ans I suspect there
> will be other drivers that get into this situation.
I suspect only those that are limited to the fixed-link case.
Only then they may decide to move phy management to
init time, but IMHO this is just an optimization.

>   You are concerned about people
> abusing this API to read fixed link status when the link is not really fixed, I'm concerned
> about declaring the link as fixed-link when it's not. Maybe the naming/binding needs to be
> revised to cover the case when all is fixed but the link.
Yes, naming is the problem. fixed-link is just a bad name.
See how it is defined:
---
Some Ethernet MACs have a "fixed link", and are not connected to a
normal MDIO-managed PHY device.
---
To me this means any non-MDIO PHY connection, but
unfortunately the name "fixed-link" suggests a bit more
than advertised. :(
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1206183

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBTdGFzIFNlcmdlZXYgW21haWx0
bzpzdHNwQGxpc3QucnVdDQo+DQo+IDEyLjA4LjIwMTUgMTY6MjYsIE1hZGFsaW4tQ3Jpc3RpYW4g
QnVjdXIg0L/QuNGI0LXRgjoNCj4gPj4+IEkndmUgdHJpZWQgdG8gbWFrZSB0aGUgc21hbGxlc3Qg
Y2hhbmdlcyB0aGF0IGFsbG93IG1lIHRvIHJldHJpZXZlIHRob3NlDQo+ID4+PiB3aXRob3V0IG1v
ZGlmeWluZyBleGlzdGluZyBBUEkuDQo+ID4+PiBXaHkgaXMgaXQgaW1wb3J0YW50IHRvIGhpZGUg
dGhlIGRlZmF1bHQgdmFsdWVzIGZyb20gdGhlIE1BQyBkcml2ZXI/DQo+ID4+IE15IHdvcnJ5IGlz
IHRoYXQgdGhlIGZpeGVkIHZhbHVlcyBhcmUgbm90IHJlYWxseSBmaXhlZCwgYW5kDQo+ID4+IHRo
ZXJlZm9yZSBhcmUgbm90IGFsd2F5cyB1c2VmdWwgdG8gYWNjZXNzIGRpcmVjdGx5LiBJdCBpcyBs
aWtlbHkNCj4gPj4gbm90IGEgcHJvYmxlbSBmb3IgeW91ciB1c2UtY2FzZSwgYXMsIGFzIHlvdSBz
YXksIHRoZSBBTiBpcw0KPiA+PiBkaXNhYmxlZCwgYnV0IHRoaXMgaXMgcHJvYmFibHkgbm90IHRo
ZSBiZXN0IHRvIGRvIGluIGdlbmVyYWwuDQo+ID4gWWVzLCBub3QgYSBwcm9ibGVtIGluIG15IGNh
c2UuDQo+ID4NCj4gPj4gQW5kIGFsc28geW91IGRvOg0KPiA+PiAtLS0NCj4gPj4NCj4gPj4gLQkJ
ZXJyID0gb2ZfcGh5X3JlZ2lzdGVyX2ZpeGVkX2xpbmsobWFjX25vZGUpOw0KPiA+PiAtCQlpZiAo
ZXJyKQ0KPiA+PiArCQlzdHJ1Y3QgcGh5X2RldmljZSAqcGh5Ow0KPiA+PiArDQo+ID4+ICsJCW1h
Y19kZXYtPmZpeGVkX2xpbmsgPSBremFsbG9jKHNpemVvZigqbWFjX2Rldi0NCj4gPj4+IGZpeGVk
X2xpbmspLA0KPiA+PiArCQkJCQkgICAgICBHRlBfS0VSTkVMKTsNCj4gPj4gKwkJaWYgKG9mX3Bo
eV9wYXJzZV9maXhlZF9saW5rKG1hY19ub2RlLCBtYWNfZGV2LQ0KPiA+Pj4gZml4ZWRfbGluaykp
DQo+ID4+ICsJCQlnb3RvIF9yZXR1cm5fZGV2X3NldF9kcnZkYXRhOw0KPiA+PiArDQo+ID4+ICsJ
CXBoeSA9IGZpeGVkX3BoeV9yZWdpc3RlcihQSFlfUE9MTCwgbWFjX2Rldi0+Zml4ZWRfbGluaywN
Cj4gPj4gKwkJCQkJIG1hY19ub2RlKTsNCj4gPj4NCj4gPj4gLS0tDQo+ID4+DQo+ID4+IHdoaWNo
IG1lYW5zIHlvdSByZWFsbHkgd2FudCB0byBjaXJjdW12ZW50IHRoZSBjdXJyZW50IE9GDQo+ID4+
IGFwaSBxdWl0ZSBhIGxvdCwgd2l0aG91dCBzYXlpbmcgd2h5IGluIHRoZSBwYXRjaCBkZXNjcmlw
dGlvbi4NCj4gPiBJIGNpcmN1bXZlbnQgdGhlIEFQSSBiZWNhdXNlIEkgZGluIG5vdCB3YW50IHRv
IGNoYW5nZSBleGlzdGluZyBBUEkuDQo+ID4gSWYgSSBjb3VsZCBnZXQgYSByZWZlcmVuY2UgdG8g
dGhlIHN0YXR1cyBzdHJ1Y3Qgd2l0aG91dCBjaGFuZ2luZyBhbnkgY29kZQ0KPiA+IG9yIHdpdGhv
dXQgYmVpbmcgcmVxdWlyZWQgdG8gY2FsbCBieSBteXNlbGYgZml4ZWRfcGh5X3JlZ2lzdGVyKCks
IEkNCj4gPiB3b3VsZCBvZiBkb25lIHRoYXQuIEdpdmVuIHRoZSBleGlzdGluZyBjb2RlIGluDQo+
IG9mX3BoeV9yZWdpc3Rlcl9maXhlZF9saW5rKCksDQo+ID4gdGhpcyB3YXMgbXkgb25seSBvcHRp
b24uIEkgY291bGQgaGF2ZSBicm9rZW4gb2ZfcGh5X3JlZ2lzdGVyX2ZpeGVkX2xpbmsoKQ0KPiA+
IGluIHR3byBmdW5jdGlvbnM6DQo+ID4NCj4gPiBvZl9waHlfcGFyc2VfZml4ZWRfbGluaygpIGFu
ZCBvZl9waHlfcmVnaXN0ZXJfZml4ZWRfbGluaygpLCB0aGUgbGF0dGVyDQo+IGRvaW5nIG9ubHkN
Cj4gPiB0aGUgY2FsbCB0byBmaXhlZF9waHlfcmVnaXN0ZXIoKQ0KPiA+DQo+ID4gdGhhdCB3b3Vs
ZCBhbGxvdyB0byBrZWVwIG9mX3BoeV9yZWdpc3Rlcl9maXhlZF9saW5rKCkgYXMgaXQgaXMsIGJy
b2tlbiBpbg0KPiB0d28gc3RhZ2VzOg0KPiA+DQo+ID4gLSBwYXJzaW5nDQo+ID4gLSByZWdpc3Rl
cmluZw0KPiA+DQo+ID4gdGhhbiBjYW4gYmUgdXNlZCBieSBvdGhlciBkcml2ZXJzIGluIG9yZGVy
IHRvIGdldCB0aGUgc3RhdHVzIGJ1dCBJIHRoaW5rIGl0J3MNCj4gb3ZlcmtpbGwuDQo+IFdoYXQg
SSByZWZlcnJlZCB0byBhcyAiY2lyY3VtdmVudGluZyBhbiBBUEkiIGlzIHRoYXQgeW91IGRvDQo+
IHBoeSA9IGZpeGVkX3BoeV9yZWdpc3RlcihQSFlfUE9MTCwgbWFjX2Rldi0+Zml4ZWRfbGluaywg
KyBtYWNfbm9kZSk7DQo+IGJ5IGhhbmRzLCBpbnN0ZWFkIG9mIGxldHRpbmcgdGhlIG9mX3BoeV9y
ZWdpc3Rlcl9maXhlZF9saW5rKCkgZG9pbmcgc28uDQo+IA0KPiBIb3cgYWJvdXQgYSBzbWFsbGVy
IGNpcmN1bXZlbnRpb24sIGxpa2UgdGhpcyBmb3IgaW5zdGFuY2U6DQo+IC0tLQ0KPiBlcnIgPSBv
Zl9waHlfcmVnaXN0ZXJfZml4ZWRfbGluayhtYWNfbm9kZSk7DQo+IHBoeSA9IG9mX3BoeV9maW5k
X2RldmljZShkbik7DQo+IHN0YXR1cyA9IGZpeGVkX3BoeV9nZXRfbGlua19zdGF0dXMocGh5KTsg
ICAgLy8gbm8gc3VjaCBmdW5jLCB0byBiZSBjb2RlZCB1cA0KPiAtLS0NCj4gDQo+IE9yIGV2ZW4g
bGlrZSB0aGlzOg0KPiAtLS0NCj4gZXJyID0gb2ZfcGh5X3JlZ2lzdGVyX2ZpeGVkX2xpbmsobWFj
X25vZGUpOw0KPiBwaHkgPSBvZl9waHlfZmluZF9kZXZpY2UoZG4pOw0KPiBzZXRfc3BlZWRfYW5k
X2R1cGxleChwaHktPnNwZWVkLCBwaHktPmR1cGxleCk7ICAgIC8vIG5vdCBzdXJlIGlmIHRoZXNl
DQo+IHZhbHVlcyBhcmUgYXZhaWxhYmxlIHRoYXQgZWFybHkNCj4gLS0tDQoNCkFmdGVyIG15IHBh
dGNoLCBhbGwgdGhhdCBvZl9waHlfcmVnaXN0ZXJfZml4ZWRfbGluaygpIGRvZXMgaXMgdG8gY2Fs
bA0KdGhlIG5ldyBwYXJzaW5nIGZ1bmN0aW9uIEkgaW50cm9kdWNlZCB0aGVuIHJlZ2lzdGVyIHRo
ZSBmaXhlZCBQSFkuDQpJIGNvdWxkIGhhdmUgZG9uZSB0aGlzIChwc2V1ZG9jb2RlKToNCg0KLSBh
ZGQgb2ZfcGh5X3BhcnNlX2ZpeGVkX2xpbmsoKSBhcyBzZWVuIGluIHRoZSBwYXRjaA0KLSBhZGQg
b2ZfcGh5X3JlZ2lzdGVyX2ZpeGVkX3BoeSgpIHRoYXQganVzdCBjYWxscyBmaXhlZF9waHlfcmVn
aXN0ZXIoKToNCg0KaW50IG9mX3BoeV9yZWdpc3Rlcl9maXhlZF9waHkobm9kZSkNCnsNCglwaHkg
PSBmaXhlZF9waHlfcmVnaXN0ZXIoUEhZX1BPTEwsIG1hY19kZXYtPmZpeGVkX2xpbmssDQoJCQkJ
ICAgICBtYWNfbm9kZSk7DQoJcmV0dXJuICghcGh5KTsNCn0NCg0KLSBjaGFuZ2Ugb2ZfcGh5X3Jl
Z2lzdGVyX2ZpeGVkX2xpbmsoKSB0byBjb250YWluIG9ubHkgdGhpczoNCg0KaW50IG9mX3BoeV9y
ZWdpc3Rlcl9maXhlZF9saW5rKG5vZGUpDQp7DQoJb2ZfcGh5X3BhcnNlX2ZpeGVkX2xpbmsobm9k
ZSwgJnN0YXR1cyk7DQoNCglyZXR1cm4gb2ZfcGh5X3JlZ2lzdGVyX2ZpeGVkX3BoeShub2RlKTsN
Cn0NCg0KVGhlbiBJIGNvdWxkIGNhbGwgb25seSBvZl8qIGZ1bmN0aW9ucyBidXQgdGhlIGVuZCBy
ZXN1bHQgd291bGQgYmUgdGhlIHNhbWUgYW5kDQpvZl9waHlfcmVnaXN0ZXJfZml4ZWRfcGh5KCkg
d291bGQgbm90IGp1c3RpZnkgaXRzIGV4aXN0ZW5jZSB0aGF0IG11Y2guLi4NCg0KVGhlIGdldHRl
ciBmb3Igc3RhdHVzIHlvdSBzdWdnZXN0IHdvdWxkIGJlIGZpbmUsIGJ1dCBub3Qgc3VyZSBob3cg
b25lIHdvdWxkIHJldHJpZXZlDQppdCBmcm9tIHRoZSBtYWNfbm9kZSB1bmxlc3Mgd2UgY2hhbmdl
IG9mX3BoeV9yZWdpc3Rlcl9maXhlZF9saW5rKCkgdG8gcmV0dXJuIHRoZQ0KcG9pbnRlciB0byBw
aHkgKGFuZCBhbGwgdGhlIGRyaXZlcnMgdGhhdCB1c2UgaXQuLi4pLg0KDQo+IA0KPiBBbHNvIEkg
bWVhbnQgdGhlIGRlc2NyaXB0aW9uIHNob3VsZCBoYXZlIGJlZW4gaW4gdGhlIHBhdGNoLA0KPiBu
b3QgaW4gdGhlIGUtbWFpbC4gOikgWW91IG9ubHkgd3JvdGUgX3doYXRfIHRoZSBwYXRjaCBkb2Vz
DQo+ICh3aGljaCBpcyBvZiBjb3Vyc2Ugb2J2aW91cyBmcm9tIHRoZSBjb2RlIGl0c2VsZiksIGJ1
dCBub3QNCj4gX3doeV8gYW5kIF93aGF0IHdhcyBmaXhlZF8gKHdoYXQgZGlkbid0IHdvcmspLg0K
PiANCg0KDQpJZiB5b3UgcmVmZXIgdG8gdGhlIGZpcnN0LCBzZXBhcmF0aW9uIHBhdGNoLCBJIHRo
b3VnaHQgdGhlIGRlc2NyaXB0aW9uIHdhcyBlbm91Z2g6DQoNCiAgICBvZjogc2VwYXJhdGUgZml4
ZWQgbGluayBwYXJzaW5nIGZyb20gcmVnaXN0cmF0aW9uDQogICAgDQogICAgU29tZSBkcml2ZXJz
IG1heSBuZWVkIHRvIHBhcnNlIHRoZSBmaXhlZCBsaW5rIHZhbHVlcyBiZWZvcmUgcmVnaXN0ZXJp
bmcNCiAgICB0aGUgZml4ZWQgbGluayBwaHkuIFNlcGFyYXRlIHRoZSBwYXJzaW5nIGZyb20gdGhl
IGFjdHVhbCByZWdpc3RyYXRpb24NCiAgICBhbmQgcHJvdmlkZSBhbiBleHBvcnQgZm9yIHRoZSBh
ZGRlZCBwYXJzaW5nIGZ1bmN0aW9uLg0KICAgIA0KICAgIFNpZ25lZC1vZmYtYnk6IE1hZGFsaW4g
QnVjdXIgPG1hZGFsaW4uYnVjdXJAZnJlZXNjYWxlLmNvbT4NCg0KRm9yIHRoaXMgb25lIGl0IHdh
cyBhIGJpdCBicmllZiwgSSBhZG1pdCAtIHRoZSBsb25nZXIgdmVyc2lvbiB3b3VsZCBiZSB0aGF0
IGJlZm9yZSBpdA0Kd2Ugd2VyZSBub3QgdXNpbmcgZnJvbSBmaXhlZCBsaW5rIGFueXRoaW5nIGVs
c2UgYnV0IHRoZSBmYWN0IHRoZSBsaW5rIHdhcyBmaXhlZA0KKGlnbm9yZWQgYWN0dWFsIHNwZWVk
LCBkdXBsZXggdmFsdWVzIHRoZXJlKSBhbmQgdGhpcyBwYXRjaCB0cmllcyB0byBmaXggdGhhdC4N
CkluIHRoZSBlbmQgdGhpcyBwYXRjaCB3aWxsIGJlIHNxdWFzaGVkIGluIGEgbmV3IEZNYW4gcGF0
Y2ggc2V0LCBsZXQgbWUgdXNlIHRoYXQgYXMNCmFuIGV4Y3VzZSBmb3IgdGhlIGJyaWVmIGNvbW1p
dCBsb2cgOikNCg0KPHNuaXA+DQoNCj4gPiAgIFlvdSBhcmUgY29uY2VybmVkIGFib3V0IHBlb3Bs
ZQ0KPiA+IGFidXNpbmcgdGhpcyBBUEkgdG8gcmVhZCBmaXhlZCBsaW5rIHN0YXR1cyB3aGVuIHRo
ZSBsaW5rIGlzIG5vdCByZWFsbHkgZml4ZWQsIEknbQ0KPiBjb25jZXJuZWQNCj4gPiBhYm91dCBk
ZWNsYXJpbmcgdGhlIGxpbmsgYXMgZml4ZWQtbGluayB3aGVuIGl0J3Mgbm90LiBNYXliZSB0aGUN
Cj4gbmFtaW5nL2JpbmRpbmcgbmVlZHMgdG8gYmUNCj4gPiByZXZpc2VkIHRvIGNvdmVyIHRoZSBj
YXNlIHdoZW4gYWxsIGlzIGZpeGVkIGJ1dCB0aGUgbGluay4NCj4gWWVzLCBuYW1pbmcgaXMgdGhl
IHByb2JsZW0uIGZpeGVkLWxpbmsgaXMganVzdCBhIGJhZCBuYW1lLg0KPiBTZWUgaG93IGl0IGlz
IGRlZmluZWQ6DQo+IC0tLQ0KPiBTb21lIEV0aGVybmV0IE1BQ3MgaGF2ZSBhICJmaXhlZCBsaW5r
IiwgYW5kIGFyZSBub3QgY29ubmVjdGVkIHRvIGENCj4gbm9ybWFsIE1ESU8tbWFuYWdlZCBQSFkg
ZGV2aWNlLg0KPiAtLS0NCj4gVG8gbWUgdGhpcyBtZWFucyBhbnkgbm9uLU1ESU8gUEhZIGNvbm5l
Y3Rpb24sIGJ1dA0KPiB1bmZvcnR1bmF0ZWx5IHRoZSBuYW1lICJmaXhlZC1saW5rIiBzdWdnZXN0
cyBhIGJpdCBtb3JlDQo+IHRoYW4gYWR2ZXJ0aXNlZC4gOigNCg0KWWVzLCBtYXliZSB0aGUgbmV3
IGJpbmRpbmcgY291bGQgYmUgdXBkYXRlZCB0byBsb2FkIHNlbWFudGljYWxseSB0aGUgcHJlc2Vu
Y2UNCm9yIGFic2VuY2Ugb2YgY2VydGFpbiBwYXJhbWV0ZXJzIHN1Y2ggYXMgbGluay4NCg==
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1206207

12.08.2015 17:43, Madalin-Cristian Bucur пишет:
>> -----Original Message-----
>> From: Stas Sergeev [mailto:stsp@list.ru]
>>
>> 12.08.2015 16:26, Madalin-Cristian Bucur пишет:
>>>>> I've tried to make the smallest changes that allow me to retrieve those
>>>>> without modifying existing API.
>>>>> Why is it important to hide the default values from the MAC driver?
>>>> My worry is that the fixed values are not really fixed, and
>>>> therefore are not always useful to access directly. It is likely
>>>> not a problem for your use-case, as, as you say, the AN is
>>>> disabled, but this is probably not the best to do in general.
>>> Yes, not a problem in my case.
>>>
>>>> And also you do:
>>>> ---
>>>>
>>>> -		err = of_phy_register_fixed_link(mac_node);
>>>> -		if (err)
>>>> +		struct phy_device *phy;
>>>> +
>>>> +		mac_dev->fixed_link = kzalloc(sizeof(*mac_dev-
>>>>> fixed_link),
>>>> +					      GFP_KERNEL);
>>>> +		if (of_phy_parse_fixed_link(mac_node, mac_dev-
>>>>> fixed_link))
>>>> +			goto _return_dev_set_drvdata;
>>>> +
>>>> +		phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link,
>>>> +					 mac_node);
>>>>
>>>> ---
>>>>
>>>> which means you really want to circumvent the current OF
>>>> api quite a lot, without saying why in the patch description.
>>> I circumvent the API because I din not want to change existing API.
>>> If I could get a reference to the status struct without changing any code
>>> or without being required to call by myself fixed_phy_register(), I
>>> would of done that. Given the existing code in
>> of_phy_register_fixed_link(),
>>> this was my only option. I could have broken of_phy_register_fixed_link()
>>> in two functions:
>>>
>>> of_phy_parse_fixed_link() and of_phy_register_fixed_link(), the latter
>> doing only
>>> the call to fixed_phy_register()
>>>
>>> that would allow to keep of_phy_register_fixed_link() as it is, broken in
>> two stages:
>>> - parsing
>>> - registering
>>>
>>> than can be used by other drivers in order to get the status but I think it's
>> overkill.
>> What I referred to as "circumventing an API" is that you do
>> phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link, + mac_node);
>> by hands, instead of letting the of_phy_register_fixed_link() doing so.
>>
>> How about a smaller circumvention, like this for instance:
>> ---
>> err = of_phy_register_fixed_link(mac_node);
>> phy = of_phy_find_device(dn);
>> status = fixed_phy_get_link_status(phy);    // no such func, to be coded up
>> ---
>>
>> Or even like this:
>> ---
>> err = of_phy_register_fixed_link(mac_node);
>> phy = of_phy_find_device(dn);
>> set_speed_and_duplex(phy->speed, phy->duplex);    // not sure if these
>> values are available that early
>> ---
> After my patch, all that of_phy_register_fixed_link() does is to call
> the new parsing function I introduced then register the fixed PHY.
> I could have done this (pseudocode):
>
> - add of_phy_parse_fixed_link() as seen in the patch
> - add of_phy_register_fixed_phy() that just calls fixed_phy_register():
>
> int of_phy_register_fixed_phy(node)
> {
> 	phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link,
> 				     mac_node);
> 	return (!phy);
> }
>
> - change of_phy_register_fixed_link() to contain only this:
>
> int of_phy_register_fixed_link(node)
> {
> 	of_phy_parse_fixed_link(node, &status);
>
> 	return of_phy_register_fixed_phy(node);
> }
But have you looked into the patch I pointed previously?
https://lkml.org/lkml/2015/7/20/711
You code will likely clash with it because my patch extends
of_phy_register_fixed_link().

> Then I could call only of_* functions but the end result would be the same and
> of_phy_register_fixed_phy() would not justify its existence that much...
You didn't say you wanted to obsolete the of_phy_register_fixed_phy().
Since it is there (and even changed by me in a way your
patch will likely clash), IMHO it would be better if it is used,
rather than copy/pasted into the driver.

> The getter for status you suggest would be fine, but not sure how one would retrieve
> it from the mac_node unless we change of_phy_register_fixed_link() to return the
> pointer to phy (and all the drivers that use it...).
If you look for instance to mvneta.c, you'll find the following:
---
err = of_phy_register_fixed_link(dn);
/* In the case of a fixed PHY, the DT node associated
  * to the PHY is the Ethernet MAC DT node.
  */
  phy_node = of_node_get(dn);
...
phy = of_phy_find_device(dn);
---

So the answer is: just use the same mac_node for both.

>> Also I meant the description should have been in the patch,
>> not in the e-mail. :) You only wrote _what_ the patch does
>> (which is of course obvious from the code itself), but not
>> _why_ and _what was fixed_ (what didn't work).
>>
> If you refer to the first, separation patch, I thought the description was enough:
>
>      of: separate fixed link parsing from registration
>      
>      Some drivers may need
"may need"? I don't understand.
If it is a fix, then they _do need_, and in this case it should
be specified what was broken and what is fixed.
If it is just a clean-up, then "may need" may suffice, but it
was not mentioned it is a clean-up. So I still don't know what
this patch is all about.
"Some drivers" - which ones? The ones that are limited to
the purely fixed links, and never support AN or MDIO?
Or some other drivers too?
So really, the description sounds very cryptic to me.

>   to parse the fixed link values before registering
>      the fixed link phy. Separate the parsing from the actual registration
>      and provide an export for the added parsing function.
>      
>      Signed-off-by: Madalin Bucur <madalin.bucur@freescale.com>
>
> For this one it was a bit brief, I admit - the longer version would be that before it
> we were not using from fixed link anything else but the fact the link was fixed
> (ignored actual speed, duplex values there)
And what didn't work as the result?

>   and this patch tries to fix that.
What started to work after that patch that didn't without it?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1206216

PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBTdGFzIFNlcmdlZXYgW21haWx0
bzpzdHNwQGxpc3QucnVdDQoNCjxzbmlwPg0KPiBCdXQgaGF2ZSB5b3UgbG9va2VkIGludG8gdGhl
IHBhdGNoIEkgcG9pbnRlZCBwcmV2aW91c2x5Pw0KPiBodHRwczovL2xrbWwub3JnL2xrbWwvMjAx
NS83LzIwLzcxMQ0KPiBZb3UgY29kZSB3aWxsIGxpa2VseSBjbGFzaCB3aXRoIGl0IGJlY2F1c2Ug
bXkgcGF0Y2ggZXh0ZW5kcw0KPiBvZl9waHlfcmVnaXN0ZXJfZml4ZWRfbGluaygpLg0KPiANCg0K
SSBhZG1pbiBJIGZhaWxlZCB0byBncmFzcCB0aGUgZGV0YWlscyBvZiB5b3VyIGNoYW5nZSAtIHRo
ZSBsYWNrIG9mIGFtcGxlIGNvbnRleHQNCkxpbmVzIG1ha2VzIGl0IGEgYml0IGRpZmZpY3VsdC4g
SSdtIHN1cmUgeW91ciBjaGFuZ2UgY291bGQgYmUgbWVyZ2VkIHRoZW4gdGhlDQpvZiBwYXJzaW5n
IGNvdWxkIGJlIHNlcGFyYXRlZCBmcm9tIHRoZSBhY3R1YWwgZml4ZWRfcGh5X3JlZ2lzdGVyKCkg
Y2FsbCBpZiBhbnlvbmUNCmNhcmVzIGFib3V0IHRoYXQuDQoNCj4gPiBUaGVuIEkgY291bGQgY2Fs
bCBvbmx5IG9mXyogZnVuY3Rpb25zIGJ1dCB0aGUgZW5kIHJlc3VsdCB3b3VsZCBiZSB0aGUgc2Ft
ZQ0KPiA+IGFuZCAgb2ZfcGh5X3JlZ2lzdGVyX2ZpeGVkX3BoeSgpIHdvdWxkIG5vdCBqdXN0aWZ5
IGl0cyBleGlzdGVuY2UgdGhhdCBtdWNoLi4uDQo+IFlvdSBkaWRuJ3Qgc2F5IHlvdSB3YW50ZWQg
dG8gb2Jzb2xldGUgdGhlIG9mX3BoeV9yZWdpc3Rlcl9maXhlZF9waHkoKS4NCj4gU2luY2UgaXQg
aXMgdGhlcmUgKGFuZCBldmVuIGNoYW5nZWQgYnkgbWUgaW4gYSB3YXkgeW91cg0KPiBwYXRjaCB3
aWxsIGxpa2VseSBjbGFzaCksIElNSE8gaXQgd291bGQgYmUgYmV0dGVyIGlmIGl0IGlzIHVzZWQs
DQo+IHJhdGhlciB0aGFuIGNvcHkvcGFzdGVkIGludG8gdGhlIGRyaXZlci4NCg0KUGxlYXNlIG5v
dGUgSSB3YXMgcmVmZXJyaW5nIHRvIGEgZmljdGlvbmFsIG5ldyBmdW5jdGlvbiB0aGF0IHdvdWxk
IGVtYmVkIHRoZSBjYWxsIHRvIA0KZml4ZWRfcGh5X3JlZ2lzdGVyKCkuIEkgd2FzIG5vdCB0YWxr
aW5nIGFib3V0IHNvbWUgZXhpc3RpbmcgQVBJLCBqdXN0IGFib3V0IGEgbmV3IA0Kb2ZfY2FsbCAg
bmFtZWQgb2ZfcGh5X3JlZ2lzdGVyX2ZpeGVkX3BoeSgpICB0aGF0IHdvdWxkIGluIHRoZSBlbmQg
YmUgY2FsbGVkIGJ5DQpvZl9waHlfcmVnaXN0ZXJfZml4ZWRfbGluaygpIGFuZCBieSBzb21lIGRy
aXZlcnMgdGhhdCB3YW50IHRvIGdldCBpbiB0aGUgbWlkZGxlDQpvZiB0aGluZ3MgYW5kIGdldCBh
IGhvbGQgb24gc3RhdHVzLg0KDQpNYXliZSB0aGUgZmFjdCB3ZSdyZSByZXZpZXdpbmcgdHdvIHBh
dGNoZXMgaW4gb25lIHRocmVhZCBpcyB3aGF0IG1ha2VzIHRoZQ0KZGlzY3Vzc2lvbiBsZXNzIHRo
YW4gb3B0aW1hbC4NCg0KPiA+IFRoZSBnZXR0ZXIgZm9yIHN0YXR1cyB5b3Ugc3VnZ2VzdCB3b3Vs
ZCBiZSBmaW5lLCBidXQgbm90IHN1cmUgaG93IG9uZQ0KPiA+IHdvdWxkIHJldHJpZXZlDQo+ID4g
aXQgZnJvbSB0aGUgbWFjX25vZGUgdW5sZXNzIHdlIGNoYW5nZSBvZl9waHlfcmVnaXN0ZXJfZml4
ZWRfbGluaygpIHRvDQo+ID4gcmV0dXJuIHRoZQ0KPiA+IHBvaW50ZXIgdG8gcGh5IChhbmQgYWxs
IHRoZSBkcml2ZXJzIHRoYXQgdXNlIGl0Li4uKS4NCj4gSWYgeW91IGxvb2sgZm9yIGluc3RhbmNl
IHRvIG12bmV0YS5jLCB5b3UnbGwgZmluZCB0aGUgZm9sbG93aW5nOg0KPiAtLS0NCj4gZXJyID0g
b2ZfcGh5X3JlZ2lzdGVyX2ZpeGVkX2xpbmsoZG4pOw0KPiAvKiBJbiB0aGUgY2FzZSBvZiBhIGZp
eGVkIFBIWSwgdGhlIERUIG5vZGUgYXNzb2NpYXRlZA0KPiAgICogdG8gdGhlIFBIWSBpcyB0aGUg
RXRoZXJuZXQgTUFDIERUIG5vZGUuDQo+ICAgKi8NCj4gICBwaHlfbm9kZSA9IG9mX25vZGVfZ2V0
KGRuKTsNCj4gLi4uDQo+IHBoeSA9IG9mX3BoeV9maW5kX2RldmljZShkbik7DQo+IC0tLQ0KPiAN
Cj4gU28gdGhlIGFuc3dlciBpczoganVzdCB1c2UgdGhlIHNhbWUgbWFjX25vZGUgZm9yIGJvdGgu
DQoNCkkgdW5kZXJzdGFuZCwgSSdsbCB1c2UgdGhpcyBhcHByb2FjaCBhbHRob3VnaCBpcyBzdWJv
cHRpbWFsIGltaG8gdG8NCnNjYW4gdGhlIGRldmljZSB0cmVlIGFnYWluIHRvIGdldCBhIHBoeSBw
b2ludGVyIHRoYXQgeW91IG5lZWQganVzdA0KdG8gZ2V0IHNvbWUgb2YgaW5mbyB0aGF0IHdhcyBw
YXJzZWQgaW4gYSBjYWxsIHlvdSBqdXN0IG1hZGUuDQoNCj4gPj4gQWxzbyBJIG1lYW50IHRoZSBk
ZXNjcmlwdGlvbiBzaG91bGQgaGF2ZSBiZWVuIGluIHRoZSBwYXRjaCwNCj4gPj4gbm90IGluIHRo
ZSBlLW1haWwuIDopIFlvdSBvbmx5IHdyb3RlIF93aGF0XyB0aGUgcGF0Y2ggZG9lcw0KPiA+PiAo
d2hpY2ggaXMgb2YgY291cnNlIG9idmlvdXMgZnJvbSB0aGUgY29kZSBpdHNlbGYpLCBidXQgbm90
DQo+ID4+IF93aHlfIGFuZCBfd2hhdCB3YXMgZml4ZWRfICh3aGF0IGRpZG4ndCB3b3JrKS4NCj4g
Pj4NCj4gPiBJZiB5b3UgcmVmZXIgdG8gdGhlIGZpcnN0LCBzZXBhcmF0aW9uIHBhdGNoLCBJIHRo
b3VnaHQgdGhlIGRlc2NyaXB0aW9uIHdhcw0KPiBlbm91Z2g6DQo+ID4NCj4gPiAgICAgIG9mOiBz
ZXBhcmF0ZSBmaXhlZCBsaW5rIHBhcnNpbmcgZnJvbSByZWdpc3RyYXRpb24NCj4gPg0KPiA+ICAg
ICAgU29tZSBkcml2ZXJzIG1heSBuZWVkDQo+ICJtYXkgbmVlZCI/IEkgZG9uJ3QgdW5kZXJzdGFu
ZC4NCj4gSWYgaXQgaXMgYSBmaXgsIHRoZW4gdGhleSBfZG8gbmVlZF8sIGFuZCBpbiB0aGlzIGNh
c2UgaXQgc2hvdWxkDQo+IGJlIHNwZWNpZmllZCB3aGF0IHdhcyBicm9rZW4gYW5kIHdoYXQgaXMg
Zml4ZWQuDQo+IElmIGl0IGlzIGp1c3QgYSBjbGVhbi11cCwgdGhlbiAibWF5IG5lZWQiIG1heSBz
dWZmaWNlLCBidXQgaXQNCj4gd2FzIG5vdCBtZW50aW9uZWQgaXQgaXMgYSBjbGVhbi11cC4gU28g
SSBzdGlsbCBkb24ndCBrbm93IHdoYXQNCj4gdGhpcyBwYXRjaCBpcyBhbGwgYWJvdXQuDQo+ICJT
b21lIGRyaXZlcnMiIC0gd2hpY2ggb25lcz8gVGhlIG9uZXMgdGhhdCBhcmUgbGltaXRlZCB0bw0K
PiB0aGUgcHVyZWx5IGZpeGVkIGxpbmtzLCBhbmQgbmV2ZXIgc3VwcG9ydCBBTiBvciBNRElPPw0K
PiBPciBzb21lIG90aGVyIGRyaXZlcnMgdG9vPw0KPiBTbyByZWFsbHksIHRoZSBkZXNjcmlwdGlv
biBzb3VuZHMgdmVyeSBjcnlwdGljIHRvIG1lLg0KDQpNaW5lLCB3aGVuIHRoZXJlIGlzIGEgZml4
ZWQgbGluayBub2RlLCBtYXliZSBvdGhlcnMuIFdoZW4gdGhlcmUgaXNuJ3QgYW55DQpmaXhlZCBs
aW5rIG5vZGUsIHRoZSBpbnRlcm5hbCBQSFkgY29uZmlnIGRlZmF1bHRzIHRvIDFHIGZ1bGwgZHVw
bGV4IEFOIGVuYWJsZWQNCmFuZCBhZGp1c3QgbGluayB0YWtlcyBjYXJlIG9mIHRoaW5ncy4NCg0K
PiANCj4gPiAgIHRvIHBhcnNlIHRoZSBmaXhlZCBsaW5rIHZhbHVlcyBiZWZvcmUgcmVnaXN0ZXJp
bmcNCj4gPiAgICAgIHRoZSBmaXhlZCBsaW5rIHBoeS4gU2VwYXJhdGUgdGhlIHBhcnNpbmcgZnJv
bSB0aGUgYWN0dWFsIHJlZ2lzdHJhdGlvbg0KPiA+ICAgICAgYW5kIHByb3ZpZGUgYW4gZXhwb3J0
IGZvciB0aGUgYWRkZWQgcGFyc2luZyBmdW5jdGlvbi4NCj4gPg0KPiA+ICAgICAgU2lnbmVkLW9m
Zi1ieTogTWFkYWxpbiBCdWN1ciA8bWFkYWxpbi5idWN1ckBmcmVlc2NhbGUuY29tPg0KPiA+DQo+
ID4gRm9yIHRoaXMgb25lIGl0IHdhcyBhIGJpdCBicmllZiwgSSBhZG1pdCAtIHRoZSBsb25nZXIg
dmVyc2lvbiB3b3VsZCBiZSB0aGF0DQo+IGJlZm9yZSBpdA0KPiA+IHdlIHdlcmUgbm90IHVzaW5n
IGZyb20gZml4ZWQgbGluayBhbnl0aGluZyBlbHNlIGJ1dCB0aGUgZmFjdCB0aGUgbGluayB3YXMN
Cj4gZml4ZWQNCj4gPiAoaWdub3JlZCBhY3R1YWwgc3BlZWQsIGR1cGxleCB2YWx1ZXMgdGhlcmUp
DQo+IEFuZCB3aGF0IGRpZG4ndCB3b3JrIGFzIHRoZSByZXN1bHQ/DQo+IA0KPiA+ICAgYW5kIHRo
aXMgcGF0Y2ggdHJpZXMgdG8gZml4IHRoYXQuDQo+IFdoYXQgc3RhcnRlZCB0byB3b3JrIGFmdGVy
IHRoYXQgcGF0Y2ggdGhhdCBkaWRuJ3Qgd2l0aG91dCBpdD8NCg0KMTBNIGhhbGYgZHVwbGV4IGZv
ciBpbnN0YW5jZQ0KDQpJJ2QgY2xvc2UgdGhpcyB0aHJlYWQgZm9yIG5vdyBhbmQgdXNlIGluIG15
IGRyaXZlciBvZl9waHlfZmluZF9kZXZpY2UobWFjX25vZGUpLg0KDQpUaGFuayB5b3UsDQpNYWRh
bGluDQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1206234

12.08.2015 18:27, Madalin-Cristian Bucur пишет:
>>> Then I could call only of_* functions but the end result would be the same
>>> and  of_phy_register_fixed_phy() would not justify its existence that much...
>> You didn't say you wanted to obsolete the of_phy_register_fixed_phy().
>> Since it is there (and even changed by me in a way your
>> patch will likely clash), IMHO it would be better if it is used,
>> rather than copy/pasted into the driver.
> Please note I was referring to a fictional new function that would embed the call to
> fixed_phy_register(). I was not talking about some existing API, just about a new
> of_call  named of_phy_register_fixed_phy()  that would in the end be called by
> of_phy_register_fixed_link() and by some drivers that want to get in the middle
> of things and get a hold on status.
Hmm, and for exactly unknown reason in your pseudocode
of_phy_register_fixed_phy() doesn't take status as an argument. :)
So I didn't see its point.
If you fix your pseudo-code, you'll add the status argument,
because it is needed for fixed_phy_register() anyway.
After that, the drivers that want to provide the status, will
just use it rather than to call fixed_phy_register() directly.
And with my changes it really have even more merits to exist.

> Maybe the fact we're reviewing two patches in one thread is what makes the
> discussion less than optimal.
I guess the bugs in the pseudo-code made me to miss its point.

>>> The getter for status you suggest would be fine, but not sure how one
>>> would retrieve
>>> it from the mac_node unless we change of_phy_register_fixed_link() to
>>> return the
>>> pointer to phy (and all the drivers that use it...).
>> If you look for instance to mvneta.c, you'll find the following:
>> ---
>> err = of_phy_register_fixed_link(dn);
>> /* In the case of a fixed PHY, the DT node associated
>>    * to the PHY is the Ethernet MAC DT node.
>>    */
>>    phy_node = of_node_get(dn);
>> ...
>> phy = of_phy_find_device(dn);
>> ---
>>
>> So the answer is: just use the same mac_node for both.
> I understand, I'll use this approach although is suboptimal imho to
Exactly!
But at least this way is used in many currently existing
drivers, while getting the fixed-link parameters directly from
DT - is a new way of circumventing the existing API.
So I'd vote for the currently existing hacks, and in fact
I already tried to start a discussion about getting rid of the
need for of_phy_find_device(), but it didn't go.

> scan the device tree again to get a phy pointer that you need just
> to get some of info that was parsed in a call you just made.
Maybe (just maybe) of_phy_register_fixed_link() could
return the phy_device pointer. At least it will solve your
problem very cheaply. But I am sure such API additions
require a separate discussion, can't be done in a context
of discussing a small fix. If you have some free time, feel
free to raise such a discussion with API extension proposals.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web