From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753500AbdEULn2 (ORCPT ); Sun, 21 May 2017 07:43:28 -0400 Received: from thoth.sbs.de ([192.35.17.2]:53605 "EHLO thoth.sbs.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752665AbdEULn0 (ORCPT ); Sun, 21 May 2017 07:43:26 -0400 Subject: Re: [PATCH v2 5/6] gpio-exar/8250-exar: Make set of exported GPIOs configurable To: Andy Shevchenko Cc: Greg Kroah-Hartman , Linus Walleij , Alexandre Courbot , Linux Kernel Mailing List , "linux-serial@vger.kernel.org" , "linux-gpio@vger.kernel.org" , Sudip Mukherjee , Sascha Weisenberger References: <357209721396d57cb871ae69d72308b1a7c9cbd7.1495119548.git.jan.kiszka@siemens.com> From: Jan Kiszka Message-ID: <83279eaf-69aa-333c-58b8-202d221375d3@siemens.com> Date: Sun, 21 May 2017 13:43:13 +0200 User-Agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2017-05-18 19:43, Andy Shevchenko wrote: > On Thu, May 18, 2017 at 5:59 PM, Jan Kiszka wrote: >> On the SIMATIC, IOT2040 only a single pin is exportable as GPIO, the >> rest is required to operate the UART. To allow modeling this case, >> expand the platform device data structure to specify a (consecutive) pin >> subset for exporting by the gpio-exar driver. > >> + unsigned int first_gpio; > > Perhaps pin? > Or shift? > > Because first_gpio a bit confusing with Linux side of GPIO. Ack, going for "pin". > >> - unsigned int bank = offset / 8; >> - unsigned int addr; >> + struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); >> + unsigned int bank, addr; >> >> + offset += exar_gpio->first_gpio; >> + bank = offset / 8; > > Can't we instead do something like the following: > > struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); > unsigned int bank = (offset + exar_gpio->pin) / 8; > unsigned int line = (offset + exar_gpio->pin) % 8; > OK, I'm using this pattern now: unsigned int addr = (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO; unsigned int bit = (offset + exar_gpio->first_pin) % 8; > >> + pdata.first_gpio = first_gpio; >> + pdata.ngpio = ngpio; > > Still thinking about device properties ("ngpios" and something like > "exar8250,gpio-start"). Changed back to properties, removing all platform data. But what's the purpose of prefixing the name here? This does not have anything to do with device trees. It's a private parameter channel between the creating device driver and the gpio driver, and there will be no other bindings. > >> + unsigned int first_gpio; >> + unsigned int ngpio; > > u16 ? > If we do that, then we would rather have to choose u8. But this is pointless restriction. I prefer to stay with the native type. Jan -- Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence Center Embedded Linux