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 7EF21CE7D1B for ; Tue, 1 Oct 2024 12:46:21 +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=qJdk49FPom1vhJW6KLrc9xb2N9g+9cOy2pAI61oaoAo=; b=iFTR56fMNeAhr7regG4uQAZsFk jYV4pztfD35T129Mt4jxSoIh/7Jj4mUWTYFjcYtzpyGWOx6HBZQx2qRholapY1eQwvCfqmhL3flfW SbkooKWLjL2M3BjI/+hycncpAjKbujmqOPc+2CXu9OK76yIdmYwwXhm8lQS+3CYA3HTI2wj9Eh8Kx yZBc3ymLJcNiWvVxMFrzGV5RvtVkZWVa9Qc8otrUcrbjx8wJzxhWfjkIJhCE3l8kiXJKDy6OY9LSa 58wzmI+r40wdGVyUKvB8jJdN0p7fYELPyD27L+EWoiERgcixT+2+BF4su21FLkXqM7xsjWLXxDwts oFzZqUiQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1svcGY-00000002olw-33Ib; Tue, 01 Oct 2024 12:46:10 +0000 Received: from mail-lf1-x12b.google.com ([2a00:1450:4864:20::12b]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1svcFH-00000002oRd-3hwa for linux-arm-kernel@lists.infradead.org; Tue, 01 Oct 2024 12:44:53 +0000 Received: by mail-lf1-x12b.google.com with SMTP id 2adb3069b0e04-5389fbb28f3so4061701e87.1 for ; Tue, 01 Oct 2024 05:44:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1727786690; x=1728391490; 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=qJdk49FPom1vhJW6KLrc9xb2N9g+9cOy2pAI61oaoAo=; b=oF6nRdpZVzen+t2xISNZB2klztmCvI98Iv7XWFAnEKVaL5n+1rBbgq3Vp+LMG3xiZ5 wB2/oNtm2Lv9VcBbFYBqXoLdZy6bApJprbiVgCza9GCmPX75ehfcRsaJ5Y+pAV6elVq8 TsFeJQVRhTyK9W61+3Ja+6j9b1EqKs7ym1E1JpyfdP8jkNK3QHNlEmikHERSGl3I2ms0 LOAuBOdS2YNfnJxHCxelZaz4rfT5q144DDw7puDVuuVhAlNJLKUDw/iDCyIa31LXGTyf r5tUQb3jKzS3YZCXBDw22H7LJEpGzrCEDroNqGSYTffxPBfXkqlvJW6YZOtzNMHHvHMF 591w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727786690; x=1728391490; 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=qJdk49FPom1vhJW6KLrc9xb2N9g+9cOy2pAI61oaoAo=; b=tUUSlojzkeDBaHg9Q4CBCh4An71VPvsob0JE7+ln3S7Mh1E5e+lKQ8veY9G7KuGaea u8LvzZX60P8gnWRZUQEZ+KNxi1FhmlX5CxAEdCKUx4zuBFqF8Rqp3idejrAptwooS0bT MFWwH8L9GV2Iavu1+B37hfaAaPBFZFW1e/9GJEvs+xjETbRKHWOJSZ+P9Z8wOQH3fURe WtVBAI30h/K7DRTqDU/K2QifXsIcxVyQXMnkLdyiwJ2em9HuuIhGUzUBYNW9sn/ETrd2 MDnDcHZux2p5Vb9n0O5j6qjBPrYL3U0nmGsV0mvoy43dnX3nUCJdOlDxxPo8y0Xoyg2j /fag== X-Forwarded-Encrypted: i=1; AJvYcCWGr+Ab/DcRY4sWrG+LUVvIbUMaA9ZhDyB1RICOAyxKocTWAjvSxis6pNAz/x0x2Hl+5PMsvfT30MrHPDTTVwlh@lists.infradead.org X-Gm-Message-State: AOJu0YyZSA7tAYLJvV5oBS8jMAZLvr+at7Q9HDSu8zSBjv4mDyF3SCrR fAKqD8rdv9iIyOwITib1nUkXil1nFEBCAf98/tax4Y0NZGFwBelhzCAAlYHxNmy+4AvSP60kwYA XXhRHkTOicv3uQrEf3727tZb7vI/3uIat5LHsVA== X-Google-Smtp-Source: AGHT+IFpLEVSrtSEfceUFr1y/mkuGvz43bOehtQ1kSTprLlq+kFdHRQGWxpV7X7Wx0HYBHJpzMV+133mIhupsz3X9bs= X-Received: by 2002:a05:6512:6c6:b0:52f:30d:526c with SMTP id 2adb3069b0e04-5399a23f44cmr1103973e87.5.1727786689856; Tue, 01 Oct 2024 05:44:49 -0700 (PDT) MIME-Version: 1.0 References: <20240912-test-v1-0-458fa57c8ccf@analog.com> <20240912-test-v1-9-458fa57c8ccf@analog.com> In-Reply-To: <20240912-test-v1-9-458fa57c8ccf@analog.com> From: Linus Walleij Date: Tue, 1 Oct 2024 14:44:39 +0200 Message-ID: Subject: Re: [PATCH 09/21] gpio: add driver for ADI ADSP-SC5xx platform To: arturs.artamonovs@analog.com Cc: Catalin Marinas , Will Deacon , Greg Malysa , Philipp Zabel , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Utsav Agarwal , Michael Turquette , Stephen Boyd , Bartosz Golaszewski , Thomas Gleixner , Andi Shyti , Greg Kroah-Hartman , Jiri Slaby , Arnd Bergmann , Olof Johansson , soc@kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-clk@vger.kernel.org, linux-gpio@vger.kernel.org, linux-i2c@vger.kernel.org, linux-serial@vger.kernel.org, adsp-linux@analog.com, Nathan Barrett-Morrison 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_054451_960890_592D4F7F X-CRM114-Status: GOOD ( 21.34 ) 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 Arturs, thanks for your patch! On Thu, Sep 12, 2024 at 8:20=E2=80=AFPM Arturs Artamonovs via B4 Relay wrote: > From: Arturs Artamonovs > > Add ADSP-SC5xx GPIO driver. > - Support all GPIO ports > - Each gpio support seperate PINT interrupt controller > > Signed-off-by: Arturs Artamonovs > Co-developed-by: Nathan Barrett-Morrison > Signed-off-by: Nathan Barrett-Morrison > Co-developed-by: Greg Malysa > Signed-off-by: Greg Malysa (...) > +config GPIO_ADI_ADSP_PORT > + bool "ADI ADSP PORT GPIO driver" > + depends on OF_GPIO > + select GPIO_GENERIC If you select this then you need to use it in the idiomatic way. +#include Drop this, just bring the contents into this file all register defines etc. +#include "gpiolib.h" No way, do this: #include > +static int adsp_gpio_direction_input(struct gpio_chip *chip, unsigned in= t offset) > +{ > + struct adsp_gpio_port *port =3D to_adsp_gpio_port(chip); > + > + __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_DIR_CLEAR); Ah these __adsp_gpio_writew/readw things are too idiomatic. Just use the base and common writew() please. > + __adsp_gpio_writew(port, BIT(offset), ADSP_PORT_REG_INEN_SET); Interrupt enable in the direction function? No thanks, poke the interrupt registers in your irqchip if you make one (you currently do not) in this case I'd say just disable all interrupts in probe() using something like writew(base + ADSP_PORT_REG_INEN_SET, 0xffff) and be done with it. > +static int adsp_gpio_get_value(struct gpio_chip *chip, unsigned int offs= et) > +{ > + struct adsp_gpio_port *port =3D to_adsp_gpio_port(chip); > + > + return !!(__adsp_gpio_readw(port, ADSP_PORT_REG_DATA) & BIT(offse= t)); > +} This becomes a reimplemenation of generic GPIO. > +static int adsp_gpio_to_irq(struct gpio_chip *chip, unsigned int offset) > +{ > + struct adsp_gpio_port *port =3D to_adsp_gpio_port(chip); > + irq_hw_number_t irq =3D offset + port->irq_offset; > + int map =3D irq_find_mapping(port->irq_domain, irq); > + > + if (map) > + return map; > + > + return irq_create_mapping(port->irq_domain, irq); > +} This irqdomain in the "port" looks weird. Implement the irqchip in the GPIO driver instead. If the domain *has* to be external to the GPIO driver then you need to use hierarchical irqdomains. > +static int adsp_gpio_probe(struct platform_device *pdev) > +{ > + struct device *dev =3D &pdev->dev; > + struct adsp_gpio_port *gpio; > + int ret; > + > + gpio =3D devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL); > + if (!gpio) > + return -ENOMEM; > + > + gpio->regs =3D devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(gpio->regs)) > + return PTR_ERR(gpio->regs); So you have gpio->regs which is the base. > + gpio->dev =3D dev; > + > + ret =3D adsp_attach_pint_to_gpio(gpio); > + if (ret) > + dev_err_probe(gpio->dev, ret, "error attaching interupt t= o gpio pin\n"); > + > + spin_lock_init(&gpio->lock); > + > + gpio->gpio.label =3D "adsp-gpio"; > + gpio->gpio.direction_input =3D adsp_gpio_direction_input; > + gpio->gpio.direction_output =3D adsp_gpio_direction_output; > + gpio->gpio.get =3D adsp_gpio_get_value; > + gpio->gpio.set =3D adsp_gpio_set_value; > + gpio->gpio.to_irq =3D adsp_gpio_to_irq; > + gpio->gpio.request =3D gpiochip_generic_request; > + gpio->gpio.free =3D gpiochip_generic_free; > + gpio->gpio.ngpio =3D ADSP_PORT_NGPIO; > + gpio->gpio.parent =3D dev; > + gpio->gpio.base =3D -1; > + return devm_gpiochip_add_data(dev, &gpio->gpio, gpio); Look in e.g. drivers/gpio/gpio-ftgpio010.c for an example of how to use generic GPIO (with an irqchip!). It will be something like: ret =3D bgpio_init(&g->gc, dev, 2, gpio->regs + ADSP_PORT_REG_DATA, gpio->regs + ADSP_PORT_REG_DATA_SET, gpio->regs + ADSP_PORT_REG_DATA_CLEAR, gpio->regs + ADSP_PORT_REG_DIR_SET, gpio->regs + ADSP_PORT_REG_DIR_CLEAR, 0); if (ret) { dev_err(dev, "unable to init generic GPIO\n"); goto dis_clk; } g->gc.label =3D dev_name(dev); g->gc.base =3D -1; g->gc.parent =3D dev; g->gc.owner =3D THIS_MODULE; /* ngpio is set by bgpio_init() */ You can augment the generic driver instance with an extra config function to set the special open drain bits. Yours, Linus Walleij