[U-Boot] [PATCH 03/17] sunxi: gpio: Add support for gpio pins on the AXP209 pmic

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 28-12-14 10:34, Ian Campbell wrote:
> On Wed, 2014-12-24 at 20:06 +0100, Hans de Goede wrote:
>> @@ -27,6 +32,17 @@ enum axp209_reg {
>>
>>   #define AXP209_POWEROFF			(1 << 7)
>>
>> +#define AXP209_GPIO_OUTPUT_LOW		0x00
>> +#define AXP209_GPIO_OUTPUT_HIGH		0x01
>> +#define AXP209_GPIO_INPUT		0x02
>> +
>> +#define AXP209_GPIO_OUTPUT_LOW		0x00
>> +#define AXP209_GPIO_OUTPUT_HIGH		0x01
>
> Aren't these LOW+HIGH ones duplicated?

Oops, copy + paste fail.

>
> Also, they lack the helpful comments which you added to the GPIO3 ones.

That is because they are self explanatory, unlike gpio3 which is
interesting with decoupled control bits, gpio0-2 simply have
3 bits which code 0 - 7, with 3+ being not interesting to us, so
we only use 0-2 which are low / high / input. Still I'll add some
comments for you :)

> I'd be included to add a /* GPIO3 is different from the others */ just
> here too (also: "sigh" re h/w inconsistency...).

Will do.

>> +#define AXP209_GPIO3_OUTPUT_LOW		0x00 /* Drive pin low, Output mode */
>> +#define AXP209_GPIO3_OUTPUT_HIGH	0x02 /* Float pin, Output mode */
>
> Is a floating output really a thing or is this a cut-and-paste-o?

Rather then a push-pull driver, gpio3 seems to have an
open collector / open drain driver, in combination with the
output enable transistor which one would expect on a tristate
driver. So it seems there are 2 ways to float the pin, one by
not driving the output-enable transistor, but also by driving
the output-enable transister, but not pulling the output low,
at least the datasheet describes 2 separate bits one to select
output/input the other to select drive-low/float.

I've chosen to map drvie output, but do not pull output low as
AXP209_GPIO3_OUTPUT_HIGH, and that is what the comment tries
to convey. Note this is all my interpretation of the not so
helpful datasheet.

>> +#define AXP209_GPIO3_INPUT		0x06 /* Float pin, Input mode */
>> +
>> +int axp_gpio_direction_output(unsigned int pin, unsigned int val)
>> +{
>> +	u8 reg = axp209_get_gpio_ctrl_reg(pin);
>> +
>> +	if (val) {
>> +		val = (pin == 3) ? AXP209_GPIO3_OUTPUT_HIGH :
>> +				   AXP209_GPIO_OUTPUT_HIGH;
>> +	} else {
>> +		val = (pin == 3) ? AXP209_GPIO3_OUTPUT_LOW :
>> +				   AXP209_GPIO_OUTPUT_LOW;
>
> Both OUTPUT_LOW values happen to be the same, although I could see why
> you might want to do it this way for consistency.

I also notices the happen to be the same, but I indeed did things
this way for consistency.

Regards,

Hans