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

[Florian Fainelli <f.fainelli@gmail.com>]

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@ni.com>
> ---

> +/* 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