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,
AndrewAttachment:
signature.asc
Description: This is a digitally signed message part
