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

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 <redacted>
---

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