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=-4.1 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 B7F3FC4727F for ; Wed, 30 Sep 2020 09:21:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6E3442075F for ; Wed, 30 Sep 2020 09:21:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="UcVYPUG6" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729248AbgI3JVq (ORCPT ); Wed, 30 Sep 2020 05:21:46 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42072 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729241AbgI3JVk (ORCPT ); Wed, 30 Sep 2020 05:21:40 -0400 Received: from mail-lf1-x141.google.com (mail-lf1-x141.google.com [IPv6:2a00:1450:4864:20::141]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 14992C0613D1 for ; Wed, 30 Sep 2020 02:21:40 -0700 (PDT) Received: by mail-lf1-x141.google.com with SMTP id q8so1268076lfb.6 for ; Wed, 30 Sep 2020 02:21:39 -0700 (PDT) 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=nDALLkEBZ3g+XnHosKYd5kNAL+8/krUGOWN367VieQw=; b=UcVYPUG6V9xpIygyNaSFeeW11q0jC3otxEzCW5tdEYBbg2lLdQhtIH7CzmwXgKaiS3 1BN34bY6ziO20ONgBRrIi18eR/G5aXmO560OEv6xdTyBRf4JCzkCpB427MuiEvyInR0A K1YF0fdMW0euDWU9AxB7jUAjIjslPtI2sA3sjcWu3vR6aBxRwHPJrSTscaBI0qtD5NRu reohlHs1BYRxasdl0rGel7ICYX3AyV6n+L4LSDD1LD78OLCIEjdMmVasW84Auj566N/r 8/jTnB3duHLfevNiq7WjY3GFi/su0eLx3h548CZm0cVu9W5GLXxydMB4vCcbwB6fwx55 ejew== 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=nDALLkEBZ3g+XnHosKYd5kNAL+8/krUGOWN367VieQw=; b=QWFQub8mWBKn2AJh20liQ/d9XDksM/BkSeVthZMV2HWDSfvWjoKXSQIfR4+6ctFJt5 IV3XHgcchG6w01kuIznncKzs4iBMARnq0TZwH/3urYNOmZBsKa0SxkFn2799I+/igyc6 MHhKansCeRt3co8AABrBhjvMSBn0BV3Lhb64Ynuy8euE6OpGbaGmQdaU8PZ52rFM5hJ8 4c5SQC6gBHAISvLAAz1Dg7LMzQgYou6v+WUKfOFcBy5hJMTKlUlZrN7Bl2jEBTVD1oZR a2edSpH1qglc3Fx5o3yb50yndZtp+Q7QRmfxaCynAvBmuupQWezP2NcyQnlnKS/53PgR 0xWA== X-Gm-Message-State: AOAM532wUfKjRm9j3ukNxxU20Mc9iKXYVe2ySw19YeWpmcOT3dzRK20e cW0Bsq67voV/0eRriBBYxlcSgwSavpqCSSIENB1eAg== X-Google-Smtp-Source: ABdhPJwA8MFQ966zPAFFRvo1oCHuuRNQ2mM89Bj+Zwhrq8WSo27dINGhXF6SeriHIm3TsyAfpOcS/0a0OIVG6S2W3I8= X-Received: by 2002:a19:6c2:: with SMTP id 185mr512735lfg.441.1601457698203; Wed, 30 Sep 2020 02:21:38 -0700 (PDT) MIME-Version: 1.0 References: <20200903133528.8595-1-lars.povlsen@microchip.com> <20200903133528.8595-2-lars.povlsen@microchip.com> <87r1r5wky3.fsf@soft-dev15.microsemi.net> In-Reply-To: <87r1r5wky3.fsf@soft-dev15.microsemi.net> From: Linus Walleij Date: Wed, 30 Sep 2020 11:21:27 +0200 Message-ID: Subject: Re: [PATCH v2 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-mchp-sgpio driver To: Lars Povlsen Cc: Rob Herring , Microchip Linux Driver Support , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , "open list:GPIO SUBSYSTEM" , Linux ARM , "linux-kernel@vger.kernel.org" , Alexandre Belloni Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org Hi Lars, thanks for working on this! On Sun, Sep 13, 2020 at 9:11 PM Lars Povlsen wrote: > > What I do not understand is why this GPIO controller is placed in the > > bindings of the pin controllers? Do you plan to add pin control > > properties to the bindings in the future? > > I have made provisions for some of the generic pinconf parameters, and > since the controller also has support for some alternate modes like > (syncronized) blink at various rates, I thought I better add it as > pinctrl straight away. OK fair enough let's keep the bindings here. BTW the latter function sounds like some kind of PWM? > >> + gpio-controller: true > >> + > >> + '#gpio-cells': > >> + description: GPIO consumers must specify four arguments, first the > >> + port number, then the bit number, then a input/output flag and > >> + finally the GPIO flags (from include/dt-bindings/gpio/gpio.h). > >> + The dt-bindings/gpio/mchp-sgpio.h file define manifest constants > >> + PIN_INPUT and PIN_OUTPUT. > >> + const: 4 > > > > I do not follow this new third input/output flag at all. > > Its actually a sort of bank address, since the individual "pins" are > unidirectional. I'm a bit confused here... The standard advice for any "banked" GPIOs is to represent each "bank" as a separate node (with a corresponding gpio_chip in the Linux kernel). Then you can just use the standard bindings to pick a line from one of these nodes. > The PIN_INPUT/PIN_OUTPUT is defined in similar fashion in other pinctrl > binding header files... I can drop the define and use, but as it will be > used to address individual pins, I think it adds to readability. Hmmm. What makes these names expecially confusing is the Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml defines: input-enable input-disable output-enable output-high output-low In the Linux kernel further there is: include/linux/pinctrl/pinconf-generic.h that defines: PIN_CONFIG_INPUT_ENABLE PIN_CONFIG_OUTPUT_ENABLE PIN_CONFIG_OUTPUT Since you are using the pin control framework this gets really hard to hash out. I don't really understand why it is needed. > Like this (excerpts from a DT with a switchdev driver using SFP's and > LED's on sgpio): > > /{ > leds { > compatible = "gpio-leds"; > led@0 { > label = "eth60:yellow"; > gpios = <&sgpio1 28 0 PIN_OUTPUT GPIO_ACTIVE_LOW>; > default-state = "off"; > }; > ... > }; > }; If what you intend to achieve is to make the GPIO come up in output mode, you can either just have the driver do that as needed by the consumer. If you absolutely have to do it in the device tree, then implement pin control (pin config) and have it something like this: leds { compatible = "gpio-leds"; pinctrl-names = "default"; pinctrl-0 = <&my_led_pinctrl>; led@0 { label = "eth60:yellow"; gpios = <&sgpio1 28 GPIO_ACTIVE_LOW>; default-state = "off"; }; ... my_led_pinctrl: pinctrl-led { pins = "gpio95"; // Just an example way of referring to the pin bias-disable; output-enable; }; }; > >> + microchip,sgpio-port-ranges: > >> + description: This is a sequence of tuples, defining intervals of > >> + enabled ports in the serial input stream. The enabled ports must > >> + match the hardware configuration in order for signals to be > >> + properly written/read to/from the controller holding > >> + registers. Being tuples, then number of arguments must be > >> + even. The tuples mast be ordered (low, high) and are > >> + inclusive. Arguments must be between 0 and 31. > >> + $ref: /schemas/types.yaml#/definitions/uint32-array > >> + minItems: 2 > >> + maxItems: 64 > > > > And you are *absolutely sure* that you can't just figure this out > > from the compatible string? Or add a few compatible strings for > > the existing variants? > > Yes, this really needs to be configured for each board individually - > and cant be probed. It defines how the bitstream to/from the shift > registers is constructed/demuxed. And you have considered the option of simply letting the driver check which board we are then? The property at the very top of the device tree. if (of_machine_is_compatible("my_board")) { .... } else if (of_machine_is_compatible("my_other_board")) { .... } So that you simply use the board compatible string to determine this? > >> +/* mchp-sgpio specific pin type defines */ > >> +#undef PIN_OUTPUT > >> +#undef PIN_INPUT > >> +#define PIN_OUTPUT 0 > >> +#define PIN_INPUT 1 > > > > I'm not a fan of this. It seems like something that should be set in > > response to the gpiochip callbacks .direction_input and > > .direction_output callbacks. > > > > As I tried to explain above, its a part of the pin address - aka bank > selector - whether your are accessing the input or the output side. And > since the directions have totally different - and concurrent - use, they > need to be individually addressed, not "configured". > > In the example presented, sgpio2-p28b0 IN is loss-of-signal, and the > OUT is the sfp tx-disable control. I suspect the proper way to do it is to create one node for the input side and one node for the output side and also create two different gpio chips in the kernel. my-device { compatible = "my-device"; gpioin: input-gpio { .... }; gpioout: output-gpio { .... }; }; Note: I didn't think over the naming in this example. You will need code in your driver to parse the subnodes and populate two gpio_chips. Yours, Linus Walleij 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=-4.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,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 173ADC4727F for ; Wed, 30 Sep 2020 09:26:12 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A869B20739 for ; Wed, 30 Sep 2020 09:26:11 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="dbmfINM0"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="UcVYPUG6" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A869B20739 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id: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=12JqNkN9eqa7bzqY2gQQnnkQ+2NCtbmYAZNY/bqCMzU=; b=dbmfINM0u/eM8Vmp1t6CVqW8k 7YH3nmEsuX8l1sr1m6n04vSuIpGaAwrIvpKHtR6K75HOf8ITInmaqOpsvMTSFQH1TQ59idUls6OQR bsFZUEcekrqO0xx5dgBxVIJZDeyNoTPsfE2nOaIZJUmh0j6Ska2G2NK1VmnfuwIZzQH7XvC0Rpvmi eZ1tUnNUsidbBaWX0E38r9JLc6BX7kyTtCKXSBmdPNvXqoC2yW/6wumYLEGSNIOCYby4iTm/ODnKB OSQ4PyjWLD6Rx7VkhbS6+oGfUv4pGeul/s0orJ9uVz9LmXASAr3PVUoZ/AZHekejQIpRoq9Sijzar iO4WD3A8Q==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kNYLg-0003KH-T3; Wed, 30 Sep 2020 09:24:33 +0000 Received: from mail-lf1-x141.google.com ([2a00:1450:4864:20::141]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kNYIu-0002Ac-HT for linux-arm-kernel@lists.infradead.org; Wed, 30 Sep 2020 09:21:52 +0000 Received: by mail-lf1-x141.google.com with SMTP id u8so1286946lff.1 for ; Wed, 30 Sep 2020 02:21:39 -0700 (PDT) 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=nDALLkEBZ3g+XnHosKYd5kNAL+8/krUGOWN367VieQw=; b=UcVYPUG6V9xpIygyNaSFeeW11q0jC3otxEzCW5tdEYBbg2lLdQhtIH7CzmwXgKaiS3 1BN34bY6ziO20ONgBRrIi18eR/G5aXmO560OEv6xdTyBRf4JCzkCpB427MuiEvyInR0A K1YF0fdMW0euDWU9AxB7jUAjIjslPtI2sA3sjcWu3vR6aBxRwHPJrSTscaBI0qtD5NRu reohlHs1BYRxasdl0rGel7ICYX3AyV6n+L4LSDD1LD78OLCIEjdMmVasW84Auj566N/r 8/jTnB3duHLfevNiq7WjY3GFi/su0eLx3h548CZm0cVu9W5GLXxydMB4vCcbwB6fwx55 ejew== 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=nDALLkEBZ3g+XnHosKYd5kNAL+8/krUGOWN367VieQw=; b=SyqwiRwXPtk8GE2IhgiCF++i8cJND+yflIgS/mql0sVek/fqmBkA0n/KQllXuW+19U wclf5KFQ9We1bHTraO4iAsHR6ex1jSLEQ/ZAtOwps4DbwprcHQvRp3A8mkY+x5sU3XC5 szpUusGLoxDiSfFzyesRuGSnJ1XLCiPkg2oO0M/fjB/orIpgb8StCg9ldeL76LPi+i8l jXVfMJfplnJcvDjMjcbuADC31a7tmQjfSvHDqigtDrBojkuKrVeI9KDfG2TTqRRtVPOA Dld0rt8P+eKq9vdwnF8dZK5WrY6x2Ud0tsuDMS27TUW98Ip2ICkyE15vQQyCQH1nnyNW 9eEA== X-Gm-Message-State: AOAM533xnjbuJUG3INlL3sP36Lwgzb5u1S2WpeX6EUeP3nqvq1QvEuzL trssHG2u2dfy+4YunsHeL3Ls/hYRSH49B9CziNcjug== X-Google-Smtp-Source: ABdhPJwA8MFQ966zPAFFRvo1oCHuuRNQ2mM89Bj+Zwhrq8WSo27dINGhXF6SeriHIm3TsyAfpOcS/0a0OIVG6S2W3I8= X-Received: by 2002:a19:6c2:: with SMTP id 185mr512735lfg.441.1601457698203; Wed, 30 Sep 2020 02:21:38 -0700 (PDT) MIME-Version: 1.0 References: <20200903133528.8595-1-lars.povlsen@microchip.com> <20200903133528.8595-2-lars.povlsen@microchip.com> <87r1r5wky3.fsf@soft-dev15.microsemi.net> In-Reply-To: <87r1r5wky3.fsf@soft-dev15.microsemi.net> From: Linus Walleij Date: Wed, 30 Sep 2020 11:21:27 +0200 Message-ID: Subject: Re: [PATCH v2 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-mchp-sgpio driver To: Lars Povlsen X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200930_052140_862497_60C6E3DD X-CRM114-Status: GOOD ( 38.65 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Alexandre Belloni , "linux-kernel@vger.kernel.org" , Microchip Linux Driver Support , "open list:GPIO SUBSYSTEM" , Rob Herring , Linux ARM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Lars, thanks for working on this! On Sun, Sep 13, 2020 at 9:11 PM Lars Povlsen wrote: > > What I do not understand is why this GPIO controller is placed in the > > bindings of the pin controllers? Do you plan to add pin control > > properties to the bindings in the future? > > I have made provisions for some of the generic pinconf parameters, and > since the controller also has support for some alternate modes like > (syncronized) blink at various rates, I thought I better add it as > pinctrl straight away. OK fair enough let's keep the bindings here. BTW the latter function sounds like some kind of PWM? > >> + gpio-controller: true > >> + > >> + '#gpio-cells': > >> + description: GPIO consumers must specify four arguments, first the > >> + port number, then the bit number, then a input/output flag and > >> + finally the GPIO flags (from include/dt-bindings/gpio/gpio.h). > >> + The dt-bindings/gpio/mchp-sgpio.h file define manifest constants > >> + PIN_INPUT and PIN_OUTPUT. > >> + const: 4 > > > > I do not follow this new third input/output flag at all. > > Its actually a sort of bank address, since the individual "pins" are > unidirectional. I'm a bit confused here... The standard advice for any "banked" GPIOs is to represent each "bank" as a separate node (with a corresponding gpio_chip in the Linux kernel). Then you can just use the standard bindings to pick a line from one of these nodes. > The PIN_INPUT/PIN_OUTPUT is defined in similar fashion in other pinctrl > binding header files... I can drop the define and use, but as it will be > used to address individual pins, I think it adds to readability. Hmmm. What makes these names expecially confusing is the Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml defines: input-enable input-disable output-enable output-high output-low In the Linux kernel further there is: include/linux/pinctrl/pinconf-generic.h that defines: PIN_CONFIG_INPUT_ENABLE PIN_CONFIG_OUTPUT_ENABLE PIN_CONFIG_OUTPUT Since you are using the pin control framework this gets really hard to hash out. I don't really understand why it is needed. > Like this (excerpts from a DT with a switchdev driver using SFP's and > LED's on sgpio): > > /{ > leds { > compatible = "gpio-leds"; > led@0 { > label = "eth60:yellow"; > gpios = <&sgpio1 28 0 PIN_OUTPUT GPIO_ACTIVE_LOW>; > default-state = "off"; > }; > ... > }; > }; If what you intend to achieve is to make the GPIO come up in output mode, you can either just have the driver do that as needed by the consumer. If you absolutely have to do it in the device tree, then implement pin control (pin config) and have it something like this: leds { compatible = "gpio-leds"; pinctrl-names = "default"; pinctrl-0 = <&my_led_pinctrl>; led@0 { label = "eth60:yellow"; gpios = <&sgpio1 28 GPIO_ACTIVE_LOW>; default-state = "off"; }; ... my_led_pinctrl: pinctrl-led { pins = "gpio95"; // Just an example way of referring to the pin bias-disable; output-enable; }; }; > >> + microchip,sgpio-port-ranges: > >> + description: This is a sequence of tuples, defining intervals of > >> + enabled ports in the serial input stream. The enabled ports must > >> + match the hardware configuration in order for signals to be > >> + properly written/read to/from the controller holding > >> + registers. Being tuples, then number of arguments must be > >> + even. The tuples mast be ordered (low, high) and are > >> + inclusive. Arguments must be between 0 and 31. > >> + $ref: /schemas/types.yaml#/definitions/uint32-array > >> + minItems: 2 > >> + maxItems: 64 > > > > And you are *absolutely sure* that you can't just figure this out > > from the compatible string? Or add a few compatible strings for > > the existing variants? > > Yes, this really needs to be configured for each board individually - > and cant be probed. It defines how the bitstream to/from the shift > registers is constructed/demuxed. And you have considered the option of simply letting the driver check which board we are then? The property at the very top of the device tree. if (of_machine_is_compatible("my_board")) { .... } else if (of_machine_is_compatible("my_other_board")) { .... } So that you simply use the board compatible string to determine this? > >> +/* mchp-sgpio specific pin type defines */ > >> +#undef PIN_OUTPUT > >> +#undef PIN_INPUT > >> +#define PIN_OUTPUT 0 > >> +#define PIN_INPUT 1 > > > > I'm not a fan of this. It seems like something that should be set in > > response to the gpiochip callbacks .direction_input and > > .direction_output callbacks. > > > > As I tried to explain above, its a part of the pin address - aka bank > selector - whether your are accessing the input or the output side. And > since the directions have totally different - and concurrent - use, they > need to be individually addressed, not "configured". > > In the example presented, sgpio2-p28b0 IN is loss-of-signal, and the > OUT is the sfp tx-disable control. I suspect the proper way to do it is to create one node for the input side and one node for the output side and also create two different gpio chips in the kernel. my-device { compatible = "my-device"; gpioin: input-gpio { .... }; gpioout: output-gpio { .... }; }; Note: I didn't think over the naming in this example. You will need code in your driver to parse the subnodes and populate two gpio_chips. Yours, Linus Walleij _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel