Re: [PATCH] gpio: Add Tegra186 support

[Thierry Reding <thierry.reding@gmail.com>]

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

On Thu, Nov 24, 2016 at 12:23:56PM +0530, Laxman Dewangan wrote:
> 
> On Tuesday 22 November 2016 11:25 PM, Thierry Reding wrote:
> > +static inline struct tegra_gpio *to_tegra_gpio(struct gpio_chip *chip)
> > +{
> > +	return container_of(chip, struct tegra_gpio, gpio);
> > +}
> 
> You dont need this as gpiochip_get_data(chip); can provide the required
> driver specific data.

It's common practice to embed the struct gpio_chip within a driver-
specific structure, and it's equally common to use a container_of() to
get at the embedding structure.

> > +static const struct tegra_gpio_port *
> > +tegra186_gpio_get_port(struct tegra_gpio *gpio, unsigned int *pin)
> > +{
> > +	unsigned int start = 0, i;
> > +
> > +	for (i = 0; i < gpio->soc->num_ports; i++) {
> > +		const struct tegra_gpio_port *port = &gpio->soc->ports[i];
> > +
> > +		if (*pin >= start && *pin < start + port->pins) {
> > +			*pin -= start;
> > +			return port;
> > +		}
> > +
> > +		start += port->pins;
> > +	}
> > +
> Why not get the port from pins and then calculate with fixed offset.
> We will not need the loop if we know the port number.

Because we don't know what the port is until we've determined that the
pin is within the range of the port. This function determines what port
a given pin is in, then returns the relative index of the pin within
that port.

> > +static int tegra186_gpio_direction_output(struct gpio_chip *chip,
> > +					  unsigned int offset, int level)
> > +{
> > +	struct tegra_gpio *gpio = to_tegra_gpio(chip);
> > +	void __iomem *base;
> > +	u32 value;
> > +
> > +	/* configure output level first */
> > +	chip->set(chip, offset, level);
> We can directly call the apis instead of function pointer at this point.

That would mean reshuffling the functions or having an unneeded forward
declaration of tegra186_gpio_set().

> > +static struct lock_class_key tegra186_gpio_lock_class;
> We will have two instance of the driver (normal and AON) and so this will be
> shared between them.
> Do we really support multiple instance with same variable?

As the type and name indicate this is to track a specific class of lock.
It's fine to use a single variable with multiple instances. It is in
fact an error to try and make these per-instance.

> > +	gpio->gpio.label = "tegra186-gpio";
> Two instance will have same name. Better to say tegra186-gpio and
> tegra186-gpio-aon.

Good point. I suppose we could either add this to struct tegra_gpio_soc
or simply use the device tree node name.

> > +	gpio->gpio.parent = &pdev->dev;
> > +
> > +	gpio->gpio.get_direction = tegra186_gpio_get_direction;
> > +	gpio->gpio.direction_input = tegra186_gpio_direction_input;
> > +	gpio->gpio.direction_output = tegra186_gpio_direction_output;
> > +	gpio->gpio.get = tegra186_gpio_get,
> > +	gpio->gpio.set = tegra186_gpio_set;
> > +	gpio->gpio.to_irq = tegra186_gpio_to_irq;
> > +
> > +	gpio->gpio.base = -1;
> > +
> > +	for (i = 0; i < gpio->soc->num_ports; i++)
> > +		gpio->gpio.ngpio += gpio->soc->ports[i].pins;
> > +
> 
> Our DT binding does not say this. We assume that we have 8 gpios per port.
> so this will not work at all.

This has nothing to do with the device tree binding. What the device
tree binding defines is the indices to use to obtain a given GPIO within
a given port. What numbering the driver uses internally is completely up
to the driver implementation.

Oh, and the above works just fine.

> > +static const struct tegra_gpio_port tegra186_main_ports[] = {
> > +	[TEGRA_MAIN_GPIO_PORT_A]  = { 0x2000, 7 },
> Use C99 style initialization which is like
>    .offset =
>    .pins =

I suppose I could do that.

Thanks,
Thierry

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