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

[Christopher S. Hall]

Hi Linus,

Thanks for the review.

On Fri, Feb 07, 2020 at 06:10:46PM +0100, Linus Walleij wrote:
> Hi Christopher,
> 
> thanks for your patch!
> 
> On Fri, Jan 31, 2020 at 7:41 AM <christopher.s.hall@xxxxxxxxx> wrote:
> 
> > From: Christopher Hall <christopher.s.hall@xxxxxxxxx>
> >

> > 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 <balbi@xxxxxxxxxx>
> >
> > Signed-off-by: Christopher Hall <christopher.s.hall@xxxxxxxxx>

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

I see your concern. TGPIO is Intel's internal name for the device, but
there's no reason we can't use some other terminology in the context of
the Linux kernel. How about removing the GP? We could refer to the device
as "timed I/O". I think that is still fairly descriptive, but removes the
confusion. Does this help the problem?

> > +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?

Yes. You're correct. Removed.

> > +/* 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?

The Timed GPIO device has some GPIO-like features, but is mostly used to
import/export a clock signal. It doesn't implement PWM or some other "GP"
features like reading/setting pin state. I think you can safely ignore
the feature.

> 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

Yes. It is missing many of general purpose features.

> rather, so this confusion needs to be explained in the commit
> message and possibly in the code as well.
> 
> What is it for really?

For import/export system clock, primarily.

> Yours,
> Linus Walleij

Thanks,
Christopher