Re: [PATCH v2 3/3] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Joey Gouly <joey.gouly@xxxxxxx>]

Hi!

On Mon, Oct 04, 2021 at 12:33:15PM +0900, Hector Martin wrote:
> On 02/10/2021 04.12, Joey Gouly wrote:
> > +#define REG_GPIO(x)          (4 * (x))
> > +#define REG_GPIOx_DATA       BIT(0)
> > +#define REG_GPIOx_MODE_MASK  GENMASK(3, 1)
> > +#define REG_GPIOx_OUT        1
> > +#define REG_GPIOx_IN_IRQ_HI  2
> > +#define REG_GPIOx_IN_IRQ_LO  3
> > +#define REG_GPIOx_IN_IRQ_UP  4
> > +#define REG_GPIOx_IN_IRQ_DN  5
> > +#define REG_GPIOx_IN_IRQ_ANY 6
> > +#define REG_GPIOx_IN_IRQ_OFF 7
> > +#define REG_GPIOx_PERIPH     BIT(5)
> > +#define REG_GPIOx_CFG_DONE   BIT(9)
> > +#define REG_GPIOx_GRP_MASK   GENMASK(18, 16)
> > +#define REG_IRQ(g, x)        (0x800 + 0x40 * (g) + 4 * ((x) >> 5))
> 
> Can we update these defines with the correct definitions and names we
> figured out the other day and add the missing ones? We now know a bunch of
> these are wrong (e.g. CFG_DONE is INPUT_ENABLE, PERIPH should be two bits,
> we're missing pull-up control, drive strength, schmitt trigger and lock
> bits). Even if we don't implement all the features in the driver yet, we
> should have all the register bit defines for documentation purposes at
> least.

Done.

> 
> > +	if (!(prev & REG_GPIOx_CFG_DONE))
> > +		writel_relaxed(cfg & ~REG_GPIOx_CFG_DONE, ppin);
> > +	writel_relaxed(cfg, ppin);
> 
> We already determined this dance doesn't make any sense; if we want to
> change the pin config before enabling the input buffer (whether this serves
> any purpose at all is an open question) then that should be handled in the
> upper code responsible for enabling/disabling the input buffer, not in the
> core register wrappers.

Removed this.

[...]

> > +static void apple_gpio_gpio_set(struct gpio_chip *chip, unsigned int offset,
> > +				int value)
> > +{
> > +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
> > +
> > +	apple_gpio_set_reg(pctl, offset, REG_GPIOx_DATA,
> > +			   REG_GPIOx_CFG_DONE | (value & REG_GPIOx_DATA));
> > +}
> 
> `value ? REG_GPIOx_DATA : 0` please, otherwise this makes assumptions about
> value always being 1 and REG_GPIOx_DATA being the LSB.

I like that.

> > +static int apple_gpio_gpio_direction_input(struct gpio_chip *chip,
> > +					   unsigned int offset)
> > +{
> > +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
> > +
> > +	apple_gpio_set_reg(pctl, offset, REG_GPIOx_MODE_MASK | REG_GPIOx_DATA,
> > +			   FIELD_PREP(REG_GPIOx_MODE_MASK,
> > +				      REG_GPIOx_IN_IRQ_OFF) |
> > +				   REG_GPIOx_CFG_DONE);
> 
> Is hardcoding IRQ_OFF correct here? Shouldn't this be getting the intended
> IRQ state from somewhere, or is it always guaranteed that that gets set
> later?

As far as I can tell, the only way to setup an IRQ on a pin/gpio goes
via the apple_gpio_irq_startup function, which calls
`apple_gpio_gpio_direction_input` and then `apple_gpio_gpio_irq_unmask`
which sets the correct IRQ_ value.

> 
> > +static int apple_gpio_gpio_direction_output(struct gpio_chip *chip,
> > +					    unsigned int offset, int value)
> > +{
> > +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
> > +
> > +	apple_gpio_set_reg(pctl, offset, REG_GPIOx_PERIPH | REG_GPIOx_DATA,
> > +			   FIELD_PREP(REG_GPIOx_MODE_MASK, REG_GPIOx_OUT) |
> > +				   (value & REG_GPIOx_DATA) |
> > +				   REG_GPIOx_CFG_DONE);
> 
> I actually wonder if we should even bother turning on the input buffer for
> output pins, given we're shadowing the value anyway. Seems unnecessary and
> might save a few nanowatts.
> 
> Also, why is this clearing the peripheral (yet direction_input isn't)?

I've removed setting the input buffer here (CFG_DONE, or INPUT_ENABLE as
it is now named in v3).

That was just an oversight, I'm clearing it in direction_input too now.

[...]

> > +static unsigned int apple_gpio_gpio_irq_startup(struct irq_data *data)
> > +{
> > +	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
> > +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
> > +
> > +	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_GRP_MASK,
> > +			   FIELD_PREP(REG_GPIOx_GRP_MASK, 0));
> 
> I guess we're only using a single IRQ group right now?
Yep. The irq handler should already work with different groups. So to
support different groups we'd have to get the group number here somehow.

Thanks,
Joey