Re: [Intel PMC TGPIO Driver 5/5] drivers/ptp: Add PMC Time-Aware GPIO Driver

Hi Christopher,

thanks for your patch!

On Fri, Jan 31, 2020 at 7:41 AM [off-list ref] wrote:

From: Christopher Hall <redacted>

Add support for PMC Time-Aware GPIO (TGPIO) hardware that is present on
upcoming Intel platforms. The hardware logic is driven by the ART clock.
The current hardware has two GPIO pins. Input interrupts are not
implemented in hardware.

The driver implements to the expanded PHC interface. Input requires use of
the user-polling interface. Also, since the ART clock can't be adjusted,
modulating the output frequency uses the edge timestamp interface
(EVENT_COUNT_TSTAMP2) and the PEROUT2 ioctl output frequency adjustment
interface.

Acknowledgment: Portions of the driver code were authored by Felipe
Balbi [off-list ref]

Signed-off-by: Christopher Hall <redacted>

This driver becomes a big confusion for the GPIO maintainer...

+config PTP_INTEL_PMC_TGPIO
+       tristate "Intel PMC Timed GPIO"
+       depends on X86
+       depends on ACPI
+       depends on PTP_1588_CLOCK

(...)

+#include <linux/gpio.h>

Don't use this header in new code, use <linux/gpio/driver.h>

But it looks like you should just drop it because there is no GPIO
of that generic type going on at all?

+/* Control Register */
+#define TGPIOCTL_EN                    BIT(0)
+#define TGPIOCTL_DIR                   BIT(1)
+#define TGPIOCTL_EP                    GENMASK(3, 2)
+#define TGPIOCTL_EP_RISING_EDGE                (0 << 2)
+#define TGPIOCTL_EP_FALLING_EDGE       (1 << 2)
+#define TGPIOCTL_EP_TOGGLE_EDGE                (2 << 2)
+#define TGPIOCTL_PM                    BIT(4)

OK this looks like some GPIO registers...

Then there is a bunch of PTP stuff I don't understand I suppose
related to the precision time protocol.

Could you explain to a simple soul like me what is going on?
Should I bother myself with this or is this "some other GPIO,
not what you work on" or could it be that it's something I should
review?

I get the impression that this so-called "general purpose I/O"
isn't very general purpose at all, it seems to be very PTP-purpose
rather, so this confusion needs to be explained in the commit
message and possibly in the code as well.

What is it for really?

Yours,
Linus Walleij