Received: by 2002:a05:6a10:f347:0:0:0:0 with SMTP id d7csp2001200pxu; Sun, 6 Dec 2020 15:16:55 -0800 (PST) X-Google-Smtp-Source: ABdhPJx8ot/fmjHhyMWcxXcqZwHkC86nfPhFHoNoRzvq40gJGU+RsWuWny7quZE0sSH5gk5nrQr2 X-Received: by 2002:a17:906:7687:: with SMTP id o7mr16812182ejm.209.1607296615104; Sun, 06 Dec 2020 15:16:55 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1607296615; cv=none; d=google.com; s=arc-20160816; b=wcgT8NmBL6N703N0JW2Dic9xRnlVEYzSo8xC6MvTnqwhYJn2n/Ml6r3lWfdUYSHsmk b0C6/iCIgsY4+kml3nC57vx1gw7YtTY/XAUuKEI4oW7Ax/H/MylBla9yJO/U3Mmvj3NI Z404hoh6hP1NHnu2M0u58bHM6vF/mM9yo5JBXdDDsh6hOJHVftj6oyXfoACoFqjSoaZ5 I5QL1IW8AHsphiICVpq5KEfX+1zYnoFsBUNVi2XUsNTuT1jY9ObtiVLk3RbJsOewVTrx l5j9douWybcx/kA9/ws8znzRFrSuwK9bcvNvNjeOiOlJsE3DKFN9UnQQU4HDbYaUMGjU GG9Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:dkim-signature; bh=sZQfoLnXFAPIemhCQbHqBClIHGxcIWwkYY30wVQIjgM=; b=P7w4L+HFKQKscGvBi7+uGhijyRl29+YIWa7RxN3aJ/XcQOYAaV5++oqa/CVhEGbItY M8O7OWnytDWgOyFO2ZJ4ZkjmcTaXtaLIVLn5YbcLKEl8DGPYOBtPE3ZB5RUiEpOIZmKA zIV/pHM8yZbBrHbwyPtETV1Gp0umsM1snw+ZjUgjTYhlaSe6ktEM79EM7fnRLgVeWmeD b9lUE7H2r3CYdDIOCZsTAv2MNWesNG8atPqF6BMuNCOmXOBjBgAVoohOQH+JAamto5/z mTOfUwNOUzRgo8Uh2wcL8ZMMgFWz7lipFAY0JG8xIKhbbfTEsYfblsXxQLpknUBOlWpo UyhQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=G7cHQPLY; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id i10si5763394ejk.428.2020.12.06.15.16.19; Sun, 06 Dec 2020 15:16:55 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=G7cHQPLY; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728446AbgLFXNs (ORCPT + 99 others); Sun, 6 Dec 2020 18:13:48 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40302 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727350AbgLFXNs (ORCPT ); Sun, 6 Dec 2020 18:13:48 -0500 Received: from mail-lf1-x143.google.com (mail-lf1-x143.google.com [IPv6:2a00:1450:4864:20::143]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B626DC0613D0 for ; Sun, 6 Dec 2020 15:13:01 -0800 (PST) Received: by mail-lf1-x143.google.com with SMTP id a8so661222lfb.3 for ; Sun, 06 Dec 2020 15:13:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=sZQfoLnXFAPIemhCQbHqBClIHGxcIWwkYY30wVQIjgM=; b=G7cHQPLYzozXqUpT9OdSrGjS6i2rZK1/3iIJS+CiC2JlHZ+kweyfS/C/liYHX0+yZN 2MrBUrEdvPvt2QsC0C4szIv5YEhlGv2SCP2E6vr9ZO83cTME4W7EUZ0uFEFVJN9/p7Ep J8ZG5p1L6/++32BsFj9AmMf1G4jc5saCu2DVImxrcyLxVBRbDh8I10NC4jesCWXlLhWw GTFqdfDX6uWmwMY3xtMwZA7yMaP6gAvi8GQNcZj6aFU2dgEHolFKZBnJd16rXT3hN1tP ngoOokyqhLfvb0CiZd8OqXBAv4+DKN0b+gGM4K9dEx01p9FqaymCpOcDocRFKYKLDmXV S/wA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=sZQfoLnXFAPIemhCQbHqBClIHGxcIWwkYY30wVQIjgM=; b=lo/+fSm8kXdEFqYBk2Qba+nfxoEw5YZuEQx1SnCJWV4eIBAsHt79u0VEnfc/daYeOp nWUbWLiMr6/lVXLalIESHjaUGGclVtfwYtka4TWhyhMJPqRpfjAzhNWOgBry787hEgl2 JHoNG1FiDbr4FUO9Q3trmsbPZ991xdUYGAXUuDnsyGTg7czHjoyhZ810A7l0KWK9aiCi PZUjZQoXL0npT4hIK4Az0xZ9c2hwps9fXOs7kVOYvc8qxcY9gCGku+LTBj4g2nLORoET Cfgtmf8OOSojM/Cd4QUB6KydbTr9tcjTeO9/ugM3qqTlnctYnTv77fUF4UTXarzBDxR3 n16A== X-Gm-Message-State: AOAM531uLZ65IFQfESkqTkQSao7/J/EGKA4pNgD7qzlAI7R4qjaeKrHF 4HOyjqbBGNtCWmLCAE6rKEYQ7fL0v5dftvatGlmdlg== X-Received: by 2002:ac2:4308:: with SMTP id l8mr6755975lfh.260.1607296379983; Sun, 06 Dec 2020 15:12:59 -0800 (PST) MIME-Version: 1.0 References: <1606901543-8957-1-git-send-email-luojiaxing@huawei.com> <1606901543-8957-2-git-send-email-luojiaxing@huawei.com> In-Reply-To: <1606901543-8957-2-git-send-email-luojiaxing@huawei.com> From: Linus Walleij Date: Mon, 7 Dec 2020 00:12:48 +0100 Message-ID: Subject: Re: [PATCH v1 1/3] gpio: gpio-hisi: Add HiSilicon GPIO support To: Luo Jiaxing Cc: Bartosz Golaszewski , Catalin Marinas , Will Deacon , Andy Shevchenko , Andy Shevchenko , "open list:GPIO SUBSYSTEM" , "linux-kernel@vger.kernel.org" , Linuxarm Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Luo! thanks for your patch! I see that Andy already provided a crowd of comments, here are some more! On Wed, Dec 2, 2020 at 10:32 AM Luo Jiaxing wrote: > +config GPIO_HISI > + tristate "HISILICON GPIO controller driver" > + depends on (ARM64 && ACPI) || COMPILE_TEST > + select GPIO_GENERIC Thanks for using the generic driver! > +#include > +#include > +#include > +#include > +#include > + > +#include "gpiolib.h" > +#include "gpiolib-acpi.h" No GPIO drivers should include these files. If you need gpiolib.h then describe in a comment right above the include why you have to do this. I don't know about gpiolig-acpi.h but not unless Andy says it is OK it is not OK. > +#define HISI_GPIO_SWPORT_DR_SET_WX 0x0 > +#define HISI_GPIO_SWPORT_DR_CLR_WX 0x4 > +#define HISI_GPIO_SWPORT_DDR_SET_WX 0x10 > +#define HISI_GPIO_SWPORT_DDR_CLR_WX 0x14 > +#define HISI_GPIO_SWPORT_DDR_ST_WX 0x18 > +#define HISI_GPIO_INTEN_SET_WX 0x20 > +#define HISI_GPIO_INTEN_CLR_WX 0x24 > +#define HISI_GPIO_INTMASK_SET_WX 0x30 > +#define HISI_GPIO_INTMASK_CLR_WX 0x34 > +#define HISI_GPIO_INTTYPE_EDGE_SET_WX 0x40 > +#define HISI_GPIO_INTTYPE_EDGE_CLR_WX 0x44 > +#define HISI_GPIO_INT_POLARITY_SET_WX 0x50 > +#define HISI_GPIO_INT_POLARITY_CLR_WX 0x54 > +#define HISI_GPIO_DEBOUNCE_SET_WX 0x60 > +#define HISI_GPIO_DEBOUNCE_CLR_WX 0x64 > +#define HISI_GPIO_INTSTATUS_WX 0x70 > +#define HISI_GPIO_PORTA_EOI_WX 0x78 > +#define HISI_GPIO_EXT_PORT_WX 0x80 > +#define HISI_GPIO_INTCOMB_MASK_WX 0xa0 > +#define HISI_GPIO_INT_DEDGE_SET 0xb0 > +#define HISI_GPIO_INT_DEDGE_CLR 0xb4 > +#define HISI_GPIO_INT_DEDGE_ST 0xb8 Nice idea with the double edge register! Hats off to the hardware engineer who created this simple yet powerful GPIO. > +#define HISI_GPIO_REG_SIZE 0x4 > +#define HISI_GPIO_REG_MAX 0x100 > +#define HISI_GPIO_PIN_NUM_MAX 32 This seems like a bit surplus definitions, I prefer to just inline these. Some use cases will go away after you start using bgpio_init(). > +struct hisi_gpio { > + struct device *dev; > + void __iomem *reg_base; > + unsigned int pin_num; I prefer "line_num", the reason I usually use the term "lines" rather than "pins" is that some GPIO lines are internal in chips and do not always go out to external pins. > + struct gpio_chip chip; > + struct irq_chip irq_chip; > + int irq; Do you need to keep irq around in the state? > +static inline u32 hisi_gpio_read_reg(struct gpio_chip *chip, > + unsigned int off) > +{ > + struct hisi_gpio *hisi_gpio = > + container_of(chip, struct hisi_gpio, chip); > + > + return chip->read_reg(hisi_gpio->reg_base + off); > +} > + > +static inline void hisi_gpio_write_reg(struct gpio_chip *chip, > + unsigned int off, u32 val) > +{ > + struct hisi_gpio *hisi_gpio = > + container_of(chip, struct hisi_gpio, chip); > + > + chip->write_reg(hisi_gpio->reg_base + off, val); > +} OK it is a bit of reusing the register accessors inside the GPIO chip generic MMIO abstraction, but to me it is really better if you just address the registers directly. The indirections through read_reg/write_reg doesn't really buy you anything. > +static void hisi_gpio_set_debounce(struct gpio_chip *chip, unsigned int off, > + u32 debounce) > +{ > + unsigned long mask = BIT(off); > + > + if (debounce) > + hisi_gpio_write_reg(chip, HISI_GPIO_DEBOUNCE_SET_WX, mask); > + else > + hisi_gpio_write_reg(chip, HISI_GPIO_DEBOUNCE_CLR_WX, mask); > +} So debounce is just on/off? No ability to set "how much" or a timer? Someone must be guessing inside the block that a certain number of ms is perfect(TM) debouncing or is there a register for this that you are not showing us? Like register 0x68 or 0x6c... > +static int hisi_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio) > +static int hisi_gpio_direction_output(struct gpio_chip *chip, > + unsigned int gpio, > + int val) > +static int hisi_gpio_direction_input(struct gpio_chip *chip, > + unsigned int gpio) These get replaced by the appropriate parameters to bgpio_init(). Read the doc in gpio-mmci.c above the function bgpio_init() for help. If you still can't figure it out, ask and describe your problem and we'll hash it out. > + /* > + * The dual-edge interrupt and other interrupt's registers do not > + * take effect at the same time. The registers of the two-edge > + * interrupts have higher priorities, the configuration of > + * the dual-edge interrupts must be disabled before the configuration > + * of other kind of interrupts. > + */ > + if (type != IRQ_TYPE_EDGE_BOTH) { > + unsigned int both = hisi_gpio_read_reg(chip, HISI_GPIO_INT_DEDGE_ST); > + > + if (both & mask) > + hisi_gpio_write_reg(chip, HISI_GPIO_INT_DEDGE_CLR, mask); > + } Nice with this comment and overall the IRQ code looks very good. Nice job! > +static void hisi_gpio_irq_enable(struct irq_data *d) > +{ > + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > + > + hisi_gpio_irq_clr_mask(d); > + hisi_gpio_write_reg(chip, HISI_GPIO_INTEN_SET_WX, BIT(irqd_to_hwirq(d))); > +} > + > +static void hisi_gpio_irq_disable(struct irq_data *d) > +{ > + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > + > + hisi_gpio_irq_set_mask(d); > + hisi_gpio_write_reg(chip, HISI_GPIO_INTEN_CLR_WX, BIT(irqd_to_hwirq(d))); > +} Interesting with a GPIO hardware that both as enable and mask bits. I can't see why, usually they just have masks but I suppose there is some reason. > +static void hisi_gpio_irq_handler(struct irq_desc *desc) > +{ > + struct hisi_gpio *hisi_gpio = irq_desc_get_handler_data(desc); > + u32 irq_msk = hisi_gpio_read_reg(&hisi_gpio->chip, > + HISI_GPIO_INTSTATUS_WX); > + struct irq_chip *irq_c = irq_desc_get_chip(desc); > + > + chained_irq_enter(irq_c, desc); > + while (irq_msk) { > + int hwirq = fls(irq_msk) - 1; > + int gpio_irq = irq_find_mapping(hisi_gpio->chip.irq.domain, > + hwirq); > + > + generic_handle_irq(gpio_irq); > + irq_msk &= ~BIT(hwirq); > + } What about just: for_each_set_bit(hwirq, &irq_msk, gc->ngpio) generic_handle_irq(irq_find_mapping(hisi_gpio->chip.irq.domain, hwirq)); > + device_for_each_child_node(dev, fwnode) { > + if (fwnode_property_read_u32(fwnode, "hisi-ngpio", > + &hisi_gpio->pin_num)) { > + dev_err(dev, > + "failed to get number of gpios for port%d and use default value instead\n", > + idx); > + hisi_gpio->pin_num = HISI_GPIO_PIN_NUM_MAX; > + } Since the registers are only 32 bits suppose this should emit a fat warning if line_num is > 32. > + dat = hisi_gpio->reg_base + HISI_GPIO_EXT_PORT_WX; > + set = hisi_gpio->reg_base + HISI_GPIO_SWPORT_DR_SET_WX; > + clr = hisi_gpio->reg_base + HISI_GPIO_SWPORT_DR_CLR_WX; > + > + res = bgpio_init(&hisi_gpio->chip, hisi_gpio->dev, HISI_GPIO_REG_SIZE, dat, set, > + clr, NULL, NULL, 0); That's a lot of variables for my taste, I usually just pass the registers to the bgpio_init() call directly, split across a few lines, but it is a matter of taste. Make sure you get the direction setting to be handled by gpio-mmio as well as described above. > + hisi_gpio->chip.direction_output = hisi_gpio_direction_output; > + hisi_gpio->chip.direction_input = hisi_gpio_direction_input; > + hisi_gpio->chip.get_direction = hisi_gpio_get_direction; So these should be handled by generic GPIO. > + hisi_gpio->chip.set_config = hisi_gpio_set_config; > + hisi_gpio->chip.ngpio = hisi_gpio->pin_num; > + hisi_gpio->chip.base = -1; But these are fine. Apart from this address everything Andy commented on as well. Yours, Linus Walleij