[RFC 0/3] Add PPS generators

[RFC 0/3] Add PPS generators

[Rodolfo Giometti]

PPS generators are special hardware which are able to produce PPS
(Pulse Per Second) signals.

This RFC patchset adds the class pps-gen to the kernel in order to get
feedback useful to produce a well-thought-out and well-defined
interface for these devices.

Rodolfo Giometti (3):
  drivers pps: add PPS generators support
  Documentation pps.rst: add PPS generators documentation
  Documentation ABI: add PPS generators documentaion

 Documentation/ABI/testing/sysfs-pps-gen |  44 ++++
 Documentation/driver-api/pps.rst        |  40 ++++
 drivers/pps/Makefile                    |   3 +-
 drivers/pps/generators/Kconfig          |  19 +-
 drivers/pps/generators/Makefile         |   4 +
 drivers/pps/generators/pps_gen-dummy.c  |  83 +++++++
 drivers/pps/generators/pps_gen.c        | 283 ++++++++++++++++++++++++
 drivers/pps/generators/sysfs.c          |  89 ++++++++
 include/linux/pps_gen_kernel.h          |  57 +++++
 include/uapi/linux/pps_gen.h            |  35 +++
 10 files changed, 655 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-pps-gen
 create mode 100644 drivers/pps/generators/pps_gen-dummy.c
 create mode 100644 drivers/pps/generators/pps_gen.c
 create mode 100644 drivers/pps/generators/sysfs.c
 create mode 100644 include/linux/pps_gen_kernel.h
 create mode 100644 include/uapi/linux/pps_gen.h

-- 
2.34.1

[RFC 1/3] drivers pps: add PPS generators support

[Rodolfo Giometti]

Sometimes one needs to be able not only to catch PPS signals but to
produce them also. For example, running a distributed simulation,
which requires computers' clock to be synchronized very tightly.

This patch adds PPS generators class in order to have a well-defined
interface for these devices.

Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
---
 drivers/pps/Makefile                   |   3 +-
 drivers/pps/generators/Kconfig         |  19 +-
 drivers/pps/generators/Makefile        |   4 +
 drivers/pps/generators/pps_gen-dummy.c |  83 ++++++++
 drivers/pps/generators/pps_gen.c       | 283 +++++++++++++++++++++++++
 drivers/pps/generators/sysfs.c         |  89 ++++++++
 include/linux/pps_gen_kernel.h         |  57 +++++
 include/uapi/linux/pps_gen.h           |  35 +++
 8 files changed, 571 insertions(+), 2 deletions(-)
 create mode 100644 drivers/pps/generators/pps_gen-dummy.c
 create mode 100644 drivers/pps/generators/pps_gen.c
 create mode 100644 drivers/pps/generators/sysfs.c
 create mode 100644 include/linux/pps_gen_kernel.h
 create mode 100644 include/uapi/linux/pps_gen.h

diff --git a/drivers/pps/Makefile b/drivers/pps/Makefile
index ceaf65cc1f1d..0aea394d4e4d 100644
--- a/drivers/pps/Makefile
+++ b/drivers/pps/Makefile
@@ -6,6 +6,7 @@
 pps_core-y			:= pps.o kapi.o sysfs.o
 pps_core-$(CONFIG_NTP_PPS)	+= kc.o
 obj-$(CONFIG_PPS)		:= pps_core.o
-obj-y				+= clients/ generators/
+obj-y				+= clients/
+obj-$(CONFIG_PPS_GENERATOR)	+= generators/
 
 ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
diff --git a/drivers/pps/generators/Kconfig b/drivers/pps/generators/Kconfig
index d615e640fcad..e3dfe31609ba 100644
--- a/drivers/pps/generators/Kconfig
+++ b/drivers/pps/generators/Kconfig
@@ -3,7 +3,22 @@
 # PPS generators configuration
 #
 
-comment "PPS generators support"
+menuconfig PPS_GENERATOR
+	tristate "PPS generators support"
+	help
+	  PPS generators are special hardware which are able to produce PPS
+	  (Pulse Per Second) signals.
+
+if PPS_GENERATOR
+
+config PPS_GENERATOR_DUMMY
+        tristate "Dummy PPS generator (Testing generator, use for debug)"
+        help
+          If you say yes here you get support for a PPS debugging generator
+          (which actual generates no PPS signal at all).
+
+          This driver can also be built as a module.  If so, the module
+          will be called pps_gen-dummy.
 
 config PPS_GENERATOR_PARPORT
 	tristate "Parallel port PPS signal generator"
@@ -12,3 +27,5 @@ config PPS_GENERATOR_PARPORT
 	  If you say yes here you get support for a PPS signal generator which
 	  utilizes STROBE pin of a parallel port to send PPS signals. It uses
 	  parport abstraction layer and hrtimers to precisely control the signal.
+
+endif # PPS_GENERATOR
diff --git a/drivers/pps/generators/Makefile b/drivers/pps/generators/Makefile
index 2589fd0f2481..dc1aa5a4688b 100644
--- a/drivers/pps/generators/Makefile
+++ b/drivers/pps/generators/Makefile
@@ -3,6 +3,10 @@
 # Makefile for PPS generators.
 #
 
+pps_gen_core-y			:= pps_gen.o sysfs.o
+obj-$(CONFIG_PPS_GENERATOR)	:= pps_gen_core.o
+
+obj-$(CONFIG_PPS_GENERATOR_DUMMY)   += pps_gen-dummy.o
 obj-$(CONFIG_PPS_GENERATOR_PARPORT) += pps_gen_parport.o
 
 ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
diff --git a/drivers/pps/generators/pps_gen-dummy.c b/drivers/pps/generators/pps_gen-dummy.c
new file mode 100644
index 000000000000..2d257f3f9bf9
--- /dev/null
+++ b/drivers/pps/generators/pps_gen-dummy.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * PPS dummy generator
+ *
+ * Copyright (C) 2024   Rodolfo Giometti <giometti@enneenne.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/time.h>
+#include <linux/pps_gen_kernel.h>
+
+/*
+ * Global variables
+ */
+
+static struct pps_gen_device *pps_gen;
+
+/*
+ * PPS Generator methods
+ */
+
+static int pps_gen_dummy_get_time(struct pps_gen_device *pps_gen,
+                                        struct timespec64 *time)
+{
+	struct system_time_snapshot snap;
+
+	ktime_get_snapshot(&snap);
+	*time = ktime_to_timespec64(snap.real);
+
+	return 0;
+}
+
+static int pps_gen_dummy_enable(struct pps_gen_device *pps_gen, bool enable)
+{
+	/* always enabled */
+	return 0;
+}
+
+/*
+ * The PPS info struct
+ */
+
+static struct pps_gen_source_info pps_gen_dummy_info = {
+        .name			= "dummy",
+	.use_system_clock	= true,
+	.get_time		= pps_gen_dummy_get_time,
+	.enable			= pps_gen_dummy_enable,
+};
+
+/*
+ * Module staff
+ */
+
+static void __exit pps_gen_dummy_exit(void)
+{
+        dev_info(pps_gen->dev, "dummy PPS generator unregistered\n");
+
+        pps_gen_unregister_source(pps_gen);
+}
+
+static int __init pps_gen_dummy_init(void)
+{
+        pps_gen = pps_gen_register_source(&pps_gen_dummy_info);
+        if (IS_ERR(pps_gen)) {
+                pr_err("cannot register PPS generator\n");
+                return PTR_ERR(pps_gen);
+        }
+
+        dev_info(pps_gen->dev, "dummy PPS generator registered\n");
+
+        return 0;
+}
+
+module_init(pps_gen_dummy_init);
+module_exit(pps_gen_dummy_exit);
+
+MODULE_AUTHOR("Rodolfo Giometti <giometti@enneenne.com>");
+MODULE_DESCRIPTION("LinuxPPS dummy generator");
+MODULE_LICENSE("GPL");
diff --git a/drivers/pps/generators/pps_gen.c b/drivers/pps/generators/pps_gen.c
new file mode 100644
index 000000000000..40b05087b4b4
--- /dev/null
+++ b/drivers/pps/generators/pps_gen.c
@@ -0,0 +1,283 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * PPS generators core file
+ *
+ * Copyright (C) 2024   Rodolfo Giometti <giometti@enneenne.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/sched.h>
+#include <linux/time.h>
+#include <linux/timex.h>
+#include <linux/uaccess.h>
+#include <linux/idr.h>
+#include <linux/mutex.h>
+#include <linux/cdev.h>
+#include <linux/fs.h>
+#include <linux/pps_gen_kernel.h>
+#include <linux/slab.h>
+
+/*
+ * Local variables
+ */
+
+static int pps_gen_major;
+static struct class *pps_gen_class;
+
+static DEFINE_MUTEX(pps_gen_idr_lock);
+static DEFINE_IDR(pps_gen_idr);
+
+/*
+ * Char device methods
+ */
+
+static long pps_gen_cdev_ioctl(struct file *file,
+		unsigned int cmd, unsigned long arg)
+{
+	struct pps_gen_device *pps_gen = file->private_data;
+	unsigned int __user *uiuarg = (unsigned int __user *) arg;
+	unsigned int status;
+	int ret;
+
+	switch (cmd) {
+	case PPS_GEN_SETENABLE:
+		dev_dbg(pps_gen->dev, "PPS_GEN_SETENABLE\n");
+
+		ret = get_user(status, uiuarg);
+		if (ret)
+			return -EFAULT;
+
+		ret = pps_gen->info.enable(pps_gen, status);
+		if (ret)
+			return ret;
+		pps_gen->enabled = status;
+
+		break;
+
+	default:
+		return -ENOTTY;
+	}
+
+	return 0;
+}
+
+#ifdef CONFIG_COMPAT
+static long pps_gen_cdev_compat_ioctl(struct file *file,
+		unsigned int cmd, unsigned long arg)
+{
+	cmd = _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(void *));
+	return pps_gen_cdev_ioctl(file, cmd, arg);
+}
+#else
+#define pps_gen_cdev_compat_ioctl	NULL
+#endif
+
+static struct pps_gen_device *pps_gen_idr_get(unsigned long id)
+{
+	struct pps_gen_device *pps_gen;
+
+	mutex_lock(&pps_gen_idr_lock);
+	pps_gen = idr_find(&pps_gen_idr, id);
+	if (pps_gen)
+		kobject_get(&pps_gen->dev->kobj);
+
+	mutex_unlock(&pps_gen_idr_lock);
+	return pps_gen;
+}
+
+static int pps_gen_cdev_open(struct inode *inode, struct file *file)
+{
+	struct pps_gen_device *pps_gen = pps_gen_idr_get(iminor(inode));
+
+	if (!pps_gen)
+		return -ENODEV;
+
+	file->private_data = pps_gen;
+	return 0;
+}
+
+static int pps_gen_cdev_release(struct inode *inode, struct file *file)
+{
+	struct pps_gen_device *pps_gen = file->private_data;
+
+	WARN_ON(pps_gen->id != iminor(inode));
+	kobject_put(&pps_gen->dev->kobj);
+	return 0;
+}
+
+/*
+ * Char device stuff
+ */
+
+static const struct file_operations pps_gen_cdev_fops = {
+	.owner		= THIS_MODULE,
+	.compat_ioctl	= pps_gen_cdev_compat_ioctl,
+	.unlocked_ioctl	= pps_gen_cdev_ioctl,
+	.open		= pps_gen_cdev_open,
+	.release	= pps_gen_cdev_release,
+};
+
+static void pps_gen_device_destruct(struct device *dev)
+{
+	struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
+
+	pr_debug("deallocating pps-gen%d\n", pps_gen->id);
+	kfree(dev);
+	kfree(pps_gen);
+}
+
+static int pps_gen_register_cdev(struct pps_gen_device *pps_gen)
+{
+	int err;
+	dev_t devt;
+
+	mutex_lock(&pps_gen_idr_lock);
+
+	err = idr_alloc(&pps_gen_idr, pps_gen, 0, PPS_GEN_MAX_SOURCES,
+					GFP_KERNEL);
+	if (err < 0) {
+		if (err == -ENOSPC) {
+			pr_err("%s: too many PPS sources in the system\n",
+			       pps_gen->info.name);
+			err = -EBUSY;
+		}
+		goto out_unlock;
+	}
+	pps_gen->id = err;
+
+	devt = MKDEV(pps_gen_major, pps_gen->id);
+	pps_gen->dev = device_create(pps_gen_class, pps_gen->info.parent, devt,
+					pps_gen, "pps-gen%d", pps_gen->id);
+	if (IS_ERR(pps_gen->dev)) {
+		err = PTR_ERR(pps_gen->dev);
+		goto free_idr;
+	}
+
+	/* Override the release function with our own */
+	pps_gen->dev->release = pps_gen_device_destruct;
+
+	pr_debug("generator %s got cdev (%d:%d)\n",
+			pps_gen->info.name, pps_gen_major, pps_gen->id);
+
+	kobject_get(&pps_gen->dev->kobj);
+	mutex_unlock(&pps_gen_idr_lock);
+	return 0;
+
+free_idr:
+	idr_remove(&pps_gen_idr, pps_gen->id);
+out_unlock:
+	mutex_unlock(&pps_gen_idr_lock);
+	return err;
+}
+
+static void pps_gen_unregister_cdev(struct pps_gen_device *pps_gen)
+{
+	pr_debug("unregistering pps-gen%d\n", pps_gen->id);
+	device_destroy(pps_gen_class, pps_gen->dev->devt);
+
+	/* Now we can release the ID for re-use */
+	mutex_lock(&pps_gen_idr_lock);
+	idr_remove(&pps_gen_idr, pps_gen->id);
+	kobject_put(&pps_gen->dev->kobj);
+	mutex_unlock(&pps_gen_idr_lock);
+}
+
+/*
+ * Exported functions
+ */
+
+/* pps_gen_register_source - add a PPS generator in the system
+ * @info: the PPS generator info struct
+ *
+ * The function returns, in case of success, the PPS generaor device. Otherwise
+ * ERR_PTR(errno).
+ */
+
+struct pps_gen_device *pps_gen_register_source(struct pps_gen_source_info *info)
+{
+        struct pps_gen_device *pps_gen;
+        int err;
+
+        pps_gen = kzalloc(sizeof(struct pps_gen_device), GFP_KERNEL);
+        if (pps_gen == NULL) {
+                err = -ENOMEM;
+                goto pps_gen_register_source_exit;
+        }
+        pps_gen->info = *info;
+	pps_gen->enabled = false;
+
+        /* Create the char device */
+        err = pps_gen_register_cdev(pps_gen);
+        if (err < 0) {
+                pr_err("%s: unable to create char device\n",
+                                        info->name);
+                goto kfree_pps_gen;
+        }
+
+        dev_info(pps_gen->dev, "new PPS generator %s\n", info->name);
+
+        return pps_gen;
+
+kfree_pps_gen:
+        kfree(pps_gen);
+
+pps_gen_register_source_exit:
+        pr_err("%s: unable to register generaor\n", info->name);
+
+        return ERR_PTR(err);
+}
+EXPORT_SYMBOL(pps_gen_register_source);
+
+/* pps_gen_unregister_source - remove a PPS generator from the system
+ * @pps_gen: the PPS generator
+ */
+
+void pps_gen_unregister_source(struct pps_gen_device *pps_gen)
+{
+        pps_gen_unregister_cdev(pps_gen);
+}
+EXPORT_SYMBOL(pps_gen_unregister_source);
+
+/*
+ * Module stuff
+ */
+
+static void __exit pps_gen_exit(void)
+{
+	class_destroy(pps_gen_class);
+	__unregister_chrdev(pps_gen_major, 0, PPS_GEN_MAX_SOURCES, "pps-gen");
+}
+
+static int __init pps_gen_init(void)
+{
+	pps_gen_class = class_create("pps-gen");
+	if (IS_ERR(pps_gen_class)) {
+		pr_err("failed to allocate class\n");
+		return PTR_ERR(pps_gen_class);
+	}
+	pps_gen_class->dev_groups = pps_gen_groups;
+
+	pps_gen_major = __register_chrdev(0, 0, PPS_GEN_MAX_SOURCES, "pps-gen",
+				      &pps_gen_cdev_fops);
+	if (pps_gen_major < 0) {
+		pr_err("failed to allocate char device region\n");
+		goto remove_class;
+	}
+
+	return 0;
+
+remove_class:
+	class_destroy(pps_gen_class);
+	return pps_gen_major;
+}
+
+subsys_initcall(pps_gen_init);
+module_exit(pps_gen_exit);
+
+MODULE_AUTHOR("Rodolfo Giometti <giometti@enneenne.com>");
+MODULE_DESCRIPTION("LinuxPPS generators support");
+MODULE_LICENSE("GPL");
diff --git a/drivers/pps/generators/sysfs.c b/drivers/pps/generators/sysfs.c
new file mode 100644
index 000000000000..247030d138e1
--- /dev/null
+++ b/drivers/pps/generators/sysfs.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * PPS generators sysfs support
+ *
+ * Copyright (C) 2024   Rodolfo Giometti <giometti@enneenne.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/pps_gen_kernel.h>
+
+/*
+ * Attribute functions
+ */
+
+static ssize_t system_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+        struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
+
+        return sysfs_emit(buf, "%d\n", pps_gen->info.use_system_clock);
+}
+static DEVICE_ATTR_RO(system);
+
+static ssize_t time_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+        struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
+	struct timespec64 time;
+	int ret;
+
+        ret = pps_gen->info.get_time(pps_gen, &time);
+        if (ret)
+                return ret;
+
+        return sysfs_emit(buf, "%llu %09lu\n", time.tv_sec, time.tv_nsec);
+}
+static DEVICE_ATTR_RO(time);
+
+static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+        struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
+	bool status;
+	unsigned int enable;
+        int ret;
+
+	ret = sscanf(buf, "%u", &enable);
+        if (ret != 1)
+		return -EINVAL;
+	status = !!enable;
+
+        ret = pps_gen->info.enable(pps_gen, status);
+        if (ret)
+                return ret;
+	pps_gen->enabled = status;
+
+        return count;
+}
+static DEVICE_ATTR_WO(enable);
+
+static ssize_t name_show(struct device *dev, struct device_attribute *attr,
+                         char *buf)
+{
+        struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
+
+        return sysfs_emit(buf, "%s\n", pps_gen->info.name);
+}
+static DEVICE_ATTR_RO(name);
+
+static struct attribute *pps_gen_attrs[] = {
+        &dev_attr_enable.attr,
+        &dev_attr_name.attr,
+        &dev_attr_time.attr,
+        &dev_attr_system.attr,
+        NULL,
+};
+
+static const struct attribute_group pps_gen_group = {
+        .attrs = pps_gen_attrs,
+};
+
+const struct attribute_group *pps_gen_groups[] = {
+        &pps_gen_group,
+        NULL,
+};
diff --git a/include/linux/pps_gen_kernel.h b/include/linux/pps_gen_kernel.h
new file mode 100644
index 000000000000..5513415b53ec
--- /dev/null
+++ b/include/linux/pps_gen_kernel.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * PPS generator API kernel header
+ *
+ * Copyright (C) 2024   Rodolfo Giometti <giometti@enneenne.com>
+ */
+
+#ifndef LINUX_PPS_GEN_KERNEL_H
+#define LINUX_PPS_GEN_KERNEL_H
+
+#include <linux/pps_gen.h>
+#include <linux/cdev.h>
+#include <linux/device.h>
+
+/*
+ * Global defines
+ */
+
+struct pps_gen_device;
+
+/* The specific PPS source info */
+struct pps_gen_source_info {
+	char name[PPS_GEN_MAX_NAME_LEN];	/* symbolic name */
+	bool use_system_clock;
+
+	int (*get_time)(struct pps_gen_device *pps_gen,
+					struct timespec64 *time);
+	int (*enable)(struct pps_gen_device *pps_gen, bool enable);
+
+	struct module *owner;
+	struct device *parent;			/* for device_create */
+};
+
+/* The main struct */
+struct pps_gen_device {
+	struct pps_gen_source_info info;	/* PSS generator info */
+	bool enabled;				/* PSS generator status */
+
+	unsigned int id;			/* PPS generator unique ID */
+	struct device *dev;
+};
+
+/*
+ * Global variables
+ */
+
+extern const struct attribute_group *pps_gen_groups[];
+
+/*
+ * Exported functions
+ */
+
+extern struct pps_gen_device *pps_gen_register_source(
+		struct pps_gen_source_info *info);
+extern void pps_gen_unregister_source(struct pps_gen_device *pps_gen);
+
+#endif /* LINUX_PPS_GEN_KERNEL_H */
diff --git a/include/uapi/linux/pps_gen.h b/include/uapi/linux/pps_gen.h
new file mode 100644
index 000000000000..7b6f50fcab8c
--- /dev/null
+++ b/include/uapi/linux/pps_gen.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+/*
+ * PPS generator API header
+ *
+ * Copyright (C) 2024   Rodolfo Giometti <giometti@enneenne.com>
+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, write to the Free Software
+ *   Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+
+#ifndef _PPS_GEN_H_
+#define _PPS_GEN_H_
+
+#include <linux/types.h>
+
+#define PPS_GEN_MAX_SOURCES	16		/* should be enough... */
+#define PPS_GEN_MAX_NAME_LEN	32
+
+#include <linux/ioctl.h>
+
+#define PPS_GEN_SETENABLE	_IOW('g', 0xa1, unsigned int *)
+
+#endif /* _PPS_GEN_H_ */
-- 
2.34.1

[RFC 2/3] Documentation pps.rst: add PPS generators documentation

[Rodolfo Giometti]

This patch adds some examples about how to register a new PPS
generator in the system, and how to manage it.

Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
---
 Documentation/driver-api/pps.rst | 40 ++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/Documentation/driver-api/pps.rst b/Documentation/driver-api/pps.rst
index 78dded03e5d8..c71b3b878e41 100644
--- a/Documentation/driver-api/pps.rst
+++ b/Documentation/driver-api/pps.rst
@@ -202,6 +202,46 @@ Sometimes one needs to be able not only to catch PPS signals but to produce
 them also. For example, running a distributed simulation, which requires
 computers' clock to be synchronized very tightly.
 
+To do so the class pps-gen has been added. PPS generators can be
+registered int the kernel by defining a struct pps_gen_source_info as
+follows::
+
+    static struct pps_gen_source_info pps_gen_dummy_info = {
+            .name                   = "dummy",
+            .use_system_clock       = true,
+            .get_time               = pps_gen_dummy_get_time,
+            .enable                 = pps_gen_dummy_enable,
+    };
+
+Where the use_system_clock states if the generator uses the system
+clock to generate its pulses, or from a peripheral device
+clock. Method get_time() is used to query the time stored into the
+generator clock, while the method enable() is used to enable or
+disable the PPS pulse generation.
+
+Then calling the function pps_gen_register_source() in your
+initialization routine as follows a new generator is created into the
+system::
+
+    pps_gen = pps_gen_register_source(&pps_gen_dummy_info);
+
+Generators SYSFS support
+------------------------
+
+If the SYSFS filesystem is enabled in the kernel it provides a new class::
+
+    $ ls /sys/class/pps-gen/
+    pps-gen0/  pps-gen1/  pps-gen2/
+
+Every directory is the ID of a PPS generator defined in the system and
+inside you find several files::
+
+    $ ls -F /sys/class/pps-gen/pps-gen0/
+    dev  enable  name  power/  subsystem@  system  time  uevent
+
+To enable the PPS signal generation you can use the command below::
+
+    $ echo 1 > /sys/class/pps-gen/pps-gen0/enable
 
 Parallel port generator
 ------------------------
-- 
2.34.1

[RFC 3/3] Documentation ABI: add PPS generators documentaion

[Rodolfo Giometti]

This patch adds the documentation for the ABI between the Linux kernel
and userspace regarding the PPS generators.

Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
---
 Documentation/ABI/testing/sysfs-pps-gen | 44 +++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-pps-gen

diff --git a/Documentation/ABI/testing/sysfs-pps-gen b/Documentation/ABI/testing/sysfs-pps-gen
new file mode 100644
index 000000000000..9ad066cb3ce5
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-pps-gen
@@ -0,0 +1,44 @@
+What:		/sys/class/pps-gen/
+Date:		October 2024
+Contact:	Rodolfo Giometti <giometti@enneenne.com>
+Description:
+		The /sys/class/pps-gen/ directory will contain files and
+		directories that will provide a unified interface to
+		the PPS generators.
+
+What:		/sys/class/pps-gen/pps-genX/
+Date:		October 2024
+Contact:	Rodolfo Giometti <giometti@enneenne.com>
+Description:
+		The /sys/class/pps-gen/pps-genX/ directory is related to X-th
+		PPS generator into the system. Each directory will
+		contain files to manage and control its PPS generator.
+
+What:		/sys/class/pps-gen/pps-genX/enable
+Date:		October 2024
+Contact:	Rodolfo Giometti <giometti@enneenne.com>
+Description:
+		This write-only file enables or disables generation of the
+		PPS signal.
+
+What:		/sys/class/pps-gen/pps-genX/name
+Date:		October 2024
+Contact:	Rodolfo Giometti <giometti@enneenne.com>
+Description:
+		This read-only file reports the name of the X-th generator.
+
+What:		/sys/class/pps-gen/pps-genX/system
+Date:		October 2024
+Contact:	Rodolfo Giometti <giometti@enneenne.com>
+Description:
+		This read-only file returns "1" if the generator takes the
+		timing from the system clock, while it returns "0" if not
+		(i.e. from a peripheral device clock).
+
+What:		/sys/class/pps-gen/pps-genX/time
+Date:		October 2024
+Contact:	Rodolfo Giometti <giometti@enneenne.com>
+Description:
+		This read-only file contains the current time stored into the
+		generator clock as two integers representing the current time
+		seconds and nanoseconds. 
-- 
2.34.1

Re: [RFC 1/3] drivers pps: add PPS generators support

[Greg KH]

On Tue, Oct 08, 2024 at 03:50:31PM +0200, Rodolfo Giometti wrote:
> Sometimes one needs to be able not only to catch PPS signals but to
> produce them also. For example, running a distributed simulation,
> which requires computers' clock to be synchronized very tightly.
> 
> This patch adds PPS generators class in order to have a well-defined
> interface for these devices.
> 
> Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>

Some minor comments on the design:

> ---
>  drivers/pps/Makefile                   |   3 +-
>  drivers/pps/generators/Kconfig         |  19 +-
>  drivers/pps/generators/Makefile        |   4 +
>  drivers/pps/generators/pps_gen-dummy.c |  83 ++++++++
>  drivers/pps/generators/pps_gen.c       | 283 +++++++++++++++++++++++++
>  drivers/pps/generators/sysfs.c         |  89 ++++++++
>  include/linux/pps_gen_kernel.h         |  57 +++++
>  include/uapi/linux/pps_gen.h           |  35 +++
>  8 files changed, 571 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/pps/generators/pps_gen-dummy.c
>  create mode 100644 drivers/pps/generators/pps_gen.c
>  create mode 100644 drivers/pps/generators/sysfs.c
>  create mode 100644 include/linux/pps_gen_kernel.h
>  create mode 100644 include/uapi/linux/pps_gen.h
> 
> diff --git a/drivers/pps/Makefile b/drivers/pps/Makefile
> index ceaf65cc1f1d..0aea394d4e4d 100644
> --- a/drivers/pps/Makefile
> +++ b/drivers/pps/Makefile
> @@ -6,6 +6,7 @@
>  pps_core-y			:= pps.o kapi.o sysfs.o
>  pps_core-$(CONFIG_NTP_PPS)	+= kc.o
>  obj-$(CONFIG_PPS)		:= pps_core.o
> -obj-y				+= clients/ generators/
> +obj-y				+= clients/
> +obj-$(CONFIG_PPS_GENERATOR)	+= generators/
>  
>  ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
> diff --git a/drivers/pps/generators/Kconfig b/drivers/pps/generators/Kconfig
> index d615e640fcad..e3dfe31609ba 100644
> --- a/drivers/pps/generators/Kconfig
> +++ b/drivers/pps/generators/Kconfig
> @@ -3,7 +3,22 @@
>  # PPS generators configuration
>  #
>  
> -comment "PPS generators support"
> +menuconfig PPS_GENERATOR
> +	tristate "PPS generators support"
> +	help
> +	  PPS generators are special hardware which are able to produce PPS
> +	  (Pulse Per Second) signals.
> +
> +if PPS_GENERATOR
> +
> +config PPS_GENERATOR_DUMMY
> +        tristate "Dummy PPS generator (Testing generator, use for debug)"
> +        help
> +          If you say yes here you get support for a PPS debugging generator
> +          (which actual generates no PPS signal at all).
> +
> +          This driver can also be built as a module.  If so, the module
> +          will be called pps_gen-dummy.

Put the dummy driver in a separate patch please, it doesn't belong with
the core changes.

>  
>  config PPS_GENERATOR_PARPORT
>  	tristate "Parallel port PPS signal generator"
> @@ -12,3 +27,5 @@ config PPS_GENERATOR_PARPORT
>  	  If you say yes here you get support for a PPS signal generator which
>  	  utilizes STROBE pin of a parallel port to send PPS signals. It uses
>  	  parport abstraction layer and hrtimers to precisely control the signal.
> +
> +endif # PPS_GENERATOR
> diff --git a/drivers/pps/generators/Makefile b/drivers/pps/generators/Makefile
> index 2589fd0f2481..dc1aa5a4688b 100644
> --- a/drivers/pps/generators/Makefile
> +++ b/drivers/pps/generators/Makefile
> @@ -3,6 +3,10 @@
>  # Makefile for PPS generators.
>  #
>  
> +pps_gen_core-y			:= pps_gen.o sysfs.o
> +obj-$(CONFIG_PPS_GENERATOR)	:= pps_gen_core.o
> +
> +obj-$(CONFIG_PPS_GENERATOR_DUMMY)   += pps_gen-dummy.o
>  obj-$(CONFIG_PPS_GENERATOR_PARPORT) += pps_gen_parport.o
>  
>  ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
> diff --git a/drivers/pps/generators/pps_gen-dummy.c b/drivers/pps/generators/pps_gen-dummy.c
> new file mode 100644
> index 000000000000..2d257f3f9bf9
> --- /dev/null
> +++ b/drivers/pps/generators/pps_gen-dummy.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * PPS dummy generator
> + *
> + * Copyright (C) 2024   Rodolfo Giometti <giometti@enneenne.com>

Why so many spaces after "2024"?

> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Why is this needed, drivers should only ever use dev_*() calls, never
pr_*() calls.

> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/time.h>
> +#include <linux/pps_gen_kernel.h>
> +
> +/*
> + * Global variables
> + */
> +
> +static struct pps_gen_device *pps_gen;

That's by definition, static, not global :)

Also, why is it needed at all?

> +
> +/*
> + * PPS Generator methods
> + */
> +
> +static int pps_gen_dummy_get_time(struct pps_gen_device *pps_gen,
> +                                        struct timespec64 *time)
> +{
> +	struct system_time_snapshot snap;
> +
> +	ktime_get_snapshot(&snap);
> +	*time = ktime_to_timespec64(snap.real);
> +
> +	return 0;
> +}
> +
> +static int pps_gen_dummy_enable(struct pps_gen_device *pps_gen, bool enable)
> +{
> +	/* always enabled */
> +	return 0;
> +}
> +
> +/*
> + * The PPS info struct
> + */
> +
> +static struct pps_gen_source_info pps_gen_dummy_info = {
> +        .name			= "dummy",
> +	.use_system_clock	= true,
> +	.get_time		= pps_gen_dummy_get_time,
> +	.enable			= pps_gen_dummy_enable,
> +};
> +
> +/*
> + * Module staff
> + */
> +
> +static void __exit pps_gen_dummy_exit(void)
> +{
> +        dev_info(pps_gen->dev, "dummy PPS generator unregistered\n");

When drivers are working properly, they are quiet.

> +
> +        pps_gen_unregister_source(pps_gen);
> +}
> +
> +static int __init pps_gen_dummy_init(void)
> +{
> +        pps_gen = pps_gen_register_source(&pps_gen_dummy_info);
> +        if (IS_ERR(pps_gen)) {
> +                pr_err("cannot register PPS generator\n");
> +                return PTR_ERR(pps_gen);
> +        }
> +
> +        dev_info(pps_gen->dev, "dummy PPS generator registered\n");

Again, quiet...

> +
> +        return 0;
> +}
> +
> +module_init(pps_gen_dummy_init);
> +module_exit(pps_gen_dummy_exit);
> +
> +MODULE_AUTHOR("Rodolfo Giometti <giometti@enneenne.com>");
> +MODULE_DESCRIPTION("LinuxPPS dummy generator");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/pps/generators/pps_gen.c b/drivers/pps/generators/pps_gen.c
> new file mode 100644
> index 000000000000..40b05087b4b4
> --- /dev/null
> +++ b/drivers/pps/generators/pps_gen.c
> @@ -0,0 +1,283 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * PPS generators core file
> + *
> + * Copyright (C) 2024   Rodolfo Giometti <giometti@enneenne.com>

Again spaces.

> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Again, not needed.

> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/sched.h>
> +#include <linux/time.h>
> +#include <linux/timex.h>
> +#include <linux/uaccess.h>
> +#include <linux/idr.h>
> +#include <linux/mutex.h>
> +#include <linux/cdev.h>

Why a cdev?

> +#include <linux/fs.h>
> +#include <linux/pps_gen_kernel.h>
> +#include <linux/slab.h>
> +
> +/*
> + * Local variables
> + */
> +
> +static int pps_gen_major;
> +static struct class *pps_gen_class;
> +
> +static DEFINE_MUTEX(pps_gen_idr_lock);
> +static DEFINE_IDR(pps_gen_idr);
> +
> +/*
> + * Char device methods
> + */
> +
> +static long pps_gen_cdev_ioctl(struct file *file,
> +		unsigned int cmd, unsigned long arg)
> +{
> +	struct pps_gen_device *pps_gen = file->private_data;
> +	unsigned int __user *uiuarg = (unsigned int __user *) arg;
> +	unsigned int status;
> +	int ret;
> +
> +	switch (cmd) {
> +	case PPS_GEN_SETENABLE:
> +		dev_dbg(pps_gen->dev, "PPS_GEN_SETENABLE\n");
> +
> +		ret = get_user(status, uiuarg);
> +		if (ret)
> +			return -EFAULT;
> +
> +		ret = pps_gen->info.enable(pps_gen, status);
> +		if (ret)
> +			return ret;
> +		pps_gen->enabled = status;
> +
> +		break;
> +
> +	default:
> +		return -ENOTTY;
> +	}
> +
> +	return 0;
> +}

Why is there an ioctl call?  That's a totally different user/kernel api
than sysfs, why do you have 2?

> +
> +#ifdef CONFIG_COMPAT
> +static long pps_gen_cdev_compat_ioctl(struct file *file,
> +		unsigned int cmd, unsigned long arg)
> +{
> +	cmd = _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(void *));
> +	return pps_gen_cdev_ioctl(file, cmd, arg);
> +}
> +#else
> +#define pps_gen_cdev_compat_ioctl	NULL
> +#endif
> +
> +static struct pps_gen_device *pps_gen_idr_get(unsigned long id)
> +{
> +	struct pps_gen_device *pps_gen;
> +
> +	mutex_lock(&pps_gen_idr_lock);
> +	pps_gen = idr_find(&pps_gen_idr, id);
> +	if (pps_gen)
> +		kobject_get(&pps_gen->dev->kobj);
> +
> +	mutex_unlock(&pps_gen_idr_lock);

Doesn't an idr have a lock in it?  I can never remember...

> +	return pps_gen;
> +}
> +
> +static int pps_gen_cdev_open(struct inode *inode, struct file *file)
> +{
> +	struct pps_gen_device *pps_gen = pps_gen_idr_get(iminor(inode));
> +
> +	if (!pps_gen)
> +		return -ENODEV;
> +
> +	file->private_data = pps_gen;
> +	return 0;
> +}
> +
> +static int pps_gen_cdev_release(struct inode *inode, struct file *file)
> +{
> +	struct pps_gen_device *pps_gen = file->private_data;
> +
> +	WARN_ON(pps_gen->id != iminor(inode));

If this can never happen, don't add this line.  If it can happen, then
handle it properly.  Either way, don't reboot a box because it happened.

> +	kobject_put(&pps_gen->dev->kobj);

Messing with a kobject reference directly from a device feels wrong and
should never be done.  Please use the proper apis.


> +	return 0;
> +}
> +
> +/*
> + * Char device stuff
> + */
> +
> +static const struct file_operations pps_gen_cdev_fops = {
> +	.owner		= THIS_MODULE,
> +	.compat_ioctl	= pps_gen_cdev_compat_ioctl,

Why compat for a new ioctl?  Why not write it properly to not need it?

> +	.unlocked_ioctl	= pps_gen_cdev_ioctl,
> +	.open		= pps_gen_cdev_open,
> +	.release	= pps_gen_cdev_release,
> +};
> +
> +static void pps_gen_device_destruct(struct device *dev)
> +{
> +	struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
> +
> +	pr_debug("deallocating pps-gen%d\n", pps_gen->id);

ftrace is your friend.

> +	kfree(dev);
> +	kfree(pps_gen);
> +}
> +
> +static int pps_gen_register_cdev(struct pps_gen_device *pps_gen)
> +{
> +	int err;
> +	dev_t devt;
> +
> +	mutex_lock(&pps_gen_idr_lock);
> +
> +	err = idr_alloc(&pps_gen_idr, pps_gen, 0, PPS_GEN_MAX_SOURCES,
> +					GFP_KERNEL);
> +	if (err < 0) {
> +		if (err == -ENOSPC) {
> +			pr_err("%s: too many PPS sources in the system\n",
> +			       pps_gen->info.name);
> +			err = -EBUSY;
> +		}
> +		goto out_unlock;
> +	}
> +	pps_gen->id = err;
> +
> +	devt = MKDEV(pps_gen_major, pps_gen->id);
> +	pps_gen->dev = device_create(pps_gen_class, pps_gen->info.parent, devt,
> +					pps_gen, "pps-gen%d", pps_gen->id);
> +	if (IS_ERR(pps_gen->dev)) {
> +		err = PTR_ERR(pps_gen->dev);
> +		goto free_idr;
> +	}
> +
> +	/* Override the release function with our own */
> +	pps_gen->dev->release = pps_gen_device_destruct;
> +
> +	pr_debug("generator %s got cdev (%d:%d)\n",
> +			pps_gen->info.name, pps_gen_major, pps_gen->id);

Why not dev_dbg()?

> +
> +	kobject_get(&pps_gen->dev->kobj);
> +	mutex_unlock(&pps_gen_idr_lock);
> +	return 0;
> +
> +free_idr:
> +	idr_remove(&pps_gen_idr, pps_gen->id);
> +out_unlock:
> +	mutex_unlock(&pps_gen_idr_lock);
> +	return err;
> +}
> +
> +static void pps_gen_unregister_cdev(struct pps_gen_device *pps_gen)
> +{
> +	pr_debug("unregistering pps-gen%d\n", pps_gen->id);
> +	device_destroy(pps_gen_class, pps_gen->dev->devt);
> +
> +	/* Now we can release the ID for re-use */
> +	mutex_lock(&pps_gen_idr_lock);
> +	idr_remove(&pps_gen_idr, pps_gen->id);
> +	kobject_put(&pps_gen->dev->kobj);
> +	mutex_unlock(&pps_gen_idr_lock);
> +}
> +
> +/*
> + * Exported functions
> + */
> +
> +/* pps_gen_register_source - add a PPS generator in the system
> + * @info: the PPS generator info struct
> + *
> + * The function returns, in case of success, the PPS generaor device. Otherwise
> + * ERR_PTR(errno).
> + */
> +
> +struct pps_gen_device *pps_gen_register_source(struct pps_gen_source_info *info)
> +{
> +        struct pps_gen_device *pps_gen;
> +        int err;
> +
> +        pps_gen = kzalloc(sizeof(struct pps_gen_device), GFP_KERNEL);
> +        if (pps_gen == NULL) {
> +                err = -ENOMEM;
> +                goto pps_gen_register_source_exit;
> +        }
> +        pps_gen->info = *info;
> +	pps_gen->enabled = false;

Whitespace is all messed up here, did you run checkpatch?


> +
> +        /* Create the char device */
> +        err = pps_gen_register_cdev(pps_gen);
> +        if (err < 0) {
> +                pr_err("%s: unable to create char device\n",
> +                                        info->name);
> +                goto kfree_pps_gen;
> +        }
> +
> +        dev_info(pps_gen->dev, "new PPS generator %s\n", info->name);

Again, quiet...

> +
> +        return pps_gen;
> +
> +kfree_pps_gen:
> +        kfree(pps_gen);
> +
> +pps_gen_register_source_exit:
> +        pr_err("%s: unable to register generaor\n", info->name);

dev_err()?

> +
> +        return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL(pps_gen_register_source);

I have to ask, why not EXPORT_SYMBOL_GPL()?


> +
> +/* pps_gen_unregister_source - remove a PPS generator from the system
> + * @pps_gen: the PPS generator
> + */
> +
> +void pps_gen_unregister_source(struct pps_gen_device *pps_gen)
> +{
> +        pps_gen_unregister_cdev(pps_gen);
> +}
> +EXPORT_SYMBOL(pps_gen_unregister_source);
> +
> +/*
> + * Module stuff
> + */
> +
> +static void __exit pps_gen_exit(void)
> +{
> +	class_destroy(pps_gen_class);
> +	__unregister_chrdev(pps_gen_major, 0, PPS_GEN_MAX_SOURCES, "pps-gen");

Why the __ version?  Are you sure?

> +}
> +
> +static int __init pps_gen_init(void)
> +{
> +	pps_gen_class = class_create("pps-gen");
> +	if (IS_ERR(pps_gen_class)) {
> +		pr_err("failed to allocate class\n");
> +		return PTR_ERR(pps_gen_class);
> +	}
> +	pps_gen_class->dev_groups = pps_gen_groups;
> +
> +	pps_gen_major = __register_chrdev(0, 0, PPS_GEN_MAX_SOURCES, "pps-gen",
> +				      &pps_gen_cdev_fops);

Again, why __?

> +	if (pps_gen_major < 0) {
> +		pr_err("failed to allocate char device region\n");
> +		goto remove_class;
> +	}
> +
> +	return 0;
> +
> +remove_class:
> +	class_destroy(pps_gen_class);
> +	return pps_gen_major;
> +}
> +
> +subsys_initcall(pps_gen_init);
> +module_exit(pps_gen_exit);
> +
> +MODULE_AUTHOR("Rodolfo Giometti <giometti@enneenne.com>");
> +MODULE_DESCRIPTION("LinuxPPS generators support");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/pps/generators/sysfs.c b/drivers/pps/generators/sysfs.c
> new file mode 100644
> index 000000000000..247030d138e1
> --- /dev/null
> +++ b/drivers/pps/generators/sysfs.c
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * PPS generators sysfs support
> + *
> + * Copyright (C) 2024   Rodolfo Giometti <giometti@enneenne.com>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

again...

> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/pps_gen_kernel.h>
> +
> +/*
> + * Attribute functions
> + */
> +
> +static ssize_t system_show(struct device *dev, struct device_attribute *attr,
> +				char *buf)
> +{
> +        struct pps_gen_device *pps_gen = dev_get_drvdata(dev);

Again whitespace...

> +
> +        return sysfs_emit(buf, "%d\n", pps_gen->info.use_system_clock);
> +}
> +static DEVICE_ATTR_RO(system);
> +
> +static ssize_t time_show(struct device *dev, struct device_attribute *attr,
> +				char *buf)
> +{
> +        struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
> +	struct timespec64 time;
> +	int ret;
> +
> +        ret = pps_gen->info.get_time(pps_gen, &time);
> +        if (ret)
> +                return ret;
> +
> +        return sysfs_emit(buf, "%llu %09lu\n", time.tv_sec, time.tv_nsec);
> +}
> +static DEVICE_ATTR_RO(time);
> +
> +static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +        struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
> +	bool status;
> +	unsigned int enable;
> +        int ret;
> +
> +	ret = sscanf(buf, "%u", &enable);
> +        if (ret != 1)
> +		return -EINVAL;
> +	status = !!enable;
> +
> +        ret = pps_gen->info.enable(pps_gen, status);
> +        if (ret)
> +                return ret;
> +	pps_gen->enabled = status;
> +
> +        return count;
> +}
> +static DEVICE_ATTR_WO(enable);
> +
> +static ssize_t name_show(struct device *dev, struct device_attribute *attr,
> +                         char *buf)
> +{
> +        struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
> +
> +        return sysfs_emit(buf, "%s\n", pps_gen->info.name);

Why have a separate name?  That shouldn't matter at all.  If it does
matter, than link to the device that created it properly, don't make up
yet another name for your device.

> +}
> +static DEVICE_ATTR_RO(name);
> +
> +static struct attribute *pps_gen_attrs[] = {
> +        &dev_attr_enable.attr,
> +        &dev_attr_name.attr,
> +        &dev_attr_time.attr,
> +        &dev_attr_system.attr,
> +        NULL,
> +};
> +
> +static const struct attribute_group pps_gen_group = {
> +        .attrs = pps_gen_attrs,
> +};
> +
> +const struct attribute_group *pps_gen_groups[] = {
> +        &pps_gen_group,
> +        NULL,
> +};
> diff --git a/include/linux/pps_gen_kernel.h b/include/linux/pps_gen_kernel.h
> new file mode 100644
> index 000000000000..5513415b53ec
> --- /dev/null
> +++ b/include/linux/pps_gen_kernel.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * PPS generator API kernel header
> + *
> + * Copyright (C) 2024   Rodolfo Giometti <giometti@enneenne.com>
> + */
> +
> +#ifndef LINUX_PPS_GEN_KERNEL_H
> +#define LINUX_PPS_GEN_KERNEL_H
> +
> +#include <linux/pps_gen.h>
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +
> +/*
> + * Global defines
> + */
> +
> +struct pps_gen_device;
> +
> +/* The specific PPS source info */
> +struct pps_gen_source_info {
> +	char name[PPS_GEN_MAX_NAME_LEN];	/* symbolic name */
> +	bool use_system_clock;
> +
> +	int (*get_time)(struct pps_gen_device *pps_gen,
> +					struct timespec64 *time);
> +	int (*enable)(struct pps_gen_device *pps_gen, bool enable);
> +
> +	struct module *owner;
> +	struct device *parent;			/* for device_create */
> +};
> +
> +/* The main struct */
> +struct pps_gen_device {
> +	struct pps_gen_source_info info;	/* PSS generator info */
> +	bool enabled;				/* PSS generator status */
> +
> +	unsigned int id;			/* PPS generator unique ID */
> +	struct device *dev;

Why not be a real device? What is this a pointer to?

> +};

This structure can be private, right?

> +
> +/*
> + * Global variables
> + */
> +
> +extern const struct attribute_group *pps_gen_groups[];

Why is this global?

> +
> +/*
> + * Exported functions
> + */
> +
> +extern struct pps_gen_device *pps_gen_register_source(
> +		struct pps_gen_source_info *info);
> +extern void pps_gen_unregister_source(struct pps_gen_device *pps_gen);
> +
> +#endif /* LINUX_PPS_GEN_KERNEL_H */
> diff --git a/include/uapi/linux/pps_gen.h b/include/uapi/linux/pps_gen.h
> new file mode 100644
> index 000000000000..7b6f50fcab8c
> --- /dev/null
> +++ b/include/uapi/linux/pps_gen.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */

I have to ask, why "GPL-2.0+"?


> +/*
> + * PPS generator API header
> + *
> + * Copyright (C) 2024   Rodolfo Giometti <giometti@enneenne.com>
> + *
> + *   This program is free software; you can redistribute it and/or modify
> + *   it under the terms of the GNU General Public License as published by
> + *   the Free Software Foundation; either version 2 of the License, or
> + *   (at your option) any later version.
> + *
> + *   This program is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *   GNU General Public License for more details.
> + *
> + *   You should have received a copy of the GNU General Public License
> + *   along with this program; if not, write to the Free Software
> + *   Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.

License boilerplate should be removed.

> + */
> +
> +
> +#ifndef _PPS_GEN_H_
> +#define _PPS_GEN_H_
> +
> +#include <linux/types.h>
> +
> +#define PPS_GEN_MAX_SOURCES	16		/* should be enough... */

What is this for?  Who is using it in userspace?

> +#define PPS_GEN_MAX_NAME_LEN	32

Why is this exported to userspace?

> +
> +#include <linux/ioctl.h>
> +
> +#define PPS_GEN_SETENABLE	_IOW('g', 0xa1, unsigned int *)

Documentation for this new ioctl?

thanks,

greg k-h

Re: [RFC 2/3] Documentation pps.rst: add PPS generators documentation

[Greg KH]

On Tue, Oct 08, 2024 at 03:50:32PM +0200, Rodolfo Giometti wrote:
> This patch adds some examples about how to register a new PPS
> generator in the system, and how to manage it.
> 
> Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
> ---
>  Documentation/driver-api/pps.rst | 40 ++++++++++++++++++++++++++++++++

All of this can go into the .c file and autogenerated there, no need for
a separate .rst file that will quickly get out-of-date.

thanks,

greg k-h

Re: [RFC 3/3] Documentation ABI: add PPS generators documentaion

[Greg KH]

On Tue, Oct 08, 2024 at 03:50:33PM +0200, Rodolfo Giometti wrote:
> This patch adds the documentation for the ABI between the Linux kernel
> and userspace regarding the PPS generators.
> 
> Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
> ---
>  Documentation/ABI/testing/sysfs-pps-gen | 44 +++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-pps-gen
> 
> diff --git a/Documentation/ABI/testing/sysfs-pps-gen b/Documentation/ABI/testing/sysfs-pps-gen
> new file mode 100644
> index 000000000000..9ad066cb3ce5
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-pps-gen
> @@ -0,0 +1,44 @@
> +What:		/sys/class/pps-gen/
> +Date:		October 2024
> +Contact:	Rodolfo Giometti <giometti@enneenne.com>
> +Description:
> +		The /sys/class/pps-gen/ directory will contain files and
> +		directories that will provide a unified interface to
> +		the PPS generators.
> +
> +What:		/sys/class/pps-gen/pps-genX/
> +Date:		October 2024
> +Contact:	Rodolfo Giometti <giometti@enneenne.com>
> +Description:
> +		The /sys/class/pps-gen/pps-genX/ directory is related to X-th
> +		PPS generator into the system. Each directory will
> +		contain files to manage and control its PPS generator.
> +
> +What:		/sys/class/pps-gen/pps-genX/enable
> +Date:		October 2024
> +Contact:	Rodolfo Giometti <giometti@enneenne.com>
> +Description:
> +		This write-only file enables or disables generation of the
> +		PPS signal.
> +
> +What:		/sys/class/pps-gen/pps-genX/name
> +Date:		October 2024
> +Contact:	Rodolfo Giometti <giometti@enneenne.com>
> +Description:
> +		This read-only file reports the name of the X-th generator.

Again, why a name?  What is that for?

> +
> +What:		/sys/class/pps-gen/pps-genX/system
> +Date:		October 2024
> +Contact:	Rodolfo Giometti <giometti@enneenne.com>
> +Description:
> +		This read-only file returns "1" if the generator takes the
> +		timing from the system clock, while it returns "0" if not
> +		(i.e. from a peripheral device clock).
> +
> +What:		/sys/class/pps-gen/pps-genX/time
> +Date:		October 2024
> +Contact:	Rodolfo Giometti <giometti@enneenne.com>
> +Description:
> +		This read-only file contains the current time stored into the
> +		generator clock as two integers representing the current time
> +		seconds and nanoseconds. 

Trailing whitespace, please always run checkpatch.pl.

thanks,

greg k-h

RE: [RFC 1/3] drivers pps: add PPS generators support

[Hall, Christopher S]

Hi Rudolfo,

> -----Original Message-----
> From: Rodolfo Giometti <giometti@enneenne.com>
> Sent: Tuesday, October 08, 2024 6:51 AM
> To: linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org

> Sometimes one needs to be able not only to catch PPS signals but to
> produce them also. For example, running a distributed simulation,
> which requires computers' clock to be synchronized very tightly.
> 
> This patch adds PPS generators class in order to have a well-defined
> interface for these devices.
> 
> Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
> ---
>  drivers/pps/Makefile                   |   3 +-
>  drivers/pps/generators/Kconfig         |  19 +-
>  drivers/pps/generators/Makefile        |   4 +
>  drivers/pps/generators/pps_gen-dummy.c |  83 ++++++++
>  drivers/pps/generators/pps_gen.c       | 283 +++++++++++++++++++++++++
>  drivers/pps/generators/sysfs.c         |  89 ++++++++
>  include/linux/pps_gen_kernel.h         |  57 +++++
>  include/uapi/linux/pps_gen.h           |  35 +++
>  8 files changed, 571 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/pps/generators/pps_gen-dummy.c
>  create mode 100644 drivers/pps/generators/pps_gen.c
>  create mode 100644 drivers/pps/generators/sysfs.c
>  create mode 100644 include/linux/pps_gen_kernel.h
>  create mode 100644 include/uapi/linux/pps_gen.h

This looks pretty good to me. I would like to see an alarm callback. We are able
to detect a missed event and rather than stopping inexplicably or writing to the
system log, it would be better to be able to notify an application directly.

Off the top of my head, something like:

void pps_gen_alarm(pps_gen_device *pps_gen) {
	pps_gen->alarm = 1;
	sysfs_notify(pps_gen->dev->kobj, NULL, "alarm");
}

The device is reset by disabling/enabling and this resets the alarm flag.

Could we add something like this?

Thanks,
Chris

Re: [RFC 1/3] drivers pps: add PPS generators support

[Rodolfo Giometti]

On 08/10/24 17:42, Greg KH wrote:
> On Tue, Oct 08, 2024 at 03:50:31PM +0200, Rodolfo Giometti wrote:
>> Sometimes one needs to be able not only to catch PPS signals but to
>> produce them also. For example, running a distributed simulation,
>> which requires computers' clock to be synchronized very tightly.
>>
>> This patch adds PPS generators class in order to have a well-defined
>> interface for these devices.
>>
>> Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
> 
> Some minor comments on the design:
> 
>> ---
>>   drivers/pps/Makefile                   |   3 +-
>>   drivers/pps/generators/Kconfig         |  19 +-
>>   drivers/pps/generators/Makefile        |   4 +
>>   drivers/pps/generators/pps_gen-dummy.c |  83 ++++++++
>>   drivers/pps/generators/pps_gen.c       | 283 +++++++++++++++++++++++++
>>   drivers/pps/generators/sysfs.c         |  89 ++++++++
>>   include/linux/pps_gen_kernel.h         |  57 +++++
>>   include/uapi/linux/pps_gen.h           |  35 +++
>>   8 files changed, 571 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/pps/generators/pps_gen-dummy.c
>>   create mode 100644 drivers/pps/generators/pps_gen.c
>>   create mode 100644 drivers/pps/generators/sysfs.c
>>   create mode 100644 include/linux/pps_gen_kernel.h
>>   create mode 100644 include/uapi/linux/pps_gen.h
>>
>> diff --git a/drivers/pps/Makefile b/drivers/pps/Makefile
>> index ceaf65cc1f1d..0aea394d4e4d 100644
>> --- a/drivers/pps/Makefile
>> +++ b/drivers/pps/Makefile
>> @@ -6,6 +6,7 @@
>>   pps_core-y			:= pps.o kapi.o sysfs.o
>>   pps_core-$(CONFIG_NTP_PPS)	+= kc.o
>>   obj-$(CONFIG_PPS)		:= pps_core.o
>> -obj-y				+= clients/ generators/
>> +obj-y				+= clients/
>> +obj-$(CONFIG_PPS_GENERATOR)	+= generators/
>>   
>>   ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
>> diff --git a/drivers/pps/generators/Kconfig b/drivers/pps/generators/Kconfig
>> index d615e640fcad..e3dfe31609ba 100644
>> --- a/drivers/pps/generators/Kconfig
>> +++ b/drivers/pps/generators/Kconfig
>> @@ -3,7 +3,22 @@
>>   # PPS generators configuration
>>   #
>>   
>> -comment "PPS generators support"
>> +menuconfig PPS_GENERATOR
>> +	tristate "PPS generators support"
>> +	help
>> +	  PPS generators are special hardware which are able to produce PPS
>> +	  (Pulse Per Second) signals.
>> +
>> +if PPS_GENERATOR
>> +
>> +config PPS_GENERATOR_DUMMY
>> +        tristate "Dummy PPS generator (Testing generator, use for debug)"
>> +        help
>> +          If you say yes here you get support for a PPS debugging generator
>> +          (which actual generates no PPS signal at all).
>> +
>> +          This driver can also be built as a module.  If so, the module
>> +          will be called pps_gen-dummy.
> 
> Put the dummy driver in a separate patch please, it doesn't belong with
> the core changes.

Done.

>>   
>>   config PPS_GENERATOR_PARPORT
>>   	tristate "Parallel port PPS signal generator"
>> @@ -12,3 +27,5 @@ config PPS_GENERATOR_PARPORT
>>   	  If you say yes here you get support for a PPS signal generator which
>>   	  utilizes STROBE pin of a parallel port to send PPS signals. It uses
>>   	  parport abstraction layer and hrtimers to precisely control the signal.
>> +
>> +endif # PPS_GENERATOR
>> diff --git a/drivers/pps/generators/Makefile b/drivers/pps/generators/Makefile
>> index 2589fd0f2481..dc1aa5a4688b 100644
>> --- a/drivers/pps/generators/Makefile
>> +++ b/drivers/pps/generators/Makefile
>> @@ -3,6 +3,10 @@
>>   # Makefile for PPS generators.
>>   #
>>   
>> +pps_gen_core-y			:= pps_gen.o sysfs.o
>> +obj-$(CONFIG_PPS_GENERATOR)	:= pps_gen_core.o
>> +
>> +obj-$(CONFIG_PPS_GENERATOR_DUMMY)   += pps_gen-dummy.o
>>   obj-$(CONFIG_PPS_GENERATOR_PARPORT) += pps_gen_parport.o
>>   
>>   ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
>> diff --git a/drivers/pps/generators/pps_gen-dummy.c b/drivers/pps/generators/pps_gen-dummy.c
>> new file mode 100644
>> index 000000000000..2d257f3f9bf9
>> --- /dev/null
>> +++ b/drivers/pps/generators/pps_gen-dummy.c
>> @@ -0,0 +1,83 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * PPS dummy generator
>> + *
>> + * Copyright (C) 2024   Rodolfo Giometti <giometti@enneenne.com>
> 
> Why so many spaces after "2024"?

Fixed.

>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
> Why is this needed, drivers should only ever use dev_*() calls, never
> pr_*() calls.

Fixed.

>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/time.h>
>> +#include <linux/pps_gen_kernel.h>
>> +
>> +/*
>> + * Global variables
>> + */
>> +
>> +static struct pps_gen_device *pps_gen;
> 
> That's by definition, static, not global :)

Fixed.

> Also, why is it needed at all?

I need a reference within module_exit() to the device created into module_init().

>> +
>> +/*
>> + * PPS Generator methods
>> + */
>> +
>> +static int pps_gen_dummy_get_time(struct pps_gen_device *pps_gen,
>> +                                        struct timespec64 *time)
>> +{
>> +	struct system_time_snapshot snap;
>> +
>> +	ktime_get_snapshot(&snap);
>> +	*time = ktime_to_timespec64(snap.real);
>> +
>> +	return 0;
>> +}
>> +
>> +static int pps_gen_dummy_enable(struct pps_gen_device *pps_gen, bool enable)
>> +{
>> +	/* always enabled */
>> +	return 0;
>> +}
>> +
>> +/*
>> + * The PPS info struct
>> + */
>> +
>> +static struct pps_gen_source_info pps_gen_dummy_info = {
>> +        .name			= "dummy",
>> +	.use_system_clock	= true,
>> +	.get_time		= pps_gen_dummy_get_time,
>> +	.enable			= pps_gen_dummy_enable,
>> +};
>> +
>> +/*
>> + * Module staff
>> + */
>> +
>> +static void __exit pps_gen_dummy_exit(void)
>> +{
>> +        dev_info(pps_gen->dev, "dummy PPS generator unregistered\n");
> 
> When drivers are working properly, they are quiet.

Fixed.

>> +
>> +        pps_gen_unregister_source(pps_gen);
>> +}
>> +
>> +static int __init pps_gen_dummy_init(void)
>> +{
>> +        pps_gen = pps_gen_register_source(&pps_gen_dummy_info);
>> +        if (IS_ERR(pps_gen)) {
>> +                pr_err("cannot register PPS generator\n");
>> +                return PTR_ERR(pps_gen);
>> +        }
>> +
>> +        dev_info(pps_gen->dev, "dummy PPS generator registered\n");
> 
> Again, quiet...

Fixed.

>> +
>> +        return 0;
>> +}
>> +
>> +module_init(pps_gen_dummy_init);
>> +module_exit(pps_gen_dummy_exit);
>> +
>> +MODULE_AUTHOR("Rodolfo Giometti <giometti@enneenne.com>");
>> +MODULE_DESCRIPTION("LinuxPPS dummy generator");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/pps/generators/pps_gen.c b/drivers/pps/generators/pps_gen.c
>> new file mode 100644
>> index 000000000000..40b05087b4b4
>> --- /dev/null
>> +++ b/drivers/pps/generators/pps_gen.c
>> @@ -0,0 +1,283 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * PPS generators core file
>> + *
>> + * Copyright (C) 2024   Rodolfo Giometti <giometti@enneenne.com>
> 
> Again spaces.

Fixed.

>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
> Again, not needed.

Fixed.

>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/sched.h>
>> +#include <linux/time.h>
>> +#include <linux/timex.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/idr.h>
>> +#include <linux/mutex.h>
>> +#include <linux/cdev.h>
> 
> Why a cdev?

PPS sources have related cdevs, so by analogy PPS generators also have them.

>> +#include <linux/fs.h>
>> +#include <linux/pps_gen_kernel.h>
>> +#include <linux/slab.h>
>> +
>> +/*
>> + * Local variables
>> + */
>> +
>> +static int pps_gen_major;
>> +static struct class *pps_gen_class;
>> +
>> +static DEFINE_MUTEX(pps_gen_idr_lock);
>> +static DEFINE_IDR(pps_gen_idr);
>> +
>> +/*
>> + * Char device methods
>> + */
>> +
>> +static long pps_gen_cdev_ioctl(struct file *file,
>> +		unsigned int cmd, unsigned long arg)
>> +{
>> +	struct pps_gen_device *pps_gen = file->private_data;
>> +	unsigned int __user *uiuarg = (unsigned int __user *) arg;
>> +	unsigned int status;
>> +	int ret;
>> +
>> +	switch (cmd) {
>> +	case PPS_GEN_SETENABLE:
>> +		dev_dbg(pps_gen->dev, "PPS_GEN_SETENABLE\n");
>> +
>> +		ret = get_user(status, uiuarg);
>> +		if (ret)
>> +			return -EFAULT;
>> +
>> +		ret = pps_gen->info.enable(pps_gen, status);
>> +		if (ret)
>> +			return ret;
>> +		pps_gen->enabled = status;
>> +
>> +		break;
>> +
>> +	default:
>> +		return -ENOTTY;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> Why is there an ioctl call?  That's a totally different user/kernel api
> than sysfs, why do you have 2?

PPS sources have ioctl()s, so by analogy... :)

>> +
>> +#ifdef CONFIG_COMPAT
>> +static long pps_gen_cdev_compat_ioctl(struct file *file,
>> +		unsigned int cmd, unsigned long arg)
>> +{
>> +	cmd = _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(void *));
>> +	return pps_gen_cdev_ioctl(file, cmd, arg);
>> +}
>> +#else
>> +#define pps_gen_cdev_compat_ioctl	NULL
>> +#endif
>> +
>> +static struct pps_gen_device *pps_gen_idr_get(unsigned long id)
>> +{
>> +	struct pps_gen_device *pps_gen;
>> +
>> +	mutex_lock(&pps_gen_idr_lock);
>> +	pps_gen = idr_find(&pps_gen_idr, id);
>> +	if (pps_gen)
>> +		kobject_get(&pps_gen->dev->kobj);
>> +
>> +	mutex_unlock(&pps_gen_idr_lock);
> 
> Doesn't an idr have a lock in it?  I can never remember...

As far as I know we must use a mutex...

>> +	return pps_gen;
>> +}
>> +
>> +static int pps_gen_cdev_open(struct inode *inode, struct file *file)
>> +{
>> +	struct pps_gen_device *pps_gen = pps_gen_idr_get(iminor(inode));
>> +
>> +	if (!pps_gen)
>> +		return -ENODEV;
>> +
>> +	file->private_data = pps_gen;
>> +	return 0;
>> +}
>> +
>> +static int pps_gen_cdev_release(struct inode *inode, struct file *file)
>> +{
>> +	struct pps_gen_device *pps_gen = file->private_data;
>> +
>> +	WARN_ON(pps_gen->id != iminor(inode));
> 
> If this can never happen, don't add this line.  If it can happen, then
> handle it properly.  Either way, don't reboot a box because it happened.

Fixed.

>> +	kobject_put(&pps_gen->dev->kobj);
> 
> Messing with a kobject reference directly from a device feels wrong and
> should never be done.

I followed the suggestions in this patch whose look sane to me:

https://lore.kernel.org/lkml/fc5fe55c-422d-4e63-a5bd-8b6b2d3e6c62@enneenne.com/T/

>  Please use the proper apis.

Which API are you talking about? Can you please provide some advice?

> 
> 
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Char device stuff
>> + */
>> +
>> +static const struct file_operations pps_gen_cdev_fops = {
>> +	.owner		= THIS_MODULE,
>> +	.compat_ioctl	= pps_gen_cdev_compat_ioctl,
> 
> Why compat for a new ioctl?  Why not write it properly to not need it?

Fixed.

> 
>> +	.unlocked_ioctl	= pps_gen_cdev_ioctl,
>> +	.open		= pps_gen_cdev_open,
>> +	.release	= pps_gen_cdev_release,
>> +};
>> +
>> +static void pps_gen_device_destruct(struct device *dev)
>> +{
>> +	struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
>> +
>> +	pr_debug("deallocating pps-gen%d\n", pps_gen->id);
> 
> ftrace is your friend.

I see, but we can also use pr_debug()! :P

> 
>> +	kfree(dev);
>> +	kfree(pps_gen);
>> +}
>> +
>> +static int pps_gen_register_cdev(struct pps_gen_device *pps_gen)
>> +{
>> +	int err;
>> +	dev_t devt;
>> +
>> +	mutex_lock(&pps_gen_idr_lock);
>> +
>> +	err = idr_alloc(&pps_gen_idr, pps_gen, 0, PPS_GEN_MAX_SOURCES,
>> +					GFP_KERNEL);
>> +	if (err < 0) {
>> +		if (err == -ENOSPC) {
>> +			pr_err("%s: too many PPS sources in the system\n",
>> +			       pps_gen->info.name);
>> +			err = -EBUSY;
>> +		}
>> +		goto out_unlock;
>> +	}
>> +	pps_gen->id = err;
>> +
>> +	devt = MKDEV(pps_gen_major, pps_gen->id);
>> +	pps_gen->dev = device_create(pps_gen_class, pps_gen->info.parent, devt,
>> +					pps_gen, "pps-gen%d", pps_gen->id);
>> +	if (IS_ERR(pps_gen->dev)) {
>> +		err = PTR_ERR(pps_gen->dev);
>> +		goto free_idr;
>> +	}
>> +
>> +	/* Override the release function with our own */
>> +	pps_gen->dev->release = pps_gen_device_destruct;
>> +
>> +	pr_debug("generator %s got cdev (%d:%d)\n",
>> +			pps_gen->info.name, pps_gen_major, pps_gen->id);
> 
> Why not dev_dbg()?

Honestly I prefer pr_debug() because this message is not device related, but it 
is geneated by the PPS subsystem.

>> +
>> +	kobject_get(&pps_gen->dev->kobj);
>> +	mutex_unlock(&pps_gen_idr_lock);
>> +	return 0;
>> +
>> +free_idr:
>> +	idr_remove(&pps_gen_idr, pps_gen->id);
>> +out_unlock:
>> +	mutex_unlock(&pps_gen_idr_lock);
>> +	return err;
>> +}
>> +
>> +static void pps_gen_unregister_cdev(struct pps_gen_device *pps_gen)
>> +{
>> +	pr_debug("unregistering pps-gen%d\n", pps_gen->id);
>> +	device_destroy(pps_gen_class, pps_gen->dev->devt);
>> +
>> +	/* Now we can release the ID for re-use */
>> +	mutex_lock(&pps_gen_idr_lock);
>> +	idr_remove(&pps_gen_idr, pps_gen->id);
>> +	kobject_put(&pps_gen->dev->kobj);
>> +	mutex_unlock(&pps_gen_idr_lock);
>> +}
>> +
>> +/*
>> + * Exported functions
>> + */
>> +
>> +/* pps_gen_register_source - add a PPS generator in the system
>> + * @info: the PPS generator info struct
>> + *
>> + * The function returns, in case of success, the PPS generaor device. Otherwise
>> + * ERR_PTR(errno).
>> + */
>> +
>> +struct pps_gen_device *pps_gen_register_source(struct pps_gen_source_info *info)
>> +{
>> +        struct pps_gen_device *pps_gen;
>> +        int err;
>> +
>> +        pps_gen = kzalloc(sizeof(struct pps_gen_device), GFP_KERNEL);
>> +        if (pps_gen == NULL) {
>> +                err = -ENOMEM;
>> +                goto pps_gen_register_source_exit;
>> +        }
>> +        pps_gen->info = *info;
>> +	pps_gen->enabled = false;
> 
> Whitespace is all messed up here, did you run checkpatch?

Fixed.

>> +
>> +        /* Create the char device */
>> +        err = pps_gen_register_cdev(pps_gen);
>> +        if (err < 0) {
>> +                pr_err("%s: unable to create char device\n",
>> +                                        info->name);
>> +                goto kfree_pps_gen;
>> +        }
>> +
>> +        dev_info(pps_gen->dev, "new PPS generator %s\n", info->name);
> 
> Again, quiet...

Fixed.

>> +
>> +        return pps_gen;
>> +
>> +kfree_pps_gen:
>> +        kfree(pps_gen);
>> +
>> +pps_gen_register_source_exit:
>> +        pr_err("%s: unable to register generaor\n", info->name);
> 
> dev_err()?
> 
>> +
>> +        return ERR_PTR(err);
>> +}
>> +EXPORT_SYMBOL(pps_gen_register_source);
> 
> I have to ask, why not EXPORT_SYMBOL_GPL()?

All PPS symbols are defined as EXPORT_SYMBOL()...

>> +
>> +/* pps_gen_unregister_source - remove a PPS generator from the system
>> + * @pps_gen: the PPS generator
>> + */
>> +
>> +void pps_gen_unregister_source(struct pps_gen_device *pps_gen)
>> +{
>> +        pps_gen_unregister_cdev(pps_gen);
>> +}
>> +EXPORT_SYMBOL(pps_gen_unregister_source);
>> +
>> +/*
>> + * Module stuff
>> + */
>> +
>> +static void __exit pps_gen_exit(void)
>> +{
>> +	class_destroy(pps_gen_class);
>> +	__unregister_chrdev(pps_gen_major, 0, PPS_GEN_MAX_SOURCES, "pps-gen");
> 
> Why the __ version?  Are you sure?

Again, see the above patch.

>> +}
>> +
>> +static int __init pps_gen_init(void)
>> +{
>> +	pps_gen_class = class_create("pps-gen");
>> +	if (IS_ERR(pps_gen_class)) {
>> +		pr_err("failed to allocate class\n");
>> +		return PTR_ERR(pps_gen_class);
>> +	}
>> +	pps_gen_class->dev_groups = pps_gen_groups;
>> +
>> +	pps_gen_major = __register_chrdev(0, 0, PPS_GEN_MAX_SOURCES, "pps-gen",
>> +				      &pps_gen_cdev_fops);
> 
> Again, why __?

Ditto.

>> +	if (pps_gen_major < 0) {
>> +		pr_err("failed to allocate char device region\n");
>> +		goto remove_class;
>> +	}
>> +
>> +	return 0;
>> +
>> +remove_class:
>> +	class_destroy(pps_gen_class);
>> +	return pps_gen_major;
>> +}
>> +
>> +subsys_initcall(pps_gen_init);
>> +module_exit(pps_gen_exit);
>> +
>> +MODULE_AUTHOR("Rodolfo Giometti <giometti@enneenne.com>");
>> +MODULE_DESCRIPTION("LinuxPPS generators support");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/pps/generators/sysfs.c b/drivers/pps/generators/sysfs.c
>> new file mode 100644
>> index 000000000000..247030d138e1
>> --- /dev/null
>> +++ b/drivers/pps/generators/sysfs.c
>> @@ -0,0 +1,89 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * PPS generators sysfs support
>> + *
>> + * Copyright (C) 2024   Rodolfo Giometti <giometti@enneenne.com>
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
> again...

Fixed.

>> +
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/string.h>
>> +#include <linux/pps_gen_kernel.h>
>> +
>> +/*
>> + * Attribute functions
>> + */
>> +
>> +static ssize_t system_show(struct device *dev, struct device_attribute *attr,
>> +				char *buf)
>> +{
>> +        struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
> 
> Again whitespace...

Fixed.

>> +
>> +        return sysfs_emit(buf, "%d\n", pps_gen->info.use_system_clock);
>> +}
>> +static DEVICE_ATTR_RO(system);
>> +
>> +static ssize_t time_show(struct device *dev, struct device_attribute *attr,
>> +				char *buf)
>> +{
>> +        struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
>> +	struct timespec64 time;
>> +	int ret;
>> +
>> +        ret = pps_gen->info.get_time(pps_gen, &time);
>> +        if (ret)
>> +                return ret;
>> +
>> +        return sysfs_emit(buf, "%llu %09lu\n", time.tv_sec, time.tv_nsec);
>> +}
>> +static DEVICE_ATTR_RO(time);
>> +
>> +static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
>> +				const char *buf, size_t count)
>> +{
>> +        struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
>> +	bool status;
>> +	unsigned int enable;
>> +        int ret;
>> +
>> +	ret = sscanf(buf, "%u", &enable);
>> +        if (ret != 1)
>> +		return -EINVAL;
>> +	status = !!enable;
>> +
>> +        ret = pps_gen->info.enable(pps_gen, status);
>> +        if (ret)
>> +                return ret;
>> +	pps_gen->enabled = status;
>> +
>> +        return count;
>> +}
>> +static DEVICE_ATTR_WO(enable);
>> +
>> +static ssize_t name_show(struct device *dev, struct device_attribute *attr,
>> +                         char *buf)
>> +{
>> +        struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
>> +
>> +        return sysfs_emit(buf, "%s\n", pps_gen->info.name);
> 
> Why have a separate name? 

This can be useful in order to distinguish between different PPS generators in 
the system.

> That shouldn't matter at all.  If it does
> matter, than link to the device that created it properly, don't make up
> yet another name for your device.

I'm not sure to understand what you mean... The "name" attribute is just a label 
which the userspace my (or my not) use to know which generator to enable or not.

>> +}
>> +static DEVICE_ATTR_RO(name);
>> +
>> +static struct attribute *pps_gen_attrs[] = {
>> +        &dev_attr_enable.attr,
>> +        &dev_attr_name.attr,
>> +        &dev_attr_time.attr,
>> +        &dev_attr_system.attr,
>> +        NULL,
>> +};
>> +
>> +static const struct attribute_group pps_gen_group = {
>> +        .attrs = pps_gen_attrs,
>> +};
>> +
>> +const struct attribute_group *pps_gen_groups[] = {
>> +        &pps_gen_group,
>> +        NULL,
>> +};
>> diff --git a/include/linux/pps_gen_kernel.h b/include/linux/pps_gen_kernel.h
>> new file mode 100644
>> index 000000000000..5513415b53ec
>> --- /dev/null
>> +++ b/include/linux/pps_gen_kernel.h
>> @@ -0,0 +1,57 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * PPS generator API kernel header
>> + *
>> + * Copyright (C) 2024   Rodolfo Giometti <giometti@enneenne.com>
>> + */
>> +
>> +#ifndef LINUX_PPS_GEN_KERNEL_H
>> +#define LINUX_PPS_GEN_KERNEL_H
>> +
>> +#include <linux/pps_gen.h>
>> +#include <linux/cdev.h>
>> +#include <linux/device.h>
>> +
>> +/*
>> + * Global defines
>> + */
>> +
>> +struct pps_gen_device;
>> +
>> +/* The specific PPS source info */
>> +struct pps_gen_source_info {
>> +	char name[PPS_GEN_MAX_NAME_LEN];	/* symbolic name */
>> +	bool use_system_clock;
>> +
>> +	int (*get_time)(struct pps_gen_device *pps_gen,
>> +					struct timespec64 *time);
>> +	int (*enable)(struct pps_gen_device *pps_gen, bool enable);
>> +
>> +	struct module *owner;
>> +	struct device *parent;			/* for device_create */
>> +};
>> +
>> +/* The main struct */
>> +struct pps_gen_device {
>> +	struct pps_gen_source_info info;	/* PSS generator info */
>> +	bool enabled;				/* PSS generator status */
>> +
>> +	unsigned int id;			/* PPS generator unique ID */
>> +	struct device *dev;
> 
> Why not be a real device? What is this a pointer to?

This is a pointer to the device created within the pps_gen_register_cdev().

>> +};
> 
> This structure can be private, right?

Yes. Just the PPS subsystem uses it.

>> +
>> +/*
>> + * Global variables
>> + */
>> +
>> +extern const struct attribute_group *pps_gen_groups[];
> 
> Why is this global?

It is used in drivers/pps/generators/pps_gen.c and referenced in 
drivers/pps/generators/sysfs.c.

>> +
>> +/*
>> + * Exported functions
>> + */
>> +
>> +extern struct pps_gen_device *pps_gen_register_source(
>> +		struct pps_gen_source_info *info);
>> +extern void pps_gen_unregister_source(struct pps_gen_device *pps_gen);
>> +
>> +#endif /* LINUX_PPS_GEN_KERNEL_H */
>> diff --git a/include/uapi/linux/pps_gen.h b/include/uapi/linux/pps_gen.h
>> new file mode 100644
>> index 000000000000..7b6f50fcab8c
>> --- /dev/null
>> +++ b/include/uapi/linux/pps_gen.h
>> @@ -0,0 +1,35 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> 
> I have to ask, why "GPL-2.0+"?

Fixed.

>> +/*
>> + * PPS generator API header
>> + *
>> + * Copyright (C) 2024   Rodolfo Giometti <giometti@enneenne.com>
>> + *
>> + *   This program is free software; you can redistribute it and/or modify
>> + *   it under the terms of the GNU General Public License as published by
>> + *   the Free Software Foundation; either version 2 of the License, or
>> + *   (at your option) any later version.
>> + *
>> + *   This program is distributed in the hope that it will be useful,
>> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *   GNU General Public License for more details.
>> + *
>> + *   You should have received a copy of the GNU General Public License
>> + *   along with this program; if not, write to the Free Software
>> + *   Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> 
> License boilerplate should be removed.

Fixed.

>> + */
>> +
>> +
>> +#ifndef _PPS_GEN_H_
>> +#define _PPS_GEN_H_
>> +
>> +#include <linux/types.h>
>> +
>> +#define PPS_GEN_MAX_SOURCES	16		/* should be enough... */
> 
> What is this for?  Who is using it in userspace?

Fixed.

>> +#define PPS_GEN_MAX_NAME_LEN	32
> 
> Why is this exported to userspace?

Fixed.

>> +
>> +#include <linux/ioctl.h>
>> +
>> +#define PPS_GEN_SETENABLE	_IOW('g', 0xa1, unsigned int *)
> 
> Documentation for this new ioctl?

Where should I add it? Can you please provide some advice?

Thanks a lot for your suggestions! I'm gping to provide a first release for this 
patchset shortly.

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming

Re: [RFC 2/3] Documentation pps.rst: add PPS generators documentation

[Rodolfo Giometti]

On 08/10/24 17:43, Greg KH wrote:
> On Tue, Oct 08, 2024 at 03:50:32PM +0200, Rodolfo Giometti wrote:
>> This patch adds some examples about how to register a new PPS
>> generator in the system, and how to manage it.
>>
>> Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
>> ---
>>   Documentation/driver-api/pps.rst | 40 ++++++++++++++++++++++++++++++++
> 
> All of this can go into the .c file and autogenerated there, no need for
> a separate .rst file that will quickly get out-of-date.

I see. I'm going to add proper documentation within .c files. But since some 
references about PPS generators are also present in this file, I think it would 
be wise to add some notes about the new interface...

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming

Re: [RFC 3/3] Documentation ABI: add PPS generators documentaion

[Rodolfo Giometti]

On 08/10/24 17:43, Greg KH wrote:
> On Tue, Oct 08, 2024 at 03:50:33PM +0200, Rodolfo Giometti wrote:
>> This patch adds the documentation for the ABI between the Linux kernel
>> and userspace regarding the PPS generators.
>>
>> Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
>> ---
>>   Documentation/ABI/testing/sysfs-pps-gen | 44 +++++++++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-pps-gen
>>
>> diff --git a/Documentation/ABI/testing/sysfs-pps-gen b/Documentation/ABI/testing/sysfs-pps-gen
>> new file mode 100644
>> index 000000000000..9ad066cb3ce5
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-pps-gen
>> @@ -0,0 +1,44 @@
>> +What:		/sys/class/pps-gen/
>> +Date:		October 2024
>> +Contact:	Rodolfo Giometti <giometti@enneenne.com>
>> +Description:
>> +		The /sys/class/pps-gen/ directory will contain files and
>> +		directories that will provide a unified interface to
>> +		the PPS generators.
>> +
>> +What:		/sys/class/pps-gen/pps-genX/
>> +Date:		October 2024
>> +Contact:	Rodolfo Giometti <giometti@enneenne.com>
>> +Description:
>> +		The /sys/class/pps-gen/pps-genX/ directory is related to X-th
>> +		PPS generator into the system. Each directory will
>> +		contain files to manage and control its PPS generator.
>> +
>> +What:		/sys/class/pps-gen/pps-genX/enable
>> +Date:		October 2024
>> +Contact:	Rodolfo Giometti <giometti@enneenne.com>
>> +Description:
>> +		This write-only file enables or disables generation of the
>> +		PPS signal.
>> +
>> +What:		/sys/class/pps-gen/pps-genX/name
>> +Date:		October 2024
>> +Contact:	Rodolfo Giometti <giometti@enneenne.com>
>> +Description:
>> +		This read-only file reports the name of the X-th generator.
> 
> Again, why a name?  What is that for?

This can be useful in order to distinguish between different PPS generators in 
the system.

For example, the PARPORT generator is not very precise, and userspace 
applications should be able to know which generator corresponds to the device 
/dev/pps-gen0 or /dev/pps-gen1, etc.

> 
>> +
>> +What:		/sys/class/pps-gen/pps-genX/system
>> +Date:		October 2024
>> +Contact:	Rodolfo Giometti <giometti@enneenne.com>
>> +Description:
>> +		This read-only file returns "1" if the generator takes the
>> +		timing from the system clock, while it returns "0" if not
>> +		(i.e. from a peripheral device clock).
>> +
>> +What:		/sys/class/pps-gen/pps-genX/time
>> +Date:		October 2024
>> +Contact:	Rodolfo Giometti <giometti@enneenne.com>
>> +Description:
>> +		This read-only file contains the current time stored into the
>> +		generator clock as two integers representing the current time
>> +		seconds and nanoseconds.
> 
> Trailing whitespace, please always run checkpatch.pl.

Fixed.

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming

Re: [RFC 1/3] drivers pps: add PPS generators support

[Rodolfo Giometti]

On 09/10/24 04:49, Hall, Christopher S wrote:
> Hi Rudolfo,
> 
>> -----Original Message-----
>> From: Rodolfo Giometti <giometti@enneenne.com>
>> Sent: Tuesday, October 08, 2024 6:51 AM
>> To: linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org
> 
>> Sometimes one needs to be able not only to catch PPS signals but to
>> produce them also. For example, running a distributed simulation,
>> which requires computers' clock to be synchronized very tightly.
>>
>> This patch adds PPS generators class in order to have a well-defined
>> interface for these devices.
>>
>> Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
>> ---
>>   drivers/pps/Makefile                   |   3 +-
>>   drivers/pps/generators/Kconfig         |  19 +-
>>   drivers/pps/generators/Makefile        |   4 +
>>   drivers/pps/generators/pps_gen-dummy.c |  83 ++++++++
>>   drivers/pps/generators/pps_gen.c       | 283 +++++++++++++++++++++++++
>>   drivers/pps/generators/sysfs.c         |  89 ++++++++
>>   include/linux/pps_gen_kernel.h         |  57 +++++
>>   include/uapi/linux/pps_gen.h           |  35 +++
>>   8 files changed, 571 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/pps/generators/pps_gen-dummy.c
>>   create mode 100644 drivers/pps/generators/pps_gen.c
>>   create mode 100644 drivers/pps/generators/sysfs.c
>>   create mode 100644 include/linux/pps_gen_kernel.h
>>   create mode 100644 include/uapi/linux/pps_gen.h
> 
> This looks pretty good to me. I would like to see an alarm callback. We are able
> to detect a missed event and rather than stopping inexplicably or writing to the
> system log, it would be better to be able to notify an application directly.
> 
> Off the top of my head, something like:
> 
> void pps_gen_alarm(pps_gen_device *pps_gen) {
> 	pps_gen->alarm = 1;
> 	sysfs_notify(pps_gen->dev->kobj, NULL, "alarm");
> }
> 
> The device is reset by disabling/enabling and this resets the alarm flag.
> 
> Could we add something like this?

Yes, of course. But it's better to provide a proper patch against my patchset V1 
(I'm going to anticipate them to you shortly), since I'm not sure to understand 
what you need.

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming

Re: [RFC 2/3] Documentation pps.rst: add PPS generators documentation

[Greg KH]

On Wed, Oct 09, 2024 at 10:48:18AM +0200, Rodolfo Giometti wrote:
> On 08/10/24 17:43, Greg KH wrote:
> > On Tue, Oct 08, 2024 at 03:50:32PM +0200, Rodolfo Giometti wrote:
> > > This patch adds some examples about how to register a new PPS
> > > generator in the system, and how to manage it.
> > > 
> > > Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
> > > ---
> > >   Documentation/driver-api/pps.rst | 40 ++++++++++++++++++++++++++++++++
> > 
> > All of this can go into the .c file and autogenerated there, no need for
> > a separate .rst file that will quickly get out-of-date.
> 
> I see. I'm going to add proper documentation within .c files. But since some
> references about PPS generators are also present in this file, I think it
> would be wise to add some notes about the new interface...

Why not just move all of the documentation into the .c files?

thanks,

greg k-h

Re: [RFC 3/3] Documentation ABI: add PPS generators documentaion

[Greg KH]

On Wed, Oct 09, 2024 at 10:48:23AM +0200, Rodolfo Giometti wrote:
> On 08/10/24 17:43, Greg KH wrote:
> > On Tue, Oct 08, 2024 at 03:50:33PM +0200, Rodolfo Giometti wrote:
> > > This patch adds the documentation for the ABI between the Linux kernel
> > > and userspace regarding the PPS generators.
> > > 
> > > Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
> > > ---
> > >   Documentation/ABI/testing/sysfs-pps-gen | 44 +++++++++++++++++++++++++
> > >   1 file changed, 44 insertions(+)
> > >   create mode 100644 Documentation/ABI/testing/sysfs-pps-gen
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-pps-gen b/Documentation/ABI/testing/sysfs-pps-gen
> > > new file mode 100644
> > > index 000000000000..9ad066cb3ce5
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-pps-gen
> > > @@ -0,0 +1,44 @@
> > > +What:		/sys/class/pps-gen/
> > > +Date:		October 2024
> > > +Contact:	Rodolfo Giometti <giometti@enneenne.com>
> > > +Description:
> > > +		The /sys/class/pps-gen/ directory will contain files and
> > > +		directories that will provide a unified interface to
> > > +		the PPS generators.
> > > +
> > > +What:		/sys/class/pps-gen/pps-genX/
> > > +Date:		October 2024
> > > +Contact:	Rodolfo Giometti <giometti@enneenne.com>
> > > +Description:
> > > +		The /sys/class/pps-gen/pps-genX/ directory is related to X-th
> > > +		PPS generator into the system. Each directory will
> > > +		contain files to manage and control its PPS generator.
> > > +
> > > +What:		/sys/class/pps-gen/pps-genX/enable
> > > +Date:		October 2024
> > > +Contact:	Rodolfo Giometti <giometti@enneenne.com>
> > > +Description:
> > > +		This write-only file enables or disables generation of the
> > > +		PPS signal.
> > > +
> > > +What:		/sys/class/pps-gen/pps-genX/name
> > > +Date:		October 2024
> > > +Contact:	Rodolfo Giometti <giometti@enneenne.com>
> > > +Description:
> > > +		This read-only file reports the name of the X-th generator.
> > 
> > Again, why a name?  What is that for?
> 
> This can be useful in order to distinguish between different PPS generators
> in the system.
> 
> For example, the PARPORT generator is not very precise, and userspace
> applications should be able to know which generator corresponds to the
> device /dev/pps-gen0 or /dev/pps-gen1, etc.

That's what the device symlink in the directory is for, no need to pick
yet-another-random-name to have to read from a file :)

thanks,

greg k-h

Re: [RFC 1/3] drivers pps: add PPS generators support

[Greg KH]

On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
> > > +	kobject_put(&pps_gen->dev->kobj);
> > 
> > Messing with a kobject reference directly from a device feels wrong and
> > should never be done.
> 
> I followed the suggestions in this patch whose look sane to me:
> 
> https://lore.kernel.org/lkml/fc5fe55c-422d-4e63-a5bd-8b6b2d3e6c62@enneenne.com/T/

That patch is wrong.

> >  Please use the proper apis.
> 
> Which API are you talking about? Can you please provide some advice?

get_device()

You are working on devices, NOT a raw kobject, no driver should EVER be
calling into a kobject function or a sysfs function, there should be
driver core functions for everything you need to do.

> 
> > 
> > 
> > > +	return 0;
> > > +}
> > > +
> > > +/*
> > > + * Char device stuff
> > > + */
> > > +
> > > +static const struct file_operations pps_gen_cdev_fops = {
> > > +	.owner		= THIS_MODULE,
> > > +	.compat_ioctl	= pps_gen_cdev_compat_ioctl,
> > 
> > Why compat for a new ioctl?  Why not write it properly to not need it?
> 
> Fixed.
> 
> > 
> > > +	.unlocked_ioctl	= pps_gen_cdev_ioctl,
> > > +	.open		= pps_gen_cdev_open,
> > > +	.release	= pps_gen_cdev_release,
> > > +};
> > > +
> > > +static void pps_gen_device_destruct(struct device *dev)
> > > +{
> > > +	struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
> > > +
> > > +	pr_debug("deallocating pps-gen%d\n", pps_gen->id);
> > 
> > ftrace is your friend.
> 
> I see, but we can also use pr_debug()! :P
> 
> > 
> > > +	kfree(dev);
> > > +	kfree(pps_gen);
> > > +}
> > > +
> > > +static int pps_gen_register_cdev(struct pps_gen_device *pps_gen)
> > > +{
> > > +	int err;
> > > +	dev_t devt;
> > > +
> > > +	mutex_lock(&pps_gen_idr_lock);
> > > +
> > > +	err = idr_alloc(&pps_gen_idr, pps_gen, 0, PPS_GEN_MAX_SOURCES,
> > > +					GFP_KERNEL);
> > > +	if (err < 0) {
> > > +		if (err == -ENOSPC) {
> > > +			pr_err("%s: too many PPS sources in the system\n",
> > > +			       pps_gen->info.name);
> > > +			err = -EBUSY;
> > > +		}
> > > +		goto out_unlock;
> > > +	}
> > > +	pps_gen->id = err;
> > > +
> > > +	devt = MKDEV(pps_gen_major, pps_gen->id);
> > > +	pps_gen->dev = device_create(pps_gen_class, pps_gen->info.parent, devt,
> > > +					pps_gen, "pps-gen%d", pps_gen->id);
> > > +	if (IS_ERR(pps_gen->dev)) {
> > > +		err = PTR_ERR(pps_gen->dev);
> > > +		goto free_idr;
> > > +	}
> > > +
> > > +	/* Override the release function with our own */
> > > +	pps_gen->dev->release = pps_gen_device_destruct;
> > > +
> > > +	pr_debug("generator %s got cdev (%d:%d)\n",
> > > +			pps_gen->info.name, pps_gen_major, pps_gen->id);
> > 
> > Why not dev_dbg()?
> 
> Honestly I prefer pr_debug() because this message is not device related, but
> it is geneated by the PPS subsystem.

But you have a device, please use it!  Otherwise it's impossible to
track back what is going on to what device in the system.

> > > +static ssize_t name_show(struct device *dev, struct device_attribute *attr,
> > > +                         char *buf)
> > > +{
> > > +        struct pps_gen_device *pps_gen = dev_get_drvdata(dev);
> > > +
> > > +        return sysfs_emit(buf, "%s\n", pps_gen->info.name);
> > 
> > Why have a separate name?
> 
> This can be useful in order to distinguish between different PPS generators
> in the system.

Again, rely on the backing device structure for this (i.e. the symlink
in sysfs), you do not need to duplicate existing infrastructure.

> > That shouldn't matter at all.  If it does
> > matter, than link to the device that created it properly, don't make up
> > yet another name for your device.
> 
> I'm not sure to understand what you mean... The "name" attribute is just a
> label which the userspace my (or my not) use to know which generator to
> enable or not.

Again, it's tied to the device in the system, don't list that same thing
again.

> 
> > > +}
> > > +static DEVICE_ATTR_RO(name);
> > > +
> > > +static struct attribute *pps_gen_attrs[] = {
> > > +        &dev_attr_enable.attr,
> > > +        &dev_attr_name.attr,
> > > +        &dev_attr_time.attr,
> > > +        &dev_attr_system.attr,
> > > +        NULL,
> > > +};
> > > +
> > > +static const struct attribute_group pps_gen_group = {
> > > +        .attrs = pps_gen_attrs,
> > > +};
> > > +
> > > +const struct attribute_group *pps_gen_groups[] = {
> > > +        &pps_gen_group,
> > > +        NULL,
> > > +};
> > > diff --git a/include/linux/pps_gen_kernel.h b/include/linux/pps_gen_kernel.h
> > > new file mode 100644
> > > index 000000000000..5513415b53ec
> > > --- /dev/null
> > > +++ b/include/linux/pps_gen_kernel.h
> > > @@ -0,0 +1,57 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * PPS generator API kernel header
> > > + *
> > > + * Copyright (C) 2024   Rodolfo Giometti <giometti@enneenne.com>
> > > + */
> > > +
> > > +#ifndef LINUX_PPS_GEN_KERNEL_H
> > > +#define LINUX_PPS_GEN_KERNEL_H
> > > +
> > > +#include <linux/pps_gen.h>
> > > +#include <linux/cdev.h>
> > > +#include <linux/device.h>
> > > +
> > > +/*
> > > + * Global defines
> > > + */
> > > +
> > > +struct pps_gen_device;
> > > +
> > > +/* The specific PPS source info */
> > > +struct pps_gen_source_info {
> > > +	char name[PPS_GEN_MAX_NAME_LEN];	/* symbolic name */
> > > +	bool use_system_clock;
> > > +
> > > +	int (*get_time)(struct pps_gen_device *pps_gen,
> > > +					struct timespec64 *time);
> > > +	int (*enable)(struct pps_gen_device *pps_gen, bool enable);
> > > +
> > > +	struct module *owner;
> > > +	struct device *parent;			/* for device_create */
> > > +};
> > > +
> > > +/* The main struct */
> > > +struct pps_gen_device {
> > > +	struct pps_gen_source_info info;	/* PSS generator info */
> > > +	bool enabled;				/* PSS generator status */
> > > +
> > > +	unsigned int id;			/* PPS generator unique ID */
> > > +	struct device *dev;
> > 
> > Why not be a real device? What is this a pointer to?
> 
> This is a pointer to the device created within the pps_gen_register_cdev().

Why isn't it a real cdev instead?

thanks,

greg k-h

Re: [RFC 1/3] drivers pps: add PPS generators support

[Greg KH]

On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
> > > +#ifdef CONFIG_COMPAT
> > > +static long pps_gen_cdev_compat_ioctl(struct file *file,
> > > +		unsigned int cmd, unsigned long arg)
> > > +{
> > > +	cmd = _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(void *));
> > > +	return pps_gen_cdev_ioctl(file, cmd, arg);
> > > +}
> > > +#else
> > > +#define pps_gen_cdev_compat_ioctl	NULL
> > > +#endif
> > > +
> > > +static struct pps_gen_device *pps_gen_idr_get(unsigned long id)
> > > +{
> > > +	struct pps_gen_device *pps_gen;
> > > +
> > > +	mutex_lock(&pps_gen_idr_lock);
> > > +	pps_gen = idr_find(&pps_gen_idr, id);
> > > +	if (pps_gen)
> > > +		kobject_get(&pps_gen->dev->kobj);
> > > +
> > > +	mutex_unlock(&pps_gen_idr_lock);
> > 
> > Doesn't an idr have a lock in it?  I can never remember...
> 
> As far as I know we must use a mutex...

If you do, someone will come along and remove it, please see:
	https://lore.kernel.org/r/b1fcc6707ec2b6309d50060fa52ccc2c892afde2.1728507153.git.christophe.jaillet@wanadoo.fr
as an example (with links that show it is not needed).

thanks,

greg k-h

Re: [RFC 1/3] drivers pps: add PPS generators support

[Rodolfo Giometti]

On 09/10/24 11:19, Greg KH wrote:
> On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
>>>> +	kobject_put(&pps_gen->dev->kobj);
>>>
>>> Messing with a kobject reference directly from a device feels wrong and
>>> should never be done.
>>
>> I followed the suggestions in this patch whose look sane to me:
>>
>> https://lore.kernel.org/lkml/fc5fe55c-422d-4e63-a5bd-8b6b2d3e6c62@enneenne.com/T/
> 
> That patch is wrong.

:(

>>>   Please use the proper apis.
>>
>> Which API are you talking about? Can you please provide some advice?
> 
> get_device()
> 
> You are working on devices, NOT a raw kobject, no driver should EVER be
> calling into a kobject function or a sysfs function, there should be
> driver core functions for everything you need to do.

OK, I'm going to provide a new RFC taking in account what you suggest.

Thanks,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming

Re: [RFC 2/3] Documentation pps.rst: add PPS generators documentation

[Rodolfo Giometti]

On 09/10/24 11:14, Greg KH wrote:
> On Wed, Oct 09, 2024 at 10:48:18AM +0200, Rodolfo Giometti wrote:
>> On 08/10/24 17:43, Greg KH wrote:
>>> On Tue, Oct 08, 2024 at 03:50:32PM +0200, Rodolfo Giometti wrote:
>>>> This patch adds some examples about how to register a new PPS
>>>> generator in the system, and how to manage it.
>>>>
>>>> Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
>>>> ---
>>>>    Documentation/driver-api/pps.rst | 40 ++++++++++++++++++++++++++++++++
>>>
>>> All of this can go into the .c file and autogenerated there, no need for
>>> a separate .rst file that will quickly get out-of-date.
>>
>> I see. I'm going to add proper documentation within .c files. But since some
>> references about PPS generators are also present in this file, I think it
>> would be wise to add some notes about the new interface...
> 
> Why not just move all of the documentation into the .c files?

I see, but this is another story. :-P

At the moment I prefer to just add a note here about the new interface, and 
later we can do what you suggest.

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming

Re: [RFC 1/3] drivers pps: add PPS generators support

[Rodolfo Giometti]

On 10/10/24 09:15, Greg KH wrote:
> On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
>>>> +#ifdef CONFIG_COMPAT
>>>> +static long pps_gen_cdev_compat_ioctl(struct file *file,
>>>> +		unsigned int cmd, unsigned long arg)
>>>> +{
>>>> +	cmd = _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(void *));
>>>> +	return pps_gen_cdev_ioctl(file, cmd, arg);
>>>> +}
>>>> +#else
>>>> +#define pps_gen_cdev_compat_ioctl	NULL
>>>> +#endif
>>>> +
>>>> +static struct pps_gen_device *pps_gen_idr_get(unsigned long id)
>>>> +{
>>>> +	struct pps_gen_device *pps_gen;
>>>> +
>>>> +	mutex_lock(&pps_gen_idr_lock);
>>>> +	pps_gen = idr_find(&pps_gen_idr, id);
>>>> +	if (pps_gen)
>>>> +		kobject_get(&pps_gen->dev->kobj);
>>>> +
>>>> +	mutex_unlock(&pps_gen_idr_lock);
>>>
>>> Doesn't an idr have a lock in it?  I can never remember...
>>
>> As far as I know we must use a mutex...
> 
> If you do, someone will come along and remove it, please see:
> 	https://lore.kernel.org/r/b1fcc6707ec2b6309d50060fa52ccc2c892afde2.1728507153.git.christophe.jaillet@wanadoo.fr
> as an example (with links that show it is not needed).

Here is an example about ida API, but I'm using idr API.

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming

Re: [RFC 1/3] drivers pps: add PPS generators support

[Greg KH]

On Thu, Oct 10, 2024 at 09:32:22AM +0200, Rodolfo Giometti wrote:
> On 10/10/24 09:15, Greg KH wrote:
> > On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
> > > > > +#ifdef CONFIG_COMPAT
> > > > > +static long pps_gen_cdev_compat_ioctl(struct file *file,
> > > > > +		unsigned int cmd, unsigned long arg)
> > > > > +{
> > > > > +	cmd = _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(void *));
> > > > > +	return pps_gen_cdev_ioctl(file, cmd, arg);
> > > > > +}
> > > > > +#else
> > > > > +#define pps_gen_cdev_compat_ioctl	NULL
> > > > > +#endif
> > > > > +
> > > > > +static struct pps_gen_device *pps_gen_idr_get(unsigned long id)
> > > > > +{
> > > > > +	struct pps_gen_device *pps_gen;
> > > > > +
> > > > > +	mutex_lock(&pps_gen_idr_lock);
> > > > > +	pps_gen = idr_find(&pps_gen_idr, id);
> > > > > +	if (pps_gen)
> > > > > +		kobject_get(&pps_gen->dev->kobj);
> > > > > +
> > > > > +	mutex_unlock(&pps_gen_idr_lock);
> > > > 
> > > > Doesn't an idr have a lock in it?  I can never remember...
> > > 
> > > As far as I know we must use a mutex...
> > 
> > If you do, someone will come along and remove it, please see:
> > 	https://lore.kernel.org/r/b1fcc6707ec2b6309d50060fa52ccc2c892afde2.1728507153.git.christophe.jaillet@wanadoo.fr
> > as an example (with links that show it is not needed).
> 
> Here is an example about ida API, but I'm using idr API.

Why not use ida then?  :)

Re: [RFC 1/3] drivers pps: add PPS generators support

[Rodolfo Giometti]

On 10/10/24 09:54, Greg KH wrote:
> On Thu, Oct 10, 2024 at 09:32:22AM +0200, Rodolfo Giometti wrote:
>> On 10/10/24 09:15, Greg KH wrote:
>>> On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
>>>>>> +#ifdef CONFIG_COMPAT
>>>>>> +static long pps_gen_cdev_compat_ioctl(struct file *file,
>>>>>> +		unsigned int cmd, unsigned long arg)
>>>>>> +{
>>>>>> +	cmd = _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(void *));
>>>>>> +	return pps_gen_cdev_ioctl(file, cmd, arg);
>>>>>> +}
>>>>>> +#else
>>>>>> +#define pps_gen_cdev_compat_ioctl	NULL
>>>>>> +#endif
>>>>>> +
>>>>>> +static struct pps_gen_device *pps_gen_idr_get(unsigned long id)
>>>>>> +{
>>>>>> +	struct pps_gen_device *pps_gen;
>>>>>> +
>>>>>> +	mutex_lock(&pps_gen_idr_lock);
>>>>>> +	pps_gen = idr_find(&pps_gen_idr, id);
>>>>>> +	if (pps_gen)
>>>>>> +		kobject_get(&pps_gen->dev->kobj);
>>>>>> +
>>>>>> +	mutex_unlock(&pps_gen_idr_lock);
>>>>>
>>>>> Doesn't an idr have a lock in it?  I can never remember...
>>>>
>>>> As far as I know we must use a mutex...
>>>
>>> If you do, someone will come along and remove it, please see:
>>> 	https://lore.kernel.org/r/b1fcc6707ec2b6309d50060fa52ccc2c892afde2.1728507153.git.christophe.jaillet@wanadoo.fr
>>> as an example (with links that show it is not needed).
>>
>> Here is an example about ida API, but I'm using idr API.
> 
> Why not use ida then?  :)

Because we need an ID associated with a pointer.

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming

Re: [RFC 1/3] drivers pps: add PPS generators support

[Greg KH]

On Thu, Oct 10, 2024 at 11:53:44AM +0200, Rodolfo Giometti wrote:
> On 10/10/24 09:54, Greg KH wrote:
> > On Thu, Oct 10, 2024 at 09:32:22AM +0200, Rodolfo Giometti wrote:
> > > On 10/10/24 09:15, Greg KH wrote:
> > > > On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
> > > > > > > +#ifdef CONFIG_COMPAT
> > > > > > > +static long pps_gen_cdev_compat_ioctl(struct file *file,
> > > > > > > +		unsigned int cmd, unsigned long arg)
> > > > > > > +{
> > > > > > > +	cmd = _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(void *));
> > > > > > > +	return pps_gen_cdev_ioctl(file, cmd, arg);
> > > > > > > +}
> > > > > > > +#else
> > > > > > > +#define pps_gen_cdev_compat_ioctl	NULL
> > > > > > > +#endif
> > > > > > > +
> > > > > > > +static struct pps_gen_device *pps_gen_idr_get(unsigned long id)
> > > > > > > +{
> > > > > > > +	struct pps_gen_device *pps_gen;
> > > > > > > +
> > > > > > > +	mutex_lock(&pps_gen_idr_lock);
> > > > > > > +	pps_gen = idr_find(&pps_gen_idr, id);
> > > > > > > +	if (pps_gen)
> > > > > > > +		kobject_get(&pps_gen->dev->kobj);
> > > > > > > +
> > > > > > > +	mutex_unlock(&pps_gen_idr_lock);
> > > > > > 
> > > > > > Doesn't an idr have a lock in it?  I can never remember...
> > > > > 
> > > > > As far as I know we must use a mutex...
> > > > 
> > > > If you do, someone will come along and remove it, please see:
> > > > 	https://lore.kernel.org/r/b1fcc6707ec2b6309d50060fa52ccc2c892afde2.1728507153.git.christophe.jaillet@wanadoo.fr
> > > > as an example (with links that show it is not needed).
> > > 
> > > Here is an example about ida API, but I'm using idr API.
> > 
> > Why not use ida then?  :)
> 
> Because we need an ID associated with a pointer.

Ah, ok, but why?  Why is the "id" here the mapping to the pointer?  Why
not use the structures you already have to store this in (i.e. the
normal driver model stuff?)

All you should need an idr/ida for is to pick a unique "number" for
naming your class device.  Everything after that should already be there
for you to get access to the structures you need to get access to.

thanks,

greg k-h

Re: [RFC 1/3] drivers pps: add PPS generators support

[Rodolfo Giometti]

On 10/10/24 12:04, Greg KH wrote:
> On Thu, Oct 10, 2024 at 11:53:44AM +0200, Rodolfo Giometti wrote:
>> On 10/10/24 09:54, Greg KH wrote:
>>> On Thu, Oct 10, 2024 at 09:32:22AM +0200, Rodolfo Giometti wrote:
>>>> On 10/10/24 09:15, Greg KH wrote:
>>>>> On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
>>>>>>>> +#ifdef CONFIG_COMPAT
>>>>>>>> +static long pps_gen_cdev_compat_ioctl(struct file *file,
>>>>>>>> +		unsigned int cmd, unsigned long arg)
>>>>>>>> +{
>>>>>>>> +	cmd = _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(void *));
>>>>>>>> +	return pps_gen_cdev_ioctl(file, cmd, arg);
>>>>>>>> +}
>>>>>>>> +#else
>>>>>>>> +#define pps_gen_cdev_compat_ioctl	NULL
>>>>>>>> +#endif
>>>>>>>> +
>>>>>>>> +static struct pps_gen_device *pps_gen_idr_get(unsigned long id)
>>>>>>>> +{
>>>>>>>> +	struct pps_gen_device *pps_gen;
>>>>>>>> +
>>>>>>>> +	mutex_lock(&pps_gen_idr_lock);
>>>>>>>> +	pps_gen = idr_find(&pps_gen_idr, id);
>>>>>>>> +	if (pps_gen)
>>>>>>>> +		kobject_get(&pps_gen->dev->kobj);
>>>>>>>> +
>>>>>>>> +	mutex_unlock(&pps_gen_idr_lock);
>>>>>>>
>>>>>>> Doesn't an idr have a lock in it?  I can never remember...
>>>>>>
>>>>>> As far as I know we must use a mutex...
>>>>>
>>>>> If you do, someone will come along and remove it, please see:
>>>>> 	https://lore.kernel.org/r/b1fcc6707ec2b6309d50060fa52ccc2c892afde2.1728507153.git.christophe.jaillet@wanadoo.fr
>>>>> as an example (with links that show it is not needed).
>>>>
>>>> Here is an example about ida API, but I'm using idr API.
>>>
>>> Why not use ida then?  :)
>>
>> Because we need an ID associated with a pointer.
> 
> Ah, ok, but why?  Why is the "id" here the mapping to the pointer?  Why
> not use the structures you already have to store this in (i.e. the
> normal driver model stuff?)
> 
> All you should need an idr/ida for is to pick a unique "number" for
> naming your class device.  Everything after that should already be there
> for you to get access to the structures you need to get access to.

OK, let me review the code and then I'm going to propose a new patchset.

Thanks for your suggestions. :)

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming