Re: [PATCH v2 2/2] s390/time: Add PtP driver

[Heiko Carstens <hca@linux.ibm.com>]

On Wed, Oct 16, 2024 at 01:53:00PM +0200, Sven Schnelle wrote:
> Add a small PtP driver which allows user space to get
> the values of the physical and tod clock. This allows
> programs like chrony to use STP as clock source and
> steer the kernel clock. The physical clock can be used
> as a debugging aid to get the clock without any additional
> offsets like STP steering or LPAR offset.
> 
> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
> ---
>  MAINTAINERS                   |   6 ++
>  arch/s390/include/asm/stp.h   |   1 +
>  arch/s390/include/asm/timex.h |   6 ++
>  arch/s390/kernel/time.c       |   6 ++
>  drivers/ptp/Kconfig           |  11 +++
>  drivers/ptp/Makefile          |   1 +
>  drivers/ptp/ptp_s390.c        | 129 ++++++++++++++++++++++++++++++++++
>  7 files changed, 160 insertions(+)
>  create mode 100644 drivers/ptp/ptp_s390.c

...

> +static __always_inline unsigned long eitod_to_ns(u128 todval)
> +{
> +	return (todval * 125) >> 9;
> +}

This should return u128 so the caller gets a non-truncated return value.

> +static struct timespec64 eitod_to_timespec64(union tod_clock *clk)
> +{
> +	return ns_to_timespec64(eitod_to_ns(clk->eitod) - TOD_UNIX_EPOCH);
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Misplaced braces. I guess you want:

	return ns_to_timespec64(eitod_to_ns(clk->eitod - TOD_UNIX_EPOCH));

> +static struct ptp_clock_info ptp_s390_stcke_info = {
> +	.owner		= THIS_MODULE,
> +	.name		= "IBM s390 STCKE Clock",

Please, as written before make this simply "s390 STCKE Clock".

> +static struct ptp_clock_info ptp_s390_qpt_info = {
> +	.owner		= THIS_MODULE,
> +	.name		= "IBM s390 Physical Clock",

"s390 Physical Clock"

> +MODULE_AUTHOR("Sven Schnelle <svens@linux.ibm.com>");
> +MODULE_DESCRIPTION("S390 Physical/STCKE Clock PtP Driver");

"s390 Physical/STCKE Clock PtP Driver"