| Message ID | 20201122082636.12451-3-ceggers@arri.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: ptp: use common defines for PTP message types in further drivers | expand |
On Sun, Nov 22, 2020 at 09:26:35AM +0100, Christian Eggers wrote: > Use recently introduced PTP wide defines instead of a driver internal > enumeration. > > Signed-off-by: Christian Eggers <ceggers@gmx.de> > Cc: Petr Machata <petrm@mellanox.com> > Cc: Jiri Pirko <jiri@nvidia.com> > Cc: Ido Schimmel <idosch@nvidia.com> Reviewed-by: Ido Schimmel <idosch@nvidia.com> But: 1. Checkpatch complains about: WARNING: From:/Signed-off-by: email address mismatch: 'From: Christian Eggers <ceggers@arri.de>' != 'Signed-off-by: Christian Eggers <ceggers@gmx.de>' 2. This series does not build, which fails the CI [1][2] and also required me to fetch the dependencies that are currently under review [3]. I believe it is generally discouraged to create dependencies between patch sets that are under review for exactly these reasons. I don't know what are Jakub's preferences, but had this happened on our internal patchwork instance, I would just ask the author to submit another version with all the patches. Anyway, I added all six patches to our regression as we have some PTP tests. Will let you know tomorrow. Thanks [1] https://lore.kernel.org/netdev/20201122082636.12451-1-ceggers@arri.de/T/#mcef35858585d23b72b8f75450a51618d5c5d3260 [2] https://patchwork.hopto.org/static/nipa/389053/11923809/build_allmodconfig_warn/summary [3] https://patchwork.kernel.org/project/netdevbpf/cover/20201120084106.10046-1-ceggers@arri.de/
On Sunday, 22 November 2020, 15:35:55 CET, Ido Schimmel wrote: > On Sun, Nov 22, 2020 at 09:26:35AM +0100, Christian Eggers wrote: > > Use recently introduced PTP wide defines instead of a driver internal > > enumeration. > > > > Signed-off-by: Christian Eggers <ceggers@gmx.de> > > Cc: Petr Machata <petrm@mellanox.com> > > Cc: Jiri Pirko <jiri@nvidia.com> > > Cc: Ido Schimmel <idosch@nvidia.com> > > Reviewed-by: Ido Schimmel <idosch@nvidia.com> > > But: > > 1. Checkpatch complains about: > WARNING: From:/Signed-off-by: email address mismatch: 'From: Christian > Eggers <ceggers@arri.de>' != 'Signed-off-by: Christian Eggers > <ceggers@gmx.de>' unfortunately I changed this after running checkpatch. My intention was to separate my (private) weekend work from the patches I do while I'm on the job. > 2. This series does not build, which fails the CI [1][2] and also > required me to fetch the dependencies that are currently under review > [3]. I believe it is generally discouraged to create dependencies > between patch sets that are under review for exactly these reasons. this was also not by intention. Vladimir found some files I missed in the first series. As the whole first series had already been reviewed at that time, I wasn't sure whether I am allowed to add further patches to it. Additionally I didn't concern that although my local build is successful, I should wait until the first series is applied... > I don't know what are Jakub's preferences, but had this happened on our > internal patchwork instance, I would just ask the author to submit > another version with all the patches. Please let me know how I shall proceed... > Anyway, I added all six patches to our regression as we have some PTP > tests. Will let you know tomorrow. > > Thanks > > [1] > https://lore.kernel.org/netdev/20201122082636.12451-1-ceggers@arri.de/T/#mc > ef35858585d23b72b8f75450a51618d5c5d3260 [2] > https://patchwork.hopto.org/static/nipa/389053/11923809/build_allmodconfig_ > warn/summary [3] > https://patchwork.kernel.org/project/netdevbpf/cover/20201120084106.10046-1 > -ceggers@arri.de/
On Sun, Nov 22, 2020 at 08:29:22PM +0100, Christian Eggers wrote: > On Sunday, 22 November 2020, 15:35:55 CET, Ido Schimmel wrote: > > On Sun, Nov 22, 2020 at 09:26:35AM +0100, Christian Eggers wrote: > > > Use recently introduced PTP wide defines instead of a driver internal > > > enumeration. > > > > > > Signed-off-by: Christian Eggers <ceggers@gmx.de> > > > Cc: Petr Machata <petrm@mellanox.com> > > > Cc: Jiri Pirko <jiri@nvidia.com> > > > Cc: Ido Schimmel <idosch@nvidia.com> > > > > Reviewed-by: Ido Schimmel <idosch@nvidia.com> > > > > But: > > > > 1. Checkpatch complains about: > > WARNING: From:/Signed-off-by: email address mismatch: 'From: Christian > > Eggers <ceggers@arri.de>' != 'Signed-off-by: Christian Eggers > > <ceggers@gmx.de>' > unfortunately I changed this after running checkpatch. My intention was to > separate my (private) weekend work from the patches I do while I'm on the job. No problem. Just make sure that authorship and Signed-off-by agree. You can use: # git commit --amend --author="Christian Eggers <ceggers@gmx.de>" > > > 2. This series does not build, which fails the CI [1][2] and also > > required me to fetch the dependencies that are currently under review > > [3]. I believe it is generally discouraged to create dependencies > > between patch sets that are under review for exactly these reasons. > this was also not by intention. Vladimir found some files I missed in the > first series. As the whole first series had already been reviewed at that time, > I wasn't sure whether I am allowed to add further patches to it. Additionally > I didn't concern that although my local build is successful, I should wait > until the first series is applied... Yea, I saw that, no problem :) > > > I don't know what are Jakub's preferences, but had this happened on our > > internal patchwork instance, I would just ask the author to submit > > another version with all the patches. > Please let me know how I shall proceed... Jakub has the final say, so I assume he will comment on that. Regardless, thanks for the patches.
On Sun, Nov 22, 2020 at 08:29:22PM +0100, Christian Eggers wrote: > this was also not by intention. Vladimir found some files I missed in the > first series. As the whole first series had already been reviewed at that time, > I wasn't sure whether I am allowed to add further patches to it. Additionally > I didn't concern that although my local build is successful, I should wait > until the first series is applied... When I said that, what I was thinking of (although it might have not been clear) was that if you send further patches, you send them _after_ the initial series is merged. Alternatively, it would have been just as valid to resend the entire series, as a 3+3 patchset with a new revision (v3 -> v4), with review tags collected from the first three, and the last 3 being completely new. Jakub could have superseded v3 and applied v4. The idea behind splicing N patches into a series is that they are logically connected to one another. For example, a patch doesn't build without another. You _could_ split logically-connected patches into separate series and post them independently for review, as long as they are build-time independent. If they aren't, well, what happens is exactly what happened: various test robots will report build failures, which from a maintainer's point of view inspires less confidence to apply a patch as-is. I would not be surprised if Jakub asked you to resend with no change at all, just to ensure that the patch series passes build tests before merging it.
On Sun, Nov 22, 2020 at 04:35:58PM +0200, Ido Schimmel wrote: > Anyway, I added all six patches to our regression as we have some PTP > tests. Will let you know tomorrow. Looks good
On Sun, 22 Nov 2020 22:01:54 +0200 Ido Schimmel wrote: > > > I don't know what are Jakub's preferences, but had this happened on our > > > internal patchwork instance, I would just ask the author to submit > > > another version with all the patches. > > Please let me know how I shall proceed... > > Jakub has the final say, so I assume he will comment on that. The pre-requisite was just merged, so please collect all the review tags you got so far and repost.
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c index ca8090a28dec..d6e9ecb14681 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c @@ -828,10 +828,10 @@ struct mlxsw_sp_ptp_state *mlxsw_sp1_ptp_init(struct mlxsw_sp *mlxsw_sp) goto err_hashtable_init; /* Delive these message types as PTP0. */ - message_type = BIT(MLXSW_SP_PTP_MESSAGE_TYPE_SYNC) | - BIT(MLXSW_SP_PTP_MESSAGE_TYPE_DELAY_REQ) | - BIT(MLXSW_SP_PTP_MESSAGE_TYPE_PDELAY_REQ) | - BIT(MLXSW_SP_PTP_MESSAGE_TYPE_PDELAY_RESP); + message_type = BIT(PTP_MSGTYPE_SYNC) | + BIT(PTP_MSGTYPE_DELAY_REQ) | + BIT(PTP_MSGTYPE_PDELAY_REQ) | + BIT(PTP_MSGTYPE_PDELAY_RESP); err = mlxsw_sp_ptp_mtptpt_set(mlxsw_sp, MLXSW_REG_MTPTPT_TRAP_ID_PTP0, message_type); if (err) diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h index 8c386571afce..1d43a3755285 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h @@ -11,13 +11,6 @@ struct mlxsw_sp; struct mlxsw_sp_port; struct mlxsw_sp_ptp_clock; -enum { - MLXSW_SP_PTP_MESSAGE_TYPE_SYNC, - MLXSW_SP_PTP_MESSAGE_TYPE_DELAY_REQ, - MLXSW_SP_PTP_MESSAGE_TYPE_PDELAY_REQ, - MLXSW_SP_PTP_MESSAGE_TYPE_PDELAY_RESP, -}; - static inline int mlxsw_sp_ptp_get_ts_info_noptp(struct ethtool_ts_info *info) { info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
Use recently introduced PTP wide defines instead of a driver internal enumeration. Signed-off-by: Christian Eggers <ceggers@gmx.de> Cc: Petr Machata <petrm@mellanox.com> Cc: Jiri Pirko <jiri@nvidia.com> Cc: Ido Schimmel <idosch@nvidia.com> --- drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c | 8 ++++---- drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h | 7 ------- 2 files changed, 4 insertions(+), 11 deletions(-)