From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 49315C38BF9 for ; Mon, 24 Feb 2020 23:17:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 23B1C2080D for ; Mon, 24 Feb 2020 23:17:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728069AbgBXXRy (ORCPT ); Mon, 24 Feb 2020 18:17:54 -0500 Received: from mga07.intel.com ([134.134.136.100]:15611 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726651AbgBXXRy (ORCPT ); Mon, 24 Feb 2020 18:17:54 -0500 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Feb 2020 15:17:53 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,481,1574150400"; d="scan'208";a="410039160" Received: from wtczc53028gn.jf.intel.com (HELO skl-build) ([10.54.87.17]) by orsmga005.jf.intel.com with ESMTP; 24 Feb 2020 15:17:53 -0800 Date: Mon, 24 Feb 2020 15:17:42 -0800 From: "Christopher S. Hall" To: Linus Walleij Cc: Mika Westerberg , Andy Shevchenko , netdev , "linux-kernel@vger.kernel.org" , Thomas Gleixner , "H. Peter Anvin" , Ingo Molnar , "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" , jacob.e.keller@intel.com, Richard Cochran , "David S. Miller" , sean.v.kelley@intel.com Subject: Re: [Intel PMC TGPIO Driver 5/5] drivers/ptp: Add PMC Time-Aware GPIO Driver Message-ID: <20200224231742.GD1508@skl-build> References: <20191211214852.26317-1-christopher.s.hall@intel.com> <20191211214852.26317-6-christopher.s.hall@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 wrote: > > > From: Christopher Hall > > > > 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 > > > > Signed-off-by: Christopher Hall > 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 > > Don't use this header in new code, use > > 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