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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id B1658C47089 for ; Tue, 29 Nov 2022 12:34:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234334AbiK2MeR (ORCPT ); Tue, 29 Nov 2022 07:34:17 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56926 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233816AbiK2MeN (ORCPT ); Tue, 29 Nov 2022 07:34:13 -0500 Received: from mga06.intel.com (mga06b.intel.com [134.134.136.31]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2FD8E5E3ED; Tue, 29 Nov 2022 04:34:10 -0800 (PST) X-IronPort-AV: E=McAfee;i="6500,9779,10545"; a="377243441" X-IronPort-AV: E=Sophos;i="5.96,203,1665471600"; d="scan'208";a="377243441" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Nov 2022 04:34:09 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10545"; a="674605782" X-IronPort-AV: E=Sophos;i="5.96,203,1665471600"; d="scan'208";a="674605782" Received: from smile.fi.intel.com ([10.237.72.54]) by orsmga008.jf.intel.com with ESMTP; 29 Nov 2022 04:34:06 -0800 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1ozzoK-001ha5-00; Tue, 29 Nov 2022 14:34:04 +0200 Date: Tue, 29 Nov 2022 14:34:03 +0200 From: Andy Shevchenko To: Hans de Goede Cc: Mark Gross , Bartosz Golaszewski , Linus Walleij , Daniel Scally , Laurent Pinchart , platform-driver-x86@vger.kernel.org, linux-gpio@vger.kernel.org, Sakari Ailus , Kate Hsuan , linux-media@vger.kernel.org Subject: Re: [PATCH 1/5] gpio: tps68470: Fix tps68470_gpio_get() reading from the wrong register Message-ID: References: <20221128214408.165726-1-hdegoede@redhat.com> <20221128214408.165726-2-hdegoede@redhat.com> <9a4336d1-3222-fe50-f234-93ab175d606a@redhat.com> <1eb61f7a-3b93-32a1-21bf-6929bbb40d36@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1eb61f7a-3b93-32a1-21bf-6929bbb40d36@redhat.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On Tue, Nov 29, 2022 at 01:19:57PM +0100, Hans de Goede wrote: > On 11/29/22 12:56, Andy Shevchenko wrote: > > On Tue, Nov 29, 2022 at 1:27 PM Hans de Goede wrote: > >> On 11/29/22 11:22, Andy Shevchenko wrote: > >>> On Mon, Nov 28, 2022 at 11:44 PM Hans de Goede wrote: > >>>> > >>>> For the regular GPIO pins the value should be read from TPS68470_REG_GPDI, > >>>> so that the actual value of the pin is read, rather then the value the pin > >>> > >>> than > >> > >> Ack. > >> > >>>> would output when put in output mode. > >>> > >>> I don't see it here and haven't checked the context, but the idea is > >>> to check the direction and return either input (if pin is in input > >>> mode) or [cached] output.If it's the case, the patch looks good to me. > >> > >> No the idea is to always actually use the input register when reading > >> the pins, independent of the input/output mode. Instead of always > >> reading the [cached] output register value. > > > > But why? This makes a little sense to me. > > I don't understand what your problem is with this patch ? > > This is standard behavior for GPIO drivers, the get() callback > always reads the actual pin values when there is a registers > to get the actual pin-values. AFAIK this is no different from what > countless other GPIO drivers do ? If the GPIO is requested is output and the pin in the HiZ mode, what value should you return? > >> The input buffer will still work when the device is in output mode > > > > Does this hardware support HiZ? > > Yes the 7 standard GPIO pins can be put in input mode, aka HiZ mode. No, input and HiZ are two different states. If the hardware doesn't support the latter, then your patch doesn't make any difference indeed, except involving "in" buffer for "out" value. Which I consider an unneeded step instead of reading [cached] "out" value. [cached] here is used in a broader sense: either SW cache, or read back value from the GPIO out register (not all hardware support that). > >> and if somehow an external force is pushing the pin low/high against > >> the output value then the input buffer will correctly reflect this > >> (assuming the output is current limited and thus the magic smoke > >> stays inside the chip). > > > > Exactly, when smoke comes out, the hardware is broken and code, > > whatever is it, makes no difference at all. > > The GPIO part of the TPS68470 supports open-drain outputs, to correctly > get the actual value on the pin from the get() callback, the GPDI > register must be used. And being able to detect some outside chip > forcing the line low in open-drain mode is important to be able to > e.g. implement a software I2C master. Right, but for push-pull mode this is not needed, correct? > As I mentioned above actually using the input buffer value in > the get() method is standard behavior for GPIO drivers, exactly > for reasons like allowing a sw I2C master implementation to > detect clock stretching or conflicts (in the multi master case). > I really don't understand what is so controversial about this > patch? There are couple of remarks above: 1) what to read at the real HiZ mode ("in" and "out" buffers are disconnected from the pin) in case hardware supports it; 2) why to read "out" value via "in" buffer in case of push-pull mode. > Note the datasheet describes the GPDO / GPDI bit 0 values as: > > GPDO bit 0: "Control of the GPIO0 output" > GPDI bit 0: "State of the GPIO0 input" > > So clearly GPDI is the correct register to use for the get() > callback, which is what this patch is doing. -- With Best Regards, Andy Shevchenko