Received: by 2002:a05:6500:1b8f:b0:1fa:5c73:8e2d with SMTP id df15csp253678lqb; Tue, 28 May 2024 14:42:28 -0700 (PDT) X-Forwarded-Encrypted: i=2; AJvYcCVCCBCVsv2uiW3yzfMUOVt0HgAQBY1b+QuIn7Ph8NwSDwg4CXW/erGaC6BLzL4y7dQVxB2r3/b2GMsnq9ej5cN9xekxAQMRXp6moWjoyg== X-Google-Smtp-Source: AGHT+IFJ1Ld8EdQWgwvONUDQbGUvfugcMbN4rK7ULoZCKQzasb6uqSyiwEHB6SX5g6r2EBh/WLIE X-Received: by 2002:a17:902:d48a:b0:1f4:9158:1022 with SMTP id d9443c01a7336-1f4eaacda50mr4821755ad.14.1716932548020; Tue, 28 May 2024 14:42:28 -0700 (PDT) Return-Path: Received: from sy.mirrors.kernel.org (sy.mirrors.kernel.org. [2604:1380:40f1:3f00::1]) by mx.google.com with ESMTPS id d9443c01a7336-1f44c96f4bcsi88091915ad.308.2024.05.28.14.42.27 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 28 May 2024 14:42:28 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel+bounces-192994-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1; Authentication-Results: mx.google.com; arc=fail (body hash mismatch); spf=pass (google.com: domain of linux-kernel+bounces-192994-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-192994-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=fail (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sy.mirrors.kernel.org (Postfix) with ESMTPS id 489DDB26A3B for ; Tue, 28 May 2024 20:02:52 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 875CC10A3E; Tue, 28 May 2024 20:02:45 +0000 (UTC) Received: from fgw22-7.mail.saunalahti.fi (fgw22-7.mail.saunalahti.fi [62.142.5.83]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8282C172BDB for ; Tue, 28 May 2024 20:02:43 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=62.142.5.83 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716926565; cv=none; b=VRB/sx8M71ze3FbS1qMHONErTblqtNXvFbE/n1Jp9DXjhIZYpMlzjiZUcZGbQvRyIlEeGALXTQatK4gugZLvTkQml5lzo0dY48cCusNyeapTgqi7vKZxAzpUk5VEHJ7o0fZMMRji3Yilr9s9SIR7AA1qV9RVW2y6gtYVoEIJQaM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1716926565; c=relaxed/simple; bh=pTf2qUi734ES9Al085wTXfYC8mQU+J7CT38EpK7LQlk=; h=From:Date:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ncZ/ucebWaclToqAjNDTJiJ9yeONmJljV83KQAc6oO5uf2OfwVv2D7HsOdJyvxFinY0Rk/nWeAV52MaIFs/E6LUzz+ena3q/KtOqDTSQhaGFkqyjY9vc2uqQYzudztBZoLhb9REBkA46DpEOenvBuo1N1MMftLbzYt/LZgKFapU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com; spf=fail smtp.mailfrom=gmail.com; arc=none smtp.client-ip=62.142.5.83 Authentication-Results: smtp.subspace.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=fail smtp.mailfrom=gmail.com Received: from localhost (88-113-26-230.elisa-laajakaista.fi [88.113.26.230]) by fgw21.mail.saunalahti.fi (Halon) with ESMTP id 3cab8281-1d2d-11ef-aaf5-005056bdd08f; Tue, 28 May 2024 23:02:41 +0300 (EEST) From: Andy Shevchenko Date: Tue, 28 May 2024 23:02:39 +0300 To: Alvin =?utf-8?Q?=C5=A0ipraga?= Cc: Mark Brown , Greg Kroah-Hartman , "Rafael J. Wysocki" , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Linus Walleij , Bartosz Golaszewski , Liam Girdwood , Jaroslav Kysela , Takashi Iwai , Michael Turquette , Stephen Boyd , Andi Shyti , Saravana Kannan , Emil Svendsen , linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-gpio@vger.kernel.org, linux-sound@vger.kernel.org, linux-clk@vger.kernel.org, linux-i2c@vger.kernel.org, Alvin =?utf-8?Q?=C5=A0ipraga?= Subject: Re: [PATCH 06/13] gpio: add AD24xx GPIO driver Message-ID: References: <20240517-a2b-v1-0-b8647554c67b@bang-olufsen.dk> <20240517-a2b-v1-6-b8647554c67b@bang-olufsen.dk> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20240517-a2b-v1-6-b8647554c67b@bang-olufsen.dk> Fri, May 17, 2024 at 02:58:04PM +0200, Alvin Šipraga kirjoitti: > From: Alvin Šipraga > > This driver adds GPIO function support for AD24xx A2B transceiver chips. "Add GPIO..." > When a GPIO is requested, the relevant pin is automatically muxed to > GPIO mode. The device tree property gpio-reserved-ranges can be used to > protect certain pins which are reserved for other functionality such as > I2S/TDM data. Why this doesn't use gpio-regmap? .. > +config GPIO_AD24XX > + tristate "Analog Devies Inc. AD24xx GPIO support" > + depends on A2B_AD24XX_NODE > + help > + Say Y here to enable GPIO support for AD24xx A2B transceivers. checkpatch probably complain about too short help text. You may extend it by explaining how module will be called. .. > +#include > +#include This seems to me not so generic as below... + bits.h + device.h + err.h > +#include > +#include + mod_devicetable.h > +#include + mutex.h > +#include Please, can we avoid OF in a new code? > +#include ..hence move that group here and put a blank line before. .. > +struct ad24xx_gpio { > + struct device *dev; > + struct a2b_func *func; > + struct a2b_node *node; > + struct regmap *regmap; > + int irqs[AD24XX_MAX_GPIOS]; > + struct gpio_chip gpio_chip; If you move this to be the first member, you might get less code being generated at compile time. > + struct irq_chip irq_chip; Should not be here, but static. > + struct mutex mutex; > + unsigned int irq_invert : AD24XX_MAX_GPIOS; > + unsigned int irq_enable : AD24XX_MAX_GPIOS; > +}; .. > + if (ret) > + dev_err(adg->dev, > + "failed to update interrupt configuration: %d\n", ret); Why and how is this useful? .. > + struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node); First of all it uses a wrong API (custom to IRQ core), second why do you need this? .. > + struct device_node *np; > + np = of_irq_find_parent(dev->of_node); > + if (!np) > + return -ENOENT; > + > + parent_domain = irq_find_host(np); > + of_node_put(np); > + if (!parent_domain) > + return -ENOENT; Why is this magic needed? .. > + ret = devm_gpiochip_add_data(dev, gpio_chip, adg); > + if (ret) > + return ret; > + > + return 0; return devm_gpiochip_add_data(...); -- With Best Regards, Andy Shevchenko