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 X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 144DEC433E7 for ; Sat, 17 Oct 2020 05:34:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B543920829 for ; Sat, 17 Oct 2020 05:34:52 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=0x0f.com header.i=@0x0f.com header.b="Hc8aQmwn" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2411781AbgJQFev (ORCPT ); Sat, 17 Oct 2020 01:34:51 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53052 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2411770AbgJQFeo (ORCPT ); Sat, 17 Oct 2020 01:34:44 -0400 Received: from mail-wm1-x341.google.com (mail-wm1-x341.google.com [IPv6:2a00:1450:4864:20::341]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7C1AAC0610DF for ; Fri, 16 Oct 2020 18:57:47 -0700 (PDT) Received: by mail-wm1-x341.google.com with SMTP id q5so5224988wmq.0 for ; Fri, 16 Oct 2020 18:57:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=0x0f.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=nuh/bmn3RkFq4gADcA0ZAUJL6az2qyr09AghLknddnE=; b=Hc8aQmwnLL+wERyyQ7CpO51Fsn3derfmkT8umMZkwXPkhXFcXuK1+srj5bzWdyGS8X w44FRI2rzKxxk+WBy/GE7xvWOVWLkwNIvcfxSm9c9uuhAX+67wPWEgXL3kgROKrUsuvf zsmczeVNeaUiEkBC4f+DsFxJ2lpdyqlb7kjm8= 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=nuh/bmn3RkFq4gADcA0ZAUJL6az2qyr09AghLknddnE=; b=H/A3JDRk9V6vGwvHWED7ZYRWeSI1up/zUw+IeQ97UhxSo7pFVYVb4TJqt4Hcq7Y7P5 84c3n2oklqPJmjU6IaS3POkITux2FvCMPMELLJxmI+onCJzMyBlAtHJ6UhP36L+Iw5SF fPD3GmRVb50y8ja6tNK696iNuF9FydJh2vkoYdm0Gt2zUGmIIsyYOD1cXPxw2bWaMPwX MjDGto+ZPsfz1t2TeO8JS09HKWygzl8tg61F5HsJ9+WenWJR7UNlb5PSYZGr9pw9WM8D hYUel1urZkGpJObMcyiDKW62NgtNaowvVOqPCjPIYi0ctpG1nAEfjyhnc/VVVHBgVele 7Xrg== X-Gm-Message-State: AOAM5333hRsEu5nih+67MAD+vd3xxvpK7aq3SxZ6/j1HwlfB5r4Y39Uf zqbET/mhHmdDiXB8dbiHFOReuJbjNGLt5mD3aI34NA== X-Google-Smtp-Source: ABdhPJz2dwlicRXSki8sbde0WGJf342Dhw9U8otxbpBoITFVZF9Vdk/vRb8AtHXnj/BTbponqM3On9YyYJ9zovqDyQ0= X-Received: by 2002:a7b:c935:: with SMTP id h21mr6120268wml.99.1602899865888; Fri, 16 Oct 2020 18:57:45 -0700 (PDT) MIME-Version: 1.0 References: <20201011024831.3868571-1-daniel@0x0f.com> <20201011024831.3868571-4-daniel@0x0f.com> In-Reply-To: From: Daniel Palmer Date: Sat, 17 Oct 2020 10:57:35 +0900 Message-ID: Subject: Re: [PATCH 3/5] gpio: msc313: MStar MSC313 GPIO driver To: Linus Walleij Cc: "open list:GPIO SUBSYSTEM" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Linux ARM , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org Hi Linus On Sat, 17 Oct 2020 at 01:56, Linus Walleij wrote: > (...) > > > +config GPIO_MSC313 > > + bool "MStar MSC313 GPIO support" > > + default y if ARCH_MSTARV7 > > + depends on ARCH_MSTARV7 > > + select GPIO_GENERIC > > Selecting GPIO_GENERIC, that is good. > But you're not using it, because you can't. > This chip does not have the bits lined up nicely > in one register, instead there seems to be something > like one register per line, right? > So skip GPIO_GENERIC. Well spotted. Copy/paste fail on my side :). > > +#define MSC313_GPIO_IN BIT(0) > > +#define MSC313_GPIO_OUT BIT(4) > > +#define MSC313_GPIO_OEN BIT(5) > > + > > +#define MSC313_GPIO_BITSTOSAVE (MSC313_GPIO_OUT | MSC313_GPIO_OEN) > > Some comment here telling us why these need saving and > not others. There is a comment near to the save function that explains it I think. When the hardware goes into low power mode with the CPU turned off the register contents are lost and those two bits are the only ones that are writable from what I can tell. I'll add an extra comment above that line. > > +#define FUART_NAMES \ > > + MSC313_PINNAME_FUART_RX, \ > > + MSC313_PINNAME_FUART_TX, \ > > + MSC313_PINNAME_FUART_CTS, \ > > + MSC313_PINNAME_FUART_RTS > > + > > +#define OFF_FUART_RX 0x50 > > +#define OFF_FUART_TX 0x54 > > +#define OFF_FUART_CTS 0x58 > > +#define OFF_FUART_RTS 0x5c > > + > > +#define FUART_OFFSETS \ > > + OFF_FUART_RX, \ > > + OFF_FUART_TX, \ > > + OFF_FUART_CTS, \ > > + OFF_FUART_RTS > > This looks a bit strange. The GPIO driver should not really > have to know about any other use cases for pins than > GPIO. But I guess it is intuitive for the driver. > > > Same with all these. I suppose it is the offsets of stuff > that would be there unless we were using it for GPIO. The pad FUART_RX can't move but the function FUART_RX can. If the function FUART_RX (or another function) isn't on the pad/pin FUART_RX it's connected to the GPIO block. Even more confusingly some of the other chips (SSD201/SSD202) have pads called GPIO1, GPIO2 etc that only have GPIO functionality but the offsets of the registers to control the GPIO on those pads might not have a relation to the name. GPIO1 isn't gpio_base + (1 * 4) and instead some random address. Basically using the pad name as the name of the GPIO made sense because it's fixed and the pad name and offset are the same with all of the chips I've seen so far. > > +static int msc313_gpio_to_irq(struct gpio_chip *chip, unsigned int offset) > > +{ > > + struct msc313_gpio *gpio = gpiochip_get_data(chip); > > +> + > > > + return gpio->irqs[offset]; > > +} > > Please do not use custom IRQ handling like this. > As there seems to be one IRQ per line, look into using > > select GPIOLIB_IRQCHIP > select IRQ_DOMAIN_HIERARCHY > > See for example in gpio-ixp4xx.c how we deal with > hiearchical GPIO IRQs. > Use hierarchical generic GPIO IRQs for these. > > Assign ->fwnode, ->parent_domain, ->child_to_parent_hwirq, > and probably also ->handler on the struct gpio_irq_chip *. > > Skip assigning gpiochip->to_irq, the generic code will > handle that. > > Again see gpio-ixp4xx.c for an example. I'll look into this. I don't have datasheets so I'm working from some crusty header files from the vendor kernel but there isn't one irq per line from what I can tell. There seems to have been 4 spare lines on an interrupt controller so they wired GPIOs to them. Thank you for the comments. I'll send a v2 in a few days. Thanks, Daniel