Re: [PATCH net-next RFC 3/9] net: dsa: mv88e6xxx: add support for GPIO configuration

[Andrew Lunn]

On Thu, Sep 28, 2017 at 10:45:03AM -0700, Florian Fainelli wrote:
> 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 <brandon.streiff@xxxxxx>
> > ---
> 
> > +/* 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?

Hi Florian

The general pattern in this code is that the lock chip->reg_lock is
taken at a higher level. That protects against other threads. The
driver tends to do that at the highest levels, at the entry points
into the driver. I've not yet checked this code follows the pattern
yet. However, we have a check in the low level to ensure the lock has
been taken. So it seems likely the lock is held.
 
> 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?

That would be my preference as well, or maybe a pinctrl driver.

     Andrew