Re: [PATCH v2 2/2] gpio: aspeed: Add banks Y, Z, AA, AB and AC

[Andrew Jeffery]

On Thu, 2017-01-26 at 14:51 +0100, Linus Walleij wrote:
> > On Tue, Jan 24, 2017 at 7:16 AM, Andrew Jeffery <andrew@xxxxxxxx> wrote:
> 
> > This is less straight-forward than one would hope, as some banks only
> > have 4 pins rather than 8, others are output only, yet more (W and
> > X, already supported) are input-only, and in the case of the g4 SoC bank
> > AC doesn't exist.
> > 
> > Add some structs to describe the varying properties of different banks
> > and integrate mechanisms to deny requests for unsupported
> > configurations.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@xxxxxxxx>
> 
> (...)
> > +#include <linux/gpio.h>
> 
> NAK don't do that. (I will explain below...)
> 
> (...)
> 
> > @@ -189,6 +258,12 @@ static int aspeed_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> > ÂÂÂÂÂÂÂÂunsigned long flags;
> > ÂÂÂÂÂÂÂÂu32 val;
> > 
> > +ÂÂÂÂÂÂÂif (!have_input(gpio, offset))
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn GPIOF_DIR_OUT;
> > +
> > +ÂÂÂÂÂÂÂif (!have_output(gpio, offset))
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn GPIOF_DIR_IN;
> 
> Please refrain from using GPIOF_* in drivers like this.
> 
> These flags are part of the consumer API, not the driver API.
> 
> Make sure the only header you include in your driver is
> <linux/gpio/driver.h> and just return 0/1 instead of these
> constants.

No worries, I'll send a follow-up fixing that. I was hesitant, but then
figured there was a reasonable chance I'd get the values around the
wrong way and went looking for macros that made it obviously correct.

> 
> > +static void set_irq_valid_mask(struct aspeed_gpio *gpio)
> > +{
> > +ÂÂÂÂÂÂÂconst struct aspeed_bank_props *props = gpio->config->props;
> > +
> > +ÂÂÂÂÂÂÂwhile (!is_bank_props_sentinel(props)) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂunsigned int offset;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst unsigned long int input = props->input;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/* Pretty crummy approach, but similar to GPIO core */
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂfor_each_clear_bit(offset, &input, 32) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂunsigned int i = props->bank * 32 + offset;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (i >= gpio->config->nr_gpios)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂclear_bit(i, gpio->chip.irq_valid_mask);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂprops++;
> > +ÂÂÂÂÂÂÂ}
> > +}
> 
> Nice that you use this feature!
> 
> > +/*
> > + * Any banks not specified in a struct aspeed_bank_props array are assumed to
> > + * have the properties:
> > + *
> > + *ÂÂÂÂÂ{ .input = 0xffffffff, .output = 0xffffffff }
> > + */
> > +
> > +static const struct aspeed_bank_props ast2400_bank_props[] = {
> > +ÂÂÂÂÂÂÂ/*ÂÂÂÂÂinputÂÂÂÂÂÂoutputÂÂÂ*/
> > +ÂÂÂÂÂÂÂ{ 5, 0xffffffff, 0x0000ffff }, /* U/V/W/X */
> > +ÂÂÂÂÂÂÂ{ 6, 0x0000000f, 0x0fffff0f }, /* Y/Z/AA/AB, two 4-GPIO holes */
> > +ÂÂÂÂÂÂÂ{ },
> > +};
> > +
> > +static const struct aspeed_gpio_config ast2400_config =
> > +ÂÂÂÂÂÂÂ/* 220 for simplicity, really 216 with two 4-GPIO holes, four at end */
> > +ÂÂÂÂÂÂÂ{ .nr_gpios = 220, .props = ast2400_bank_props, };
> > +
> > +static const struct aspeed_bank_props ast2500_bank_props[] = {
> > +ÂÂÂÂÂÂÂ/*ÂÂÂÂÂinputÂÂÂÂÂÂoutputÂÂÂ*/
> > +ÂÂÂÂÂÂÂ{ 5, 0xffffffff, 0x0000ffff }, /* U/V/W/X */
> > +ÂÂÂÂÂÂÂ{ 6, 0x0fffffff, 0x0fffffff }, /* Y/Z/AA/AB, 4-GPIO hole */
> > +ÂÂÂÂÂÂÂ{ 7, 0x000000ff, 0x000000ff }, /* AC */
> > +ÂÂÂÂÂÂÂ{ },
> > +};
> 
> A bit of magic values but there are comments to explain it so OK.
> 
> You could consider using the GENMASK() macro from <linux/bitops.h>
> to indicate the bits here but no big deal.

I'm going to leave it as is if you're not bothered, unless there are
strong objections from others.

Thanks,

Andrew

Attachment: signature.asc
Description: This is a digitally signed message part