On 11/20/2012 10:42 AM, Florian Fainelli wrote:
> Hi Hauke,
>
> This driver looks good to me, a couple of minor comments below.
>
> On Monday 19 November 2012 23:57:53 Hauke Mehrtens wrote:
>> Register a GPIO driver to access the GPIOs provided by the chip.
>> The GPIOs of the SoC should always start at 0 and the other GPIOs could
>> start at a random position. There is just one SoC in a system and when
>> they start at 0 the number is predictable.
>>
>> Signed-off-by: Hauke Mehrtens <hauke@xxxxxxxxxx>
>> ---
> [snip]
>> +#ifdef CONFIG_BCMA_DRIVER_GPIO
>> +/* driver_gpio.c */
>> +int bcma_gpio_init(struct bcma_drv_cc *cc);
>> +#else
>> +static inline int bcma_gpio_init(struct bcma_drv_cc *cc)
>> +{
>> + return 0;
>> +}
>> +#endif /* CONFIG_BCMA_DRIVER_GPIO */
>
> I wonder if it would not make more sense here to return -ENODEV or -ENOTSUPP
> so we can identify a kernel not being built with BCMA GPIO support.
I added that and changed the logging for such a message to debug level,
but I do not know if this would confuse people just using bcma/ssb for
their wireless pcie cards.
>> +
>> #endif
>> diff --git a/drivers/bcma/driver_gpio.c b/drivers/bcma/driver_gpio.c
>> new file mode 100644
>> index 0000000..2b9e404
>> --- /dev/null
>> +++ b/drivers/bcma/driver_gpio.c
>> @@ -0,0 +1,95 @@
>> +/*
>> + * Broadcom specific AMBA
>> + * GPIO driver
>> + *
>> + * Copyright 2011, Broadcom Corporation
>> + * Copyright 2012, Hauke Mehrtens <hauke@xxxxxxxxxx>
>> + *
>> + * Licensed under the GNU/GPL. See COPYING for details.
>> + */
>> +
>> +#include <linux/gpio.h>
>> +#include <linux/export.h>
>> +#include <linux/bcma/bcma.h>
>> +
>> +#include "bcma_private.h"
>> +
>> +static inline struct bcma_drv_cc *bcma_gpio_get_cc(struct gpio_chip *chip)
>> +{
>> + return container_of(chip, struct bcma_drv_cc, gpio);
>> +}
>> +
>> +static int bcma_gpio_get_value(struct gpio_chip *chip, unsigned gpio)
>> +{
>> + struct bcma_drv_cc *cc = bcma_gpio_get_cc(chip);
>> +
>> + return !!bcma_chipco_gpio_in(cc, 1 << gpio);
>> +}
>> +
>> +static void bcma_gpio_set_value(struct gpio_chip *chip, unsigned gpio,
>> + int value)
>> +{
>> + struct bcma_drv_cc *cc = bcma_gpio_get_cc(chip);
>> +
>> + bcma_chipco_gpio_out(cc, 1 << gpio, value ? 1 << gpio : 0);
>
> This is a little confusing at first, because most GPIO "drivers" actually just
> pass the value directly.
The bcma_chipco_gpio API exposes the raw registers, and the conversion
of the generic GPIO API to these registers is done here. Should I change
something here?
> [snip]
>
>> +int bcma_gpio_init(struct bcma_drv_cc *cc)
>> +{
>> + struct gpio_chip *chip = &cc->gpio;
>> +
>> + chip->label = "bcma_gpio";
>> + chip->owner = THIS_MODULE;
>> + chip->request = bcma_gpio_request;
>> + chip->free = bcma_gpio_free;
>> + chip->get = bcma_gpio_get_value;
>> + chip->set = bcma_gpio_set_value;
>> + chip->direction_input = bcma_gpio_direction_input;
>> + chip->direction_output = bcma_gpio_direction_output;
>> + chip->ngpio = 16;
>> + if (cc->core->bus->hosttype == BCMA_HOSTTYPE_SOC)
>> + chip->base = 0;
>> + else
>> + chip->base = -1;
>
> You might want to add a comment to explain why base auto-assignment is not used
> when the host type is SOC.
I added a comment.
> --
> Florian
>
