| Message ID | 20231205103559.9605-6-fancer.lancer@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: pcs: xpcs: Add memory-based management iface support | expand |
On Tue, Dec 05, 2023 at 01:35:26PM +0300, Serge Semin wrote: > Generic MDIO-device driver will support setting a custom device ID for the > particular MDIO-device. Why future tense? I don't see anything later in this patch set adding this.
On Tue, Dec 05, 2023 at 10:45:34AM +0000, Russell King (Oracle) wrote: > On Tue, Dec 05, 2023 at 01:35:26PM +0300, Serge Semin wrote: > > Generic MDIO-device driver will support setting a custom device ID for the > > particular MDIO-device. > > Why future tense? I don't see anything later in this patch set adding > this. After the next patch is applied [PATCH net-next 10/16] net: pcs: xpcs: Add generic DW XPCS MDIO-device support the DW XPCS driver _will_ support setting custom IDs based on the platform data and the DT compatibles. It can be used for instance to fix the already available SJ1105 and SJ1110 MDIO bus implementations, so instead of substituting the XPCS IDs on the PHYSID CSR reads the driver could just pass the device ID and PMA ID via the device platform data. If my patch log text looks unclear anyway, just say so. I'll change it accordingly. I guess it would be enough to say that moving is required just to collect all the IDs in a single place. -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Dec 05, 2023 at 02:14:34PM +0300, Serge Semin wrote: > On Tue, Dec 05, 2023 at 10:45:34AM +0000, Russell King (Oracle) wrote: > > On Tue, Dec 05, 2023 at 01:35:26PM +0300, Serge Semin wrote: > > > Generic MDIO-device driver will support setting a custom device ID for the > > > particular MDIO-device. > > > > Why future tense? I don't see anything later in this patch set adding > > this. > > After the next patch is applied > [PATCH net-next 10/16] net: pcs: xpcs: Add generic DW XPCS MDIO-device support > the DW XPCS driver _will_ support setting custom IDs based on the > platform data and the DT compatibles. What is confusing is that the sentence makes it sound like it's some generic driver that can be used for any PCS, whereas in reality it is _this_ XPCS driver which is not generic. "This driver will support setting a custom device ID in a future patch." or explicitly state the summary line of the patch concerned so one can refer to it. Future references are difficult to find whether they're in email and especially once they're merged into git. > It can be used for instance to > fix the already available SJ1105 and SJ1110 MDIO bus implementations, > so instead of substituting the XPCS IDs on the PHYSID CSR reads the > driver could just pass the device ID and PMA ID via the device > platform data. > > If my patch log text looks unclear anyway, just say so. I'll change it > accordingly. I guess it would be enough to say that moving is required > just to collect all the IDs in a single place. You need to adjust your attitude - I did exactly that. There was something which I didn't understand, so I raised the issue. Sorry for spotting a problem, but do you always get arsey when a reviewer picks up on something wrong? If that's your attitude, then for this entire series: NAK.
On Tue, Dec 05, 2023 at 01:35:26PM +0300, Serge Semin wrote: > In addition to that having all supported DW XPCS device IDs defined in > a sinle place will improve the code maintainability and readability. single
On Tue, Dec 05, 2023 at 11:22:50AM +0000, Russell King (Oracle) wrote: > On Tue, Dec 05, 2023 at 02:14:34PM +0300, Serge Semin wrote: > > On Tue, Dec 05, 2023 at 10:45:34AM +0000, Russell King (Oracle) wrote: > > > On Tue, Dec 05, 2023 at 01:35:26PM +0300, Serge Semin wrote: > > > > Generic MDIO-device driver will support setting a custom device ID for the > > > > particular MDIO-device. > > > > > > Why future tense? I don't see anything later in this patch set adding > > > this. > > > > After the next patch is applied > > [PATCH net-next 10/16] net: pcs: xpcs: Add generic DW XPCS MDIO-device support > > the DW XPCS driver _will_ support setting custom IDs based on the > > platform data and the DT compatibles. > > What is confusing is that the sentence makes it sound like it's some > generic driver that can be used for any PCS, whereas in reality it is > _this_ XPCS driver which is not generic. > > "This driver will support setting a custom device ID in a future patch." > or explicitly state the summary line of the patch concerned so one can > refer to it. Future references are difficult to find whether they're in > email and especially once they're merged into git. Ok. I'll convert the patch log to be less confusing. As I already said to Vladimir writing sometimes overcomplicated messages my eternal problem. > > > It can be used for instance to > > fix the already available SJ1105 and SJ1110 MDIO bus implementations, > > so instead of substituting the XPCS IDs on the PHYSID CSR reads the > > driver could just pass the device ID and PMA ID via the device > > platform data. > > > > If my patch log text looks unclear anyway, just say so. I'll change it > > accordingly. I guess it would be enough to say that moving is required > > just to collect all the IDs in a single place. > > You need to adjust your attitude - I did exactly that. There was > something which I didn't understand, so I raised the issue. Sorry > for spotting a problem, but do you always get arsey when a reviewer > picks up on something wrong? If that's your attitude, then for this > entire series: NAK. I'm sorry if what I wrote sounded like I was arsey. I didn't mean it at all, really. By this sentence: > I guess it would be enough to say that moving is required > just to collect all the IDs in a single place. I meant that _I_ should have just stated in the log message that moving was required to collect all the IDs in a single place. The rest of the text was redundant and caused confusion what you pointed out to. -Serge(y) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Dec 05, 2023 at 01:27:55PM +0200, Vladimir Oltean wrote: > On Tue, Dec 05, 2023 at 01:35:26PM +0300, Serge Semin wrote: > > In addition to that having all supported DW XPCS device IDs defined in > > a sinle place will improve the code maintainability and readability. > > single Got it. Thanks. Seeing there are so many comments about the log messages I'll double check them on v2. -Serge(y)
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c index 46afeb5510c0..2850122f354a 100644 --- a/drivers/net/pcs/pcs-xpcs.c +++ b/drivers/net/pcs/pcs-xpcs.c @@ -1343,16 +1343,16 @@ static const struct xpcs_compat nxp_sja1110_xpcs_compat[DW_XPCS_INTERFACE_MAX] = static const struct xpcs_id xpcs_id_list[] = { { - .id = SYNOPSYS_XPCS_ID, - .mask = SYNOPSYS_XPCS_MASK, + .id = DW_XPCS_ID, + .mask = DW_XPCS_ID_MASK, .compat = synopsys_xpcs_compat, }, { .id = NXP_SJA1105_XPCS_ID, - .mask = SYNOPSYS_XPCS_MASK, + .mask = DW_XPCS_ID_MASK, .compat = nxp_sja1105_xpcs_compat, }, { .id = NXP_SJA1110_XPCS_ID, - .mask = SYNOPSYS_XPCS_MASK, + .mask = DW_XPCS_ID_MASK, .compat = nxp_sja1110_xpcs_compat, }, }; diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h index 96c36b32ca99..369e9196f45a 100644 --- a/drivers/net/pcs/pcs-xpcs.h +++ b/drivers/net/pcs/pcs-xpcs.h @@ -6,9 +6,6 @@ * Author: Jose Abreu <Jose.Abreu@synopsys.com> */ -#define SYNOPSYS_XPCS_ID 0x7996ced0 -#define SYNOPSYS_XPCS_MASK 0xffffffff - /* Vendor regs access */ #define DW_VENDOR BIT(15) diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h index da3a6c30f6d2..8dfe90295f12 100644 --- a/include/linux/pcs/pcs-xpcs.h +++ b/include/linux/pcs/pcs-xpcs.h @@ -12,6 +12,8 @@ #define NXP_SJA1105_XPCS_ID 0x00000010 #define NXP_SJA1110_XPCS_ID 0x00000020 +#define DW_XPCS_ID 0x7996ced0 +#define DW_XPCS_ID_MASK 0xffffffff /* AN mode */ #define DW_AN_C73 1
Generic MDIO-device driver will support setting a custom device ID for the particular MDIO-device. Generic DW XPCS ID can be used for that as well just in case if the DW XPCS-device was erroneously synthesized with no or some undefined ID. In addition to that having all supported DW XPCS device IDs defined in a sinle place will improve the code maintainability and readability. Note while at it rename the macros to be shorter and looking alike to the already defined NXP XPCS ID macro. Signed-off-by: Serge Semin <fancer.lancer@gmail.com> --- drivers/net/pcs/pcs-xpcs.c | 8 ++++---- drivers/net/pcs/pcs-xpcs.h | 3 --- include/linux/pcs/pcs-xpcs.h | 2 ++ 3 files changed, 6 insertions(+), 7 deletions(-)