Received: by 2002:a05:6358:3188:b0:123:57c1:9b43 with SMTP id q8csp2817268rwd; Fri, 26 May 2023 11:40:34 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ51IfFdS4k6QwJyKJ0bjHDfBi50HH1e4kY9gCZZcAdMoBhtnVOk0aN+vigm0T2oihhWkbf8 X-Received: by 2002:a17:902:c40e:b0:1ae:44f0:82d0 with SMTP id k14-20020a170902c40e00b001ae44f082d0mr3922979plk.43.1685126433860; Fri, 26 May 2023 11:40:33 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1685126433; cv=none; d=google.com; s=arc-20160816; b=UUDk+r1ju7cSK8AlIZdfNoG8/feMvIINRqJTsGp2+W7dzF9p+YrdR2yl+wtxc3DUn5 Kf5COdxbM/T/rXdquJfu43wlNT2tLDE59MxkQ2edd5jkE9xuDxZUq6mtaDIn4OxSdrYD Y8aEHVUlAs2f14WVTbiZu+NrD7mCjUk1qcbecwmw0wirfpSUHY73Z1+Q9UsCcS0CqFSL Oiej0t+EYyyC3wCsX3B+dOULUA453bn2iuBiwI9VcTrsphgTnI7WcQYRR5enYun8Kc3O RFSmqqoSTm9FJDNheyCDJwDTlX/bNklEMUd0UANPBUGTyCJI2ZFp/b0ApntvS7MV23y8 GLng== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:date:from; bh=pb85W4k66oNT5jfwc239Zl/nr5FwBrEc0QPnZoEbFp0=; b=AQxC0yHM0XI3xexU0pQBUJUlO+Mzv1XzISFZ78+5wdAD5y2Mcxy1kUdBa5364yn4EH dh2qXXFPbXEP8jU/VZfexsFoYpRO1aHCgQ7r49Uhy0FNYsHAACPBo3/uXsQCd4wy/25b spDRNs6a08Vt1l1mF3rqGQRlIFMXUx7cC5BOV3sGGqexr7f6zK7VPhMFQ3Z6uGnB/5zA GPVLzhV/nMzCQZ55QuBFjCUMa2MoFfJhnlGZ8REZqebYJ6Vy2M1wtE4ZMvOTlAT0hL5j qB+UmE4d9EEbz/ZRK7pI6sW2yXb6SYHGiHtv3Ou0hQ3N0e9sez+caUuu6482/vw07Jfe nwEQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id o5-20020a170902778500b001ab007aff67si4385971pll.32.2023.05.26.11.40.20; Fri, 26 May 2023 11:40:33 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231144AbjEZSQG (ORCPT + 99 others); Fri, 26 May 2023 14:16:06 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35328 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230144AbjEZSQF (ORCPT ); Fri, 26 May 2023 14:16:05 -0400 Received: from fgw21-7.mail.saunalahti.fi (fgw21-7.mail.saunalahti.fi [62.142.5.82]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1115113A for ; Fri, 26 May 2023 11:16:00 -0700 (PDT) Received: from localhost (88-113-26-95.elisa-laajakaista.fi [88.113.26.95]) by fgw21.mail.saunalahti.fi (Halon) with ESMTP id 581a60e7-fbf1-11ed-abf4-005056bdd08f; Fri, 26 May 2023 21:15:51 +0300 (EEST) From: andy.shevchenko@gmail.com Date: Fri, 26 May 2023 21:15:50 +0300 To: Jerome Neanne Cc: Linus Walleij , Bartosz Golaszewski , Tony Lindgren , Lee Jones , linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, linux-omap@vger.kernel.org, Jonathan Cormier Subject: Re: [PATCH v3 1/2] gpio: tps65219: add GPIO support for TPS65219 PMIC Message-ID: References: <20230511-tps65219-add-gpio-support-v3-0-19837a34d820@baylibre.com> <20230511-tps65219-add-gpio-support-v3-1-19837a34d820@baylibre.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230511-tps65219-add-gpio-support-v3-1-19837a34d820@baylibre.com> X-Spam-Status: No, score=0.7 required=5.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, FORGED_GMAIL_RCVD,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED,SPF_HELO_NONE, SPF_SOFTFAIL,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Fri, May 26, 2023 at 05:06:03PM +0200, Jerome Neanne kirjoitti: > Add support for TPS65219 PMICs GPIO interface. > > 3 GPIO pins: > - GPIO0 only is IO but input mode reserved for MULTI_DEVICE_ENABLE usage > - GPIO1 and GPIO2 are Output only and referred as GPO1 and GPO2 in spec > > GPIO0 is statically configured as input or output prior to Linux boot. > it is used for MULTI_DEVICE_ENABLE function. > This setting is statically configured by NVM. > GPIO0 can't be used as a generic GPIO (specification Table 8-34). > It's either a GPO when MULTI_DEVICE_EN=0, > or a GPI when MULTI_DEVICE_EN=1. > Datasheet describes specific usage for non standard GPIO. > Link: https://www.ti.com/lit/ds/symlink/tps65219.pdf > This blank line makes Link: above not to be a tag. Tag block mustn't have blank lines. OTOH, the other text must be delimited by a blank line before the tag block. That said, move this blank line to before Link: line. > Co-developed-by: Jonathan Cormier > Signed-off-by: Jonathan Cormier > Signed-off-by: Jerome Neanne ... > + help > + Select this option to enable GPIO driver for the TPS65219 > + chip family. > + GPIO0 is statically configured as input or output prior to Linux boot. > + it is used for MULTI_DEVICE_ENABLE function. > + This setting is statically configured by NVM. > + GPIO0 can't be used as a generic GPIO. > + It's either a GPO when MULTI_DEVICE_EN=0, > + or a GPI when MULTI_DEVICE_EN=1. This is strange indentation, we have longer lines, why not using all room available? Btw, seems the commit message itself has the same issue. > + This driver can also be built as a module. If so, the > + module will be called gpio_tps65219. ... Missing bits.h > +#include > +#include > +#include > +#include > +#include ... > +static int tps65219_gpio_get(struct gpio_chip *gc, unsigned int offset) > +{ > + struct tps65219_gpio *gpio = gpiochip_get_data(gc); With struct device *dev = gpio->tps->dev; you may make some code blocks shorter. > + int ret, val; > + > + if (offset != TPS65219_GPIO0_IDX) { > + dev_err(gpio->tps->dev, > + "GPIO%d is output only, cannot get\n", > + offset); Like here. > + return -EOPNOTSUPP; > + } > + > + ret = regmap_read(gpio->tps->regmap, TPS65219_REG_MFP_CTRL, &val); > + if (ret) > + return ret; > + > + dev_warn(gpio->tps->dev, > + "GPIO%d = %d, used for MULTI_DEVICE_ENABLE, not as standard GPIO\n", > + offset, !!(val & BIT(TPS65219_MFP_GPIO_STATUS_MASK))); Isn't it the same... > + /* depends on NVM config return error if dir output else the GPIO0 status bit */ > + if (tps65219_gpio_get_direction(gc, offset) == TPS65219_GPIO_DIR_OUT) > + return -EOPNOTSUPP; > + > + return !!(val & BIT(TPS65219_MFP_GPIO_STATUS_MASK)); ...as this one? What the point to evaluate it twice? > +} ... > + dev_err(gpio->tps->dev, "GPIO%d, set to value %d failed.\n", offset, value); Yeah, there is an inconsistency between line lengths in different functions. Define for yourself the style 80 or 100 and use it everywhere. ... > + /* Documentation is stating that GPIO0 direction must not be changed in Linux: > + * Table 8-34. MFP_1_CONFIG(3): MULTI_DEVICE_ENABLE, > + * Should only be changed in INITIALIZE state (prior to ON Request). > + * Set statically by NVM, changing direction in application can cause a hang. > + * Below can be used for test purpose only: > + */ > + /* * The style of multi-line comment * is incorrect. See this example. * Besides that, remove unneeded * blank line above. */ > + if (IS_ENABLED(DEBUG)) { > + int ret = regmap_update_bits(gpio->tps->regmap, TPS65219_REG_MFP_1_CONFIG, > + TPS65219_GPIO0_DIR_MASK, direction); > + if (ret) { > + dev_err(gpio->tps->dev, > + "DEBUG enabled: Fail to change direction to %u for GPIO%d. \ > + For test only\n", Do not split string literals on non-\n characters. > + direction, offset); > + return ret; Wrong indentation. > + } > + } -- With Best Regards, Andy Shevchenko