Re: [PATCHv3 2/11] GPIO: GPIO Driver for DA9052 PMIC

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Tue, Jul 20, 2010 at 11:14:42AM +0100, dd diasemi wrote:

> +#if (DA9052_GPIO_PIN_0 == DA9052_GPIO_CONFIG)
> +	msg.addr = DA9052_GPIO0001_REG;
> +	msg.data = 0;
> +

As has been said many times now with most of your drivers things like
this should be pased in as platform data rather than supplied via
Kconfig.  It should be possible to build a kernel that works on many
systems so compile time in-driver configuration is undesirable.  There's
also an awful lot of redundant code with just small data differences in
these blocks from the looks of it.

> +	if (da9052->read(da9052, &msg)) {
> +		da9052_unlock(da9052);
> +		return -EIO;

Better to feed back the original error code from the read function
surely?

> +s32 da9052_gpio_read_port(struct da9052_gpio_read_write *read_port,
> +				struct da9052 *da9052)
> +{

This looks like it should be static, or possibly part of the core driver?

> +s32 da9052_gpio_multiple_read(struct da9052_gpio_multiple_read *multiple_port,
> +				struct da9052 *da9052)
> +{

This too.

> +config DA9052_GPIO_ENABLE
> +	bool "Dialog Semiconductor DA9052 GPIO Driver"
> +	depends on PMIC_DA9052
> +	help
> +	  Say Y  to enable the GPIO driver for the DA9052 chip
> +

No need for the _ENABLE in the name (look at existing drivers and try to
make your functionality consistent).

> +/* Currently used defines for GPIO PINs  */
> +/*--------------------------------------------------------------------------*/
> +#define DA9052_GPIO_PIN_0	DA9052_GPIO_CONFIG_ADC
> +#define DA9052_GPIO_PIN_1	DA9052_GPIO_CONFIG_ADC
> +#define DA9052_GPIO_PIN_2	DA9052_GPIO_CONFIG_ADC
> +
> +#define DA9052_GPIO_PIN_3	DA9052_GPIO_CONFIG_TSI
> +#define DA9052_GPIO_PIN_4	DA9052_GPIO_CONFIG_TSI
> +#define DA9052_GPIO_PIN_5	DA9052_GPIO_CONFIG_TSI
> +#define DA9052_GPIO_PIN_6	DA9052_GPIO_CONFIG_TSI
> +#define DA9052_GPIO_PIN_7	DA9052_GPIO_CONFIG_TSI
> +
> +#define DA9052_GPIO_PIN_8	DA9052_GPIO_CONFIG
> +#define DA9052_GPIO_PIN_9	DA9052_GPIO_CONFIG
> +#define DA9052_GPIO_PIN_10	DA9052_GPIO_CONFIG
> +#define DA9052_GPIO_PIN_11	DA9052_GPIO_CONFIG
> +
> +#define DA9052_GPIO_PIN_12	DA9052_GPIO_CONFIG
> +#define DA9052_GPIO_PIN_13	DA9052_GPIO_CONFIG
> +
> +#define DA9052_GPIO_PIN_14	DA9052_GPIO_CONFIG_I2C
> +#define DA9052_GPIO_PIN_15	DA9052_GPIO_CONFIG_I2C

These all look like platform data...

> +enum ip_op_type {
> +	ALTERNATE_FUNCTIONALITY = 0,
> +	INPUT,
> +	OUTPUT_OPENDRAIN,
> +	OUTPUT_PUSHPULL
> +};

These and most of the other defines in the remainder of the file need to
be namespaced.