lkml.org 
[lkml]   [2017]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 2/2] gpio: aspeed: Add banks Y, Z, AA, AB and AC
On Tue, Jan 24, 2017 at 7:16 AM, Andrew Jeffery <andrew@aj.id.au> 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@aj.id.au>

(...)
> +#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.

> +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.

Yours,
Linus Walleij

\
 
 \ /
  Last update: 2017-01-26 14:52    [W:0.071 / U:0.976 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site