From: "Csókás Bence" <csokas.bence@prolan.hu>
To: Andrew Lunn <andrew@lunn.ch>
Cc: <netdev@vger.kernel.org>,
Richard Cochran <richardcochran@gmail.com>,
Fugang Duan <fugang.duan@nxp.com>
Subject: Re: [PATCH] fec: Restart PPS after link state change
Date: Thu, 11 Aug 2022 16:45:14 +0200 [thread overview]
Message-ID: <91d6794e-9e22-2a7c-b40a-436da63c78b5@prolan.hu> (raw)
In-Reply-To: <YvUEgKl6fpHwMwuS@lunn.ch>
On 2022. 08. 11. 15:30, Andrew Lunn wrote:
>> `fep->pps_enable` is the state of the PPS the driver *believes* to be the
>> case. After a reset, this belief may or may not be true anymore: if the
>> driver believed formerly that the PPS is down, then after a reset, its
>> belief will still be correct, thus nothing needs to be done about the
>> situation. If, however, the driver thought that PPS was up, after controller
>> reset, it no longer holds, so we need to update our world-view
>> (`fep->pps_enable = 0;`), and then correct for the fact that PPS just
>> unexpectedly stopped.
>
> Your way of doing it just seems very unclean. I would make
> fec_ptp_enable_pps() read the actual status from the
> hardware. fep->pps_enable then has the clear meaning of user space
> requested it should be enabled.
1. It is not "my way", it is how it was in the original code. I am
merely following those who came before me.
2. There is already a variable which holds userspace's wish: parameter
`uint enable` in `fec_ptp_enable_pps()`. `fep->pps_enable` is whether
the driver already fulfilled this wish.
>
> Andrew
Honestly, I would rather see the entire `fec` driver re-written from
scratch, it is really bad code and full of bugs. Plus, Fugang Duan's
mail server keeps bouncing back all my emails (I can only hope he sees
these mails through the mailing list). However, that exceeds my
capabilities unfortunately (I know not nearly enough of the various
fec-based controllers and their internals, I only have the i.MX6 TX6UL
to test). So the best I can do is provide fixes to the bugs we
experienced, while changing as little of the original driver's code as
possible, so as to (hopefully) not introduce even more bugs.
Bence
next prev parent reply other threads:[~2022-08-11 14:45 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-09 12:41 [PATCH] fec: Restart PPS after link state change Csókás Bence
2022-08-09 17:28 ` Andrew Lunn
2022-08-10 11:36 ` Csókás Bence
2022-08-11 0:05 ` Andrew Lunn
2022-08-11 1:37 ` Richard Cochran
2022-08-11 2:05 ` Andrew Lunn
2022-08-11 8:07 ` Csókás Bence
2022-08-11 16:38 ` Richard Cochran
2022-08-11 9:23 ` Csókás Bence
2022-08-11 13:30 ` Andrew Lunn
2022-08-11 14:45 ` Csókás Bence [this message]
-- strict thread matches above, loose matches on Subject: below --
2022-08-10 14:00 Csókás Bence
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=91d6794e-9e22-2a7c-b40a-436da63c78b5@prolan.hu \
--to=csokas.bence@prolan.hu \
--cc=andrew@lunn.ch \
--cc=fugang.duan@nxp.com \
--cc=netdev@vger.kernel.org \
--cc=richardcochran@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.