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.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS 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 A4F98C433E2 for ; Sun, 13 Sep 2020 19:11:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 3DC5921D80 for ; Sun, 13 Sep 2020 19:11:55 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=microchip.com header.i=@microchip.com header.b="MUJf0Wxy" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725938AbgIMTLy (ORCPT ); Sun, 13 Sep 2020 15:11:54 -0400 Received: from esa1.microchip.iphmx.com ([68.232.147.91]:48942 "EHLO esa1.microchip.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725936AbgIMTLx (ORCPT ); Sun, 13 Sep 2020 15:11:53 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=microchip.com; i=@microchip.com; q=dns/txt; s=mchp; t=1600024312; x=1631560312; h=references:from:to:cc:subject:in-reply-to:date: message-id:mime-version; bh=+VgAIgh+Py6mj3na0ijrCwl0Z/NjJ7JtWl8Awedh1FA=; b=MUJf0WxysCxuB7+IlnMt+OjzfenZ2XbASqcIya6BGGbhdtBwdtI8fbQy KOSCDRIbiTXZfSnmc3pRY3/ZyhmGS/+oXjbhmCoIMQIAD2+0Qseytz6T8 sov882jWnulzK+8/ZeDcy6RZx67Qq7jOm31BUji581HiJ8IPp7MBOfFy7 zUm9xzOxxnIcy9B0QPJXjvv7g5zsZ8DPR5Q9w3RLVFFHSCWIjj+5cB1Mq h+f5TRWME/1oUsAzgqldAcp156QJ/zJGKUZZcl7KFO2EwOkDLkEBUOn+9 kmd+r1Rk9QoEN/6+Cn3kTLpE9oqY+Mx+bPIBJN9SGUUiez2P0/fASkIxE g==; IronPort-SDR: WhudMxrX8gTYscexf1bBtUECJhaQfGrVZURQ5Z2R7JfYTIOwHxsjpQCYuDhfytqh/f9LMIDB3i N0pCPkevfMA7Idm3l8J7Y6ZtKS/Al0/vF59HJmr5aUfLZlZhkkkLkmvxDloUC//WZkwtbEvRqn KIHVHsz3Z1JCek9S+JDkSm0BF1Og1dQ6DvvBsZgQ9+052zvPc6nAnLMJHmjP8WQR9mwyXvTuzY pSiugo+sD2jKWf2hWdWPZZ87U6BTeXINUOUQBL9bmLkNwRyNhCiZ8H15JSsuN9ciY5HjXhlI67 9M0= X-IronPort-AV: E=Sophos;i="5.76,423,1592895600"; d="scan'208";a="95548931" Received: from smtpout.microchip.com (HELO email.microchip.com) ([198.175.253.82]) by esa1.microchip.iphmx.com with ESMTP/TLS/AES256-SHA256; 13 Sep 2020 12:11:51 -0700 Received: from chn-vm-ex03.mchp-main.com (10.10.85.151) by chn-vm-ex04.mchp-main.com (10.10.85.152) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1979.3; Sun, 13 Sep 2020 12:11:47 -0700 Received: from soft-dev15.microsemi.net.microchip.com (10.10.115.15) by chn-vm-ex03.mchp-main.com (10.10.85.151) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1979.3 via Frontend Transport; Sun, 13 Sep 2020 12:11:45 -0700 References: <20200903133528.8595-1-lars.povlsen@microchip.com> <20200903133528.8595-2-lars.povlsen@microchip.com> From: Lars Povlsen To: Linus Walleij CC: Lars Povlsen , 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 Subject: Re: [PATCH v2 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-mchp-sgpio driver In-Reply-To: Date: Sun, 13 Sep 2020 21:11:48 +0200 Message-ID: <87r1r5wky3.fsf@soft-dev15.microsemi.net> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-gpio-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org Linus Walleij writes: > Hi Lars, > > thanks for your patch! You're welcome - thank you for you taking time to review it! > > On Thu, Sep 3, 2020 at 3:35 PM Lars Povlsen wrote: > >> This adds DT bindings for the Microsemi/Microchip SGPIO controller, > > 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. > >> +description: | >> + By using a serial interface, the SIO controller significantly extend >> + the number of available GPIOs with a minimum number of additional >> + pins on the device. The primary purpose of the SIO controllers is to >> + connect control signals from SFP modules and to act as an LED >> + controller. > > This doesn't sound like it will ever be pin control? above. > >> + 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. 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. 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"; }; ... }; }; &axi { sfp_eth60: sfp-eth60 { compatible = "sff,sfp"; i2c-bus = <&i2c152>; tx-disable-gpios = <&sgpio2 28 0 PIN_OUTPUT GPIO_ACTIVE_LOW>; rate-select0-gpios = <&sgpio2 28 1 PIN_OUTPUT GPIO_ACTIVE_HIGH>; los-gpios = <&sgpio2 28 0 PIN_INPUT GPIO_ACTIVE_HIGH>; mod-def0-gpios = <&sgpio2 28 1 PIN_INPUT GPIO_ACTIVE_LOW>; tx-fault-gpios = <&sgpio2 28 2 PIN_INPUT GPIO_ACTIVE_HIGH>; }; ... }; > - If it is a property of the hardware, it is something the driver should > handle by determining which hardware it is from the compatible > string. > > - If it is a configuration it should be turned into something that is generic > and useful for *all* GPIO controllers. If it is pin config it should use > the pinconf bindings rather than shortcuts like this, but I think it is > something the driver can do as an effect of the pin being requested > as input or output in the operating system, depending on who the > consumer is. Linux for example has GPIOD_OUT_LOW, > GPIOD_OUT_HIGH, GPIOD_IN, GPIOD_ASIS... > > - Is it not just a hog? We have bindings for those. I hope the above shed some light on this. > >> + 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. >> + microchip,sgpio-frequency: >> + description: The sgpio controller frequency (Hz). This dictates >> + the serial bitstream speed, which again affects the latency in >> + getting control signals back and forth between external shift >> + registers. The speed must be no larger than half the system >> + clock, and larger than zero. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + minimum: 1 >> + default: 12500000 > > I understand why you need this binding now, OK. > >> +/* 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. > Yours, > Linus Walleij ---Lars -- Lars Povlsen, Microchip 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=-5.4 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 108D9C433E2 for ; Sun, 13 Sep 2020 19:14: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 9B382221EB for ; Sun, 13 Sep 2020 19:14: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="ZvfTci0/"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=microchip.com header.i=@microchip.com header.b="ahk+dH4t" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9B382221EB Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=microchip.com 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:MIME-Version:Message-ID:Date:In-Reply-To:Subject:To: From:References:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=GDHwgNXQE0/6YGFd7Uej2Aj4cR8hBkM+RKSYK/Most8=; b=ZvfTci0/+DQLppiInv4itCXNt BBa6Du96GSQUqpSAHsK0/vuXK0OphsR1iNOMrBTOpSYQKREabdyqYiTwJP2NlWcYkgg8m9F5GQcTF qGT7+LkYyJfy5dtL79IDVpgALko08zCZAmJHx6szzq0DIDydw26zEqwRtdAUXPnB/0tzED/Gg/55B YN/iuzqyOk6wLAa4QBLhsvvgtreEYtSQ42U/7DoizZM9wKcNXwb9QmA6S+a/zEnT2b5O1nnsz9vyO 1Nyu3rsIVM0DeZnMAnys8K3M77bhfvdrn2ndCEqJdoviQeZdtvGoYzFwAONQ1SPWdPWY924kAGc3l lXeSXgg+g==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kHXPr-0003xn-NJ; Sun, 13 Sep 2020 19:11:59 +0000 Received: from esa1.microchip.iphmx.com ([68.232.147.91]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kHXPo-0003x7-PH for linux-arm-kernel@lists.infradead.org; Sun, 13 Sep 2020 19:11:57 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=microchip.com; i=@microchip.com; q=dns/txt; s=mchp; t=1600024316; x=1631560316; h=references:from:to:cc:subject:in-reply-to:date: message-id:mime-version; bh=+VgAIgh+Py6mj3na0ijrCwl0Z/NjJ7JtWl8Awedh1FA=; b=ahk+dH4t7nJbDGNYYw5hXmYa+IcO1G8i1uGeJrfW6kFZjjEDVutU2DyI ui+XKNF1H0lbaJyiZmPcnBwv2POxLOOuCUxnPQzWeef+s7Xm520+5JOS9 S/caftZ2l0aSqL+FCshH6MokBrT1lF1Pdvszdw8HsJXuo81r78MJPx840 //b+7MFoFPWkrz9eBfjqTmxQ4pUn+HG7Dlajpq/Q1J2/ViAeUVuDLdOyl R9ZlQR3TnCicq616gFFLirYfNkEGfZsWHHCPeD0B83KscoMdjXsFosCY6 YWnveNDj/K1TR2e0y7j4pS2RKyACijkhH0cCetBsLcrMGhWFNa4LOBv+W w==; IronPort-SDR: WhudMxrX8gTYscexf1bBtUECJhaQfGrVZURQ5Z2R7JfYTIOwHxsjpQCYuDhfytqh/f9LMIDB3i N0pCPkevfMA7Idm3l8J7Y6ZtKS/Al0/vF59HJmr5aUfLZlZhkkkLkmvxDloUC//WZkwtbEvRqn KIHVHsz3Z1JCek9S+JDkSm0BF1Og1dQ6DvvBsZgQ9+052zvPc6nAnLMJHmjP8WQR9mwyXvTuzY pSiugo+sD2jKWf2hWdWPZZ87U6BTeXINUOUQBL9bmLkNwRyNhCiZ8H15JSsuN9ciY5HjXhlI67 9M0= X-IronPort-AV: E=Sophos;i="5.76,423,1592895600"; d="scan'208";a="95548931" Received: from smtpout.microchip.com (HELO email.microchip.com) ([198.175.253.82]) by esa1.microchip.iphmx.com with ESMTP/TLS/AES256-SHA256; 13 Sep 2020 12:11:51 -0700 Received: from chn-vm-ex03.mchp-main.com (10.10.85.151) by chn-vm-ex04.mchp-main.com (10.10.85.152) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1979.3; Sun, 13 Sep 2020 12:11:47 -0700 Received: from soft-dev15.microsemi.net.microchip.com (10.10.115.15) by chn-vm-ex03.mchp-main.com (10.10.85.151) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1979.3 via Frontend Transport; Sun, 13 Sep 2020 12:11:45 -0700 References: <20200903133528.8595-1-lars.povlsen@microchip.com> <20200903133528.8595-2-lars.povlsen@microchip.com> From: Lars Povlsen To: Linus Walleij Subject: Re: [PATCH v2 1/3] dt-bindings: pinctrl: Add bindings for pinctrl-mchp-sgpio driver In-Reply-To: Date: Sun, 13 Sep 2020 21:11:48 +0200 Message-ID: <87r1r5wky3.fsf@soft-dev15.microsemi.net> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200913_151156_948202_35C2E217 X-CRM114-Status: GOOD ( 33.50 ) 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 , Lars Povlsen , 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 Linus Walleij writes: > Hi Lars, > > thanks for your patch! You're welcome - thank you for you taking time to review it! > > On Thu, Sep 3, 2020 at 3:35 PM Lars Povlsen wrote: > >> This adds DT bindings for the Microsemi/Microchip SGPIO controller, > > 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. > >> +description: | >> + By using a serial interface, the SIO controller significantly extend >> + the number of available GPIOs with a minimum number of additional >> + pins on the device. The primary purpose of the SIO controllers is to >> + connect control signals from SFP modules and to act as an LED >> + controller. > > This doesn't sound like it will ever be pin control? above. > >> + 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. 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. 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"; }; ... }; }; &axi { sfp_eth60: sfp-eth60 { compatible = "sff,sfp"; i2c-bus = <&i2c152>; tx-disable-gpios = <&sgpio2 28 0 PIN_OUTPUT GPIO_ACTIVE_LOW>; rate-select0-gpios = <&sgpio2 28 1 PIN_OUTPUT GPIO_ACTIVE_HIGH>; los-gpios = <&sgpio2 28 0 PIN_INPUT GPIO_ACTIVE_HIGH>; mod-def0-gpios = <&sgpio2 28 1 PIN_INPUT GPIO_ACTIVE_LOW>; tx-fault-gpios = <&sgpio2 28 2 PIN_INPUT GPIO_ACTIVE_HIGH>; }; ... }; > - If it is a property of the hardware, it is something the driver should > handle by determining which hardware it is from the compatible > string. > > - If it is a configuration it should be turned into something that is generic > and useful for *all* GPIO controllers. If it is pin config it should use > the pinconf bindings rather than shortcuts like this, but I think it is > something the driver can do as an effect of the pin being requested > as input or output in the operating system, depending on who the > consumer is. Linux for example has GPIOD_OUT_LOW, > GPIOD_OUT_HIGH, GPIOD_IN, GPIOD_ASIS... > > - Is it not just a hog? We have bindings for those. I hope the above shed some light on this. > >> + 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. >> + microchip,sgpio-frequency: >> + description: The sgpio controller frequency (Hz). This dictates >> + the serial bitstream speed, which again affects the latency in >> + getting control signals back and forth between external shift >> + registers. The speed must be no larger than half the system >> + clock, and larger than zero. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + minimum: 1 >> + default: 12500000 > > I understand why you need this binding now, OK. > >> +/* 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. > Yours, > Linus Walleij ---Lars -- Lars Povlsen, Microchip _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel