Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752202AbdI1RpN (ORCPT ); Thu, 28 Sep 2017 13:45:13 -0400 Received: from mail-qt0-f195.google.com ([209.85.216.195]:57307 "EHLO mail-qt0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751339AbdI1RpL (ORCPT ); Thu, 28 Sep 2017 13:45:11 -0400 X-Google-Smtp-Source: AOwi7QDmoc1cDqm0tnLbGU1939kfidRDNM4Tn/hy07MpCcq8L2irmmwo74Vl4QNMv0+7bfEmv7XfCw== Subject: Re: [PATCH net-next RFC 3/9] net: dsa: mv88e6xxx: add support for GPIO configuration To: Brandon Streiff , netdev@vger.kernel.org Cc: linux-kernel@vger.kernel.org, "David S. Miller" , Andrew Lunn , Vivien Didelot , Richard Cochran , Erik Hons References: <1506612341-18061-1-git-send-email-brandon.streiff@ni.com> <1506612341-18061-4-git-send-email-brandon.streiff@ni.com> From: Florian Fainelli Message-ID: <659c4254-d0b7-52dc-dd9b-3921cd2f20c0@gmail.com> Date: Thu, 28 Sep 2017 10:45:03 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <1506612341-18061-4-git-send-email-brandon.streiff@ni.com> 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 Content-Length: 2844 Lines: 89 On 09/28/2017 08:25 AM, Brandon Streiff wrote: > The Scratch/Misc register is a windowed interface that provides access > to the GPIO configuration. Provide a new method for configuration of > GPIO functions. > > Signed-off-by: Brandon Streiff > --- > +/* Offset 0x1A: Scratch and Misc. Register */ > +static int mv88e6xxx_g2_scratch_reg_read(struct mv88e6xxx_chip *chip, > + int reg, u8 *data) > +{ > + int err; > + u16 value; > + > + err = mv88e6xxx_g2_write(chip, MV88E6XXX_G2_SCRATCH_MISC_MISC, > + reg << 8); > + if (err) > + return err; > + > + err = mv88e6xxx_g2_read(chip, MV88E6XXX_G2_SCRATCH_MISC_MISC, &value); > + if (err) > + return err; > + > + *data = (value & MV88E6XXX_G2_SCRATCH_MISC_DATA_MASK); > + > + return 0; > +} With the write and read acquiring and then releasing the lock immediately, is no there room for this sequence to be interrupted in the middle and end-up returning inconsistent reads? > + > +static int mv88e6xxx_g2_scratch_reg_write(struct mv88e6xxx_chip *chip, > + int reg, u8 data) > +{ > + u16 value = (reg << 8) | data; > + > + return mv88e6xxx_g2_update(chip, MV88E6XXX_G2_SCRATCH_MISC_MISC, value); > +} > + > +/* Configures the specified pin for the specified function. This function > + * does not unset other pins configured for the same function. If multiple > + * pins are configured for the same function, the lower-index pin gets > + * that function and the higher-index pin goes back to being GPIO. > + */ > +int mv88e6xxx_g2_set_gpio_config(struct mv88e6xxx_chip *chip, int pin, > + int func, int dir) > +{ > + int mode_reg = MV88E6XXX_G2_SCRATCH_GPIO_MODE(pin); > + int dir_reg = MV88E6XXX_G2_SCRATCH_GPIO_DIR(pin); > + int err; > + u8 val; > + > + if (pin < 0 || pin >= mv88e6xxx_num_gpio(chip)) > + return -ERANGE; > + > + /* Set function first */ > + err = mv88e6xxx_g2_scratch_reg_read(chip, mode_reg, &val); > + if (err) > + return err; > + > + /* Zero bits in the field for this GPIO and OR in new config */ > + val &= ~MV88E6XXX_G2_SCRATCH_GPIO_MODE_MASK(pin); > + val |= (func << MV88E6XXX_G2_SCRATCH_GPIO_MODE_OFFSET(pin)); > + > + err = mv88e6xxx_g2_scratch_reg_write(chip, mode_reg, val); > + if (err) > + return err; > + > + /* Set direction */ > + err = mv88e6xxx_g2_scratch_reg_read(chip, dir_reg, &val); > + if (err) > + return err; > + > + /* Zero bits in the field for this GPIO and OR in new config */ > + val &= ~MV88E6XXX_G2_SCRATCH_GPIO_DIR_MASK(pin); > + val |= (dir << MV88E6XXX_G2_SCRATCH_GPIO_DIR_OFFSET(pin)); > + > + return mv88e6xxx_g2_scratch_reg_write(chip, dir_reg, val); > +} Would there be any value in implementing a proper gpiochip structure here such that other pieces of SW can see this GPIO controller as a provider and you can reference it from e.g: Device Tree using GPIO descriptors? -- Florian