Re: [PATCH 4/8] bcma: add GPIO driver

[Hauke Mehrtens <hauke@hauke-m.de>]

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@hauke-m.de>
>> ---
> [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@hauke-m.de>
>> + *
>> + * 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
>