On Sat, Dec 04, 2021 at 04:57:56PM +0100, Mathieu Peyrega wrote: > Le vendredi 03 décembre 2021 à 14:11 +0100, Greg KH a écrit : > > On Thu, Dec 02, 2021 at 11:56:10AM +0100, Mathieu Peyrega wrote: > > > Hello, > > > > > > this is my first contribution to this list (and the Linux kernel) > > > and I'd > > > like trying to revive a subject already discussed in a 2017/2018 > > > thread about > > > adding a possibility to use the CTS pin instead of the DCD pin for > > > 1PPS line > > > disciplining (cf. > > > https://www.spinics.net/lists/linux-serial/msg27604.html) > > > > A few meta-comments about the patch. > > > > Look at how other patches are submitted, there's no need for this > > type > > of introduction in the changelog text, and in fact, you have no > > changelog text in your patch at all. So the body of this email needs > > to > > go into the changelog area, please do so when you resend. > > will do > > > > The rationale is similar to the original poster's one: some TTL > > > UARTs hardware > > > implementations have an incomplete wiring and do not expose a DCD > > > pin (e.g. on > > > some SBCs). On those platforms only TX, RX, CTS and RTS are > > > available. In such > > > cases, being able to use the CTS pin for 1PPS time disciplining is > > > useful. > > > > > > In addition, to that primary need, I believe there is another > > > missing feature > > > in current implementation. Some non-RS232 UARTs (e.g. TTL UARTs > > > also often > > > found on SBCs) have an inverted behaviour with respect to RS232 > > > rising edge or > > > falling edge vs. assert or clear logics. Not taking this inversion > > > into account > > > results in a disciplining where the kernel time ends with an offset > > > from actual > > > signal time. Offset value is the width/duration of the 1PPS square > > > pulse signal. > > > At least this is what I experienced in the testing process on an > > > Odroid H2 SBC > > > (Intel x86_64 based) and a GNSS driven 1PPS signal (from a CORS > > > station that I > > > manage). Maybe this can be handled from a later userland process > > > (e.g. ntpd) > > > but I believe that being able to handle it straight at kernel level > > > is better. > > > > > > As for the module behaviour control, I went with adding 2 > > > parameters to > > > pps-ldisc: > > > > > > - activepin (charp) wich can be dcd (default) or cts and drives > > > the pin > > > which should be consider to detect the 1PPS signal > > > - invertlogic (bool) which can be false (default) or true and > > > defines if > > > the driver complies with a RS232 type signal where assert is on > > > the > > > rising edge or inverted as for some TTL UARTs. Default values > > > match > > > the current behaviour. > > > > New module parameters should never be added, as they only affect the > > whole system, and not on a per-device basis. If you have to add new > > options, please do it some other way. > > I don't fully understand the point. Isn't the existing pps_ldisc module > already affecting the whole system ? (with it's builtin fixed > "options"). How different tunable options such as the proposal make > things fundamentally different ? Still I agree that per device settings > would be better. Per device settings are required, this would prevent multiple devices working in the same system, one using the existing line discipline functionality, and one with your new changes. > > Also, by adding new options, that means you are changing the logic > > here, > > why not create a new line discipline that acts the way you want it to > > act? The existing one is very small, it should not be much to make a > > new one with just the new functionality, right? > > I can retry on this track. However I believe , it will also need > support from userland utility (especially ldattach). I don't know if > this kind of consequences is in the scope of the discussion here. Why would userspace need to be modified? Try this as a new line discipline, should be much easier and simpler overall for everyone. thanks, greg k-h
