Re: [PATCH v2] gpio: Add Tegra186 support

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3573 bytes --]

On Mon, Mar 13, 2017 at 09:52:04PM +0530, Laxman Dewangan wrote:
> 
> On Friday 10 March 2017 09:56 PM, Thierry Reding wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > Tegra186 has two GPIO controllers that are largely register compatible
> > between one another but are completely different from the controller
> > found on earlier generations.
> > 
> > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> > Changes in v2:
> > - add pin names to allow easy lookup using the chardev interface
> > - distinguish AON and main GPIO controllers by label
> > - use gpiochip_get_data() instead of container_of()
> > - use C99 initializers
> > 
> 
> Overall it looks fine to me.
> Only thing I did not like is the of_xlate manipulation where the gpio number
> is translated to another number for register access.
> The reason is that it complicate in debugging when we instrument the
> callbacks for some gpios and this is very common in our debugging.
> 
> I still request here to use simple mapping instead of complicating.

I fail to see how this is complicating things. Yes, you can no longer
directly map from an offset to the port/index, but since we have a name
associated with each pin, we can simply print the name if we want to
know what pin we're dealing with.

So you could simply do something like this:

	static int tegra186_gpio_get_direction(struct gpio_chip *chip,
					       unsigned int offset)
	{
		struct gpio_desc *desc = gpiochip_get_desc(chip, offset);
		...
		dev_dbg(chip->parent, "GPIO %s (%s)\n", desc->name, desc->label);
		...
	}

The alternative that you're talking about would be something like this:

	static int tegra186_gpio_get_direction(struct gpio_chip *chip,
					       unsigned int offset)
	{
		struct tegra_gpio_port_soc_info *info;
		unsigned int port = GPIO_PORT(offset);
		unsigned int pin = GPIO_PIN(offset);
		...
		info = tgi->soc->port[port];
		...
		dev_dbg(chip->parent, "GPIO P%s.%02u\n", info->port_name, pin);
		...
	}

And with the above you'd either duplicate the logic that constructs the
pin name: once in the instrumentation code and once in the code that
sets up the pin names. Or you could decide not to set up any pin names
at all, in which case userspace would need to deal with the numbering
schemes itself, which makes things really complicated, especially when
we start supporting multiple SoC families.

The proposal in this patch is really quite elegant, if I may say so, in
that individual pins are identified by their name. Userspace can query
the list and get only the ones that actually exist. The wholes in the
GPIO range are not there, so there's not even a chance of userspace
messing with non-existing GPIOs.

Similarly the kernel has access to the name of these GPIOs, so the same
"string representation" can be used as for userspace.

I think this makes the driver very consistent across all use-cases. The
only remaining downside I see is that we're moving away from the 8-pins
per port from earlier chips. But I actually see that as a feature, even
something that we should backport to older chips.

The only reason why the driver for older chips works is because the
registers are laid out such that the pins of a port share one register
block, so if a non-existing GPIO is accessed, we'd be accessing a non-
existing field in an existing register, which seems to be less of an
issue than accessing a non-existing register.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]