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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 99872CE837A for ; Tue, 1 Oct 2024 13:21:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:Cc:To:Subject:Message-ID:Date:From:In-Reply-To:References: MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=5d0oytKKUoV7wMWG1zsqDf6MGeXNHEnligQuB1wzGpo=; b=xqUGkAQn0qD3D9+OQ7lHrDbcEX coM6XFPCDE0JjDbgW177b7s4mdy+72mogHYsOLYAaG6Go2xR638ZiFF5DxUmdxUS3Sb9TF9veu5Dn b9WFx758wtFKRyRnJyNjjTtOomzdGJWtSaVUEkx4hjE3K30tqbYy3Xkxy7AEcCvsZegAMY74iNXqT LZ/rxZT1kVNkaj6Wp5UtO4lkaEq3omkqqL9QBZA1++jbDEyqYNbP4MTYKiD44whAE7rsORSlEjlu0 LVD8QvyNQqFVGBNuMhZdifv6IcmrJO++BrQ4etkNLnEE9HL+Eaa/lpX74tbFhkCFEP8TiepvP+6C/ w9rK80Xg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1svcoA-00000002uor-05vT; Tue, 01 Oct 2024 13:20:54 +0000 Received: from mail-lj1-x236.google.com ([2a00:1450:4864:20::236]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1svcjO-00000002saO-37GI for linux-arm-kernel@lists.infradead.org; Tue, 01 Oct 2024 13:16:00 +0000 Received: by mail-lj1-x236.google.com with SMTP id 38308e7fff4ca-2fac6b3c220so29260531fa.2 for ; Tue, 01 Oct 2024 06:15:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1727788556; x=1728393356; darn=lists.infradead.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=5d0oytKKUoV7wMWG1zsqDf6MGeXNHEnligQuB1wzGpo=; b=sLkKOXEFlitYIpUFKuE0RlTJgaBdMlkpPMwiQ4/qlhhWgnMZ1sQ1fxvg6cxkTC+ds8 /eveZ0kqx6MzFsFZwBH13CkA78TYPnxycM07ZbqKZHU8RVB0pXBJJyA478dhaZFs8cv7 kj351bus6L1NBZFr52xJrB7vhT/UEhDMVg5q2pRJcx+V05dNKWv+fLELGo4LsO/Imkpi 5ERiFB5QR4rKKVFV5mGc9EnTzXXj480g9ZWxsl6sTWjZ28ReU8FpF8RpkrmDLIaXkla4 TD/an0X4KId87SHjvlMyxdg2usSoSV+F/9RSux+a93Zo8imagNIvJAa4UOYwH//ihtqE /MbA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727788556; x=1728393356; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=5d0oytKKUoV7wMWG1zsqDf6MGeXNHEnligQuB1wzGpo=; b=BsmUeaBIEBWOdWmu3quVU1UAALMV2T9mKtxFoL2McyxzIgCFqnLJWuJAvrvmou1OYn tEbiNcvVhXiE0CqLSFUKX2Qja+kHQe9c2di4QQP7qogEjrEnjg9GFuDKoddIFiFJoT9V o/X5NlQtDhUmG8yg2ULLQ3gyJlI8ZnPLmIj/DtEdkiU3JfqwOZmmyZaotq7m3RzN4Y6r dZudMSvknA+aFHRUDLZZwfaboa+A1SlTebRYwWAH0CL8l2wul8JOD2F8hpEDNJ2LpuWC hfhqIo1lWXEitM5eaDvO8Enj5HrMhIKmhJgwSFFy0Am7wCvKnp4NEQz08rQLh9uQKLuE zk7g== X-Forwarded-Encrypted: i=1; AJvYcCUE5VIROiIQWfE4tBYj2z/TloEqkvWRnM35GTxvKwY99MOtMH2AeY6sWTuJpcpwNR1tMzT11dKBqmuKlMqS3dgC@lists.infradead.org X-Gm-Message-State: AOJu0YwkMIiquwLj8pvZBC6WujNQAb1WcEwK4xNjwx+twKydjd94ip/2 7ixKjgF8uw8pAEt7elKK/iMhh4Tb+hVdRlljBAjJpzFOaDcm91D10xLaN+o+gssjFJzbm8G+CJi TFt78IDhPIxWsxJKuuIbfQb4lHKs9pqvp2zfclQ== X-Google-Smtp-Source: AGHT+IFSCJODkQZaEKKvFqJ003dLieDQX4xED+qGDI+GkjgnscjHC7N9CLzuAMkbp4HaVOfhZAiiAgojIxmNepS1Y24= X-Received: by 2002:a05:6512:3343:b0:531:8f2f:8ae7 with SMTP id 2adb3069b0e04-5389fc46b2dmr10930455e87.25.1727788555727; Tue, 01 Oct 2024 06:15:55 -0700 (PDT) MIME-Version: 1.0 References: <20240926143122.1385658-1-andrei.stefanescu@oss.nxp.com> <20240926143122.1385658-4-andrei.stefanescu@oss.nxp.com> In-Reply-To: <20240926143122.1385658-4-andrei.stefanescu@oss.nxp.com> From: Linus Walleij Date: Tue, 1 Oct 2024 15:15:43 +0200 Message-ID: Subject: Re: [PATCH v4 3/4] gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support To: Andrei Stefanescu Cc: Bartosz Golaszewski , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Chester Lin , Matthias Brugger , Greg Kroah-Hartman , "Rafael J. Wysocki" , linux-gpio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, NXP S32 Linux Team , Christophe Lizzi , Alberto Ruiz , Enric Balletbo Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241001_061558_865777_D7A61F51 X-CRM114-Status: GOOD ( 37.62 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Andrei, thanks for your patch! Sorry for being so late in giving any deeper review of it. On Thu, Sep 26, 2024 at 4:32=E2=80=AFPM Andrei Stefanescu wrote: > Add the GPIO driver for S32G2/S32G3 SoCs. This driver uses the SIUL2 > (System Integration Unit Lite2) hardware module. There are two > SIUL2 hardware modules present, SIUL2_0(controlling GPIOs 0-101) and > SIUL2_1 for the rest. > > The GPIOs are not fully contiguous, there are some gaps: > - GPIO102 up to GPIO111(inclusive) are invalid > - GPIO123 up to GPIO143(inclusive) are invalid > > Some GPIOs are input only(i.e. GPI182) though this restriction > is not yet enforced in code. > > This patch adds basic GPIO functionality(no interrupts, no > suspend/resume functions). > > Signed-off-by: Ghennadi Procopciuc > Signed-off-by: Larisa Grigore > Signed-off-by: Phu Luu An > Signed-off-by: Andrei Stefanescu (...) > +#include > +#include > +#include > +#include > +#include > +#include > +#include Hm, are you sure you don't want one of those combined GPIO+pinctrl drivers? Look in drivers/pinctrl for examples such as drivers/pinctrl/pinctrl-stmfx.c > +/* PGPDOs are 16bit registers that come in big endian > + * order if they are grouped in pairs of two. > + * > + * For example, the order is PGPDO1, PGPDO0, PGPDO3, PGPDO2... > + */ > +#define SIUL2_PGPDO(N) (((N) ^ 1) * 2) > +#define S32G2_SIUL2_NUM 2 > +#define S32G2_PADS_DTS_TAG_LEN 7 > + > +#define SIUL2_GPIO_16_PAD_SIZE 16 You seem to be manipulating "pads" which is pin control territory. That should be done in a pin control driver. > +/** > + * struct siul2_device_data - platform data attached to the compatible. > + * @pad_access: access table for I/O pads, consists of S32G2_SIUL2_NUM t= ables. > + * @reset_cnt: reset the pin name counter to zero when switching to SIUL= 2_1. > + */ > +struct siul2_device_data { > + const struct regmap_access_table **pad_access; > + const bool reset_cnt; I don't understand the reset_cnt variable at all. Explain why it exists in = the kerneldoc please. > +/** > + * struct siul2_desc - describes a SIUL2 hw module. > + * @pad_access: array of valid I/O pads. Now that is really pin control isn't it. > + * @opadmap: the regmap of the Parallel GPIO Pad Data Out Register. > + * @ipadmap: the regmap of the Parallel GPIO Pad Data In Register. > + * @gpio_base: the first GPIO pin. > + * @gpio_num: the number of GPIO pins. > + */ > +struct siul2_desc { > + const struct regmap_access_table *pad_access; > + struct regmap *opadmap; > + struct regmap *ipadmap; > + u32 gpio_base; > + u32 gpio_num; > +}; (...) > +static int siul2_get_gpio_pinspec(struct platform_device *pdev, > + struct of_phandle_args *pinspec, > + unsigned int range_index) > +{ > + struct device_node *np =3D pdev->dev.of_node; > + > + return of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, > + range_index, pinspec); > +} I do not see why a driver would parse gpio-ranges since the gpiolib core should normally deal with the translation between GPIO lines and pins. This looks hacky and probably an indication that you want to use a combined pinctrl+gpio driver so the different parts of the driver can access the same structures easily without hacks. > +static void siul2_gpio_set_direction(struct siul2_gpio_dev *dev, > + unsigned int gpio, int dir) > +{ > + guard(raw_spinlock_irqsave)(&dev->lock); > + > + if (dir =3D=3D GPIO_LINE_DIRECTION_IN) > + __clear_bit(gpio, dev->pin_dir_bitmap); > + else > + __set_bit(gpio, dev->pin_dir_bitmap); > +} This looks like a job for gpio regmap? select GPIO_REGMAP, look in other driver for examples of how to use it. > +static int siul2_get_direction(struct siul2_gpio_dev *dev, > + unsigned int gpio) > +{ > + return test_bit(gpio, dev->pin_dir_bitmap) ? GPIO_LINE_DIRECTION_= OUT : > + GPIO_LINE_DIRECTION_= IN; > +} Also looks like a reimplementation of GPIO_REGMAP. > +static int siul2_gpio_dir_out(struct gpio_chip *chip, unsigned int gpio, > + int val) > +{ > + struct siul2_gpio_dev *gpio_dev; > + int ret =3D 0; > + > + gpio_dev =3D to_siul2_gpio_dev(chip); > + siul2_gpio_set_val(chip, gpio, val); > + > + ret =3D pinctrl_gpio_direction_output(chip, gpio); (...) This is nice, pin control is properly used as the back-end. > +static int siul2_gpio_remove_reserved_names(struct device *dev, > + struct siul2_gpio_dev *gpio_d= ev, > + char **names) > +{ > + struct device_node *np =3D dev->of_node; > + int num_ranges, i, j, ret; > + u32 base_gpio, num_gpio; > + > + /* Parse the gpio-reserved-ranges to know which GPIOs to exclude.= */ > + > + num_ranges =3D of_property_count_u32_elems(dev->of_node, > + "gpio-reserved-ranges"); > + > + /* The "gpio-reserved-ranges" is optional. */ > + if (num_ranges < 0) > + return 0; > + num_ranges /=3D 2; > + > + for (i =3D 0; i < num_ranges; i++) { > + ret =3D of_property_read_u32_index(np, "gpio-reserved-ran= ges", > + i * 2, &base_gpio); > + if (ret) { > + dev_err(dev, "Could not parse the start GPIO: %d\= n", > + ret); > + return ret; > + } > + > + ret =3D of_property_read_u32_index(np, "gpio-reserved-ran= ges", > + i * 2 + 1, &num_gpio); > + if (ret) { > + dev_err(dev, "Could not parse num. GPIOs: %d\n", = ret); > + return ret; > + } > + > + if (base_gpio + num_gpio > gpio_dev->gc.ngpio) { > + dev_err(dev, "Reserved GPIOs outside of GPIO rang= e\n"); > + return -EINVAL; > + } > + > + /* Remove names set for reserved GPIOs. */ > + for (j =3D base_gpio; j < base_gpio + num_gpio; j++) { > + devm_kfree(dev, names[j]); > + names[j] =3D NULL; > + } > + } > + > + return 0; > +} I don't get this. gpio-reserved-ranges is something that is parsed and handled by gpiolib. Read the code in gpiolib.c and you'll probably figure out how to let gpiolib take care of this for you. > +static int siul2_gpio_populate_names(struct device *dev, > + struct siul2_gpio_dev *gpio_dev) > +{ > + unsigned int num_index =3D 0; > + char ch_index =3D 'A'; > + char **names; > + int i, ret; > + > + names =3D devm_kcalloc(dev, gpio_dev->gc.ngpio, sizeof(*names), > + GFP_KERNEL); > + if (!names) > + return -ENOMEM; > + > + for (i =3D 0; i < S32G2_SIUL2_NUM; i++) { > + ret =3D siul2_gen_names(dev, gpio_dev->siul2[i].gpio_num, > + names + gpio_dev->siul2[i].gpio_bas= e, > + &ch_index, &num_index); > + if (ret) { > + dev_err(dev, "Could not set names for SIUL2_%d GP= IOs\n", > + i); > + return ret; > + } > + > + if (gpio_dev->platdata->reset_cnt) > + num_index =3D 0; > + > + ch_index++; > + } > + > + ret =3D siul2_gpio_remove_reserved_names(dev, gpio_dev, names); > + if (ret) > + return ret; > + > + gpio_dev->gc.names =3D (const char *const *)names; > + > + return 0; > +} Interesting! I'm not against, on the contrary this looks really helpful to users. > + gc =3D &gpio_dev->gc; No poking around in the gpiolib internals please. Look at what other drivers do and do like they do. On top of these comments: everywhere you do [raw_spin_]locks: try to use guards or scoped guards instead. git grep guard drivers/gpio for many examples. Yours, Linus Walleij