Re: [PATCH] gpio: Add Tegra186 support

[Laxman Dewangan]

On Thursday 24 November 2016 08:38 PM, Thierry Reding wrote:

* PGP Signed by an unknown key

On Thu, Nov 24, 2016 at 08:14:31PM +0530, Laxman Dewangan wrote:


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.


Nop, it will not work. The reason is:
include/dt-binding/gpio/tegra186-gpio.h


#define TEGRA_MAIN_GPIO(port, offset) \
 ((TEGRA_MAIN_GPIO_PORT_##port * 8) + offset)


so in your DTS file, if you use this macro for the gpio number then you will
have pin per port as 8.
And so your total GPIO is 23 *8 (Port CC) but in source code ngpio is very
less.

Yes, within the source code, ngpio will be the exact number of pins that
the GPIO controller physically exposes. But that still works fine, feel
free to test the driver if you don't believe me. The translation from
one numberspace to the other is done in tegra186_gpio_of_xlate().


OK, so you are mapping the DT gpio number to new number using xlate.

This method is fine but it complicate the driver and always it needs to 
calculate the base for the port.

If we have one to one mapping then probably, we can have fixed lookup table.

I am just incline to make stuff simple so that we can have better 
maintainability and debugging. This is very common driver and almost all 
BSPS engineer debug here so want to make this driver extremely simple.

Re: [PATCH] gpio: Add Tegra186 support

[Laxman Dewangan]

On Thursday 24 November 2016 08:38 PM, Thierry Reding wrote:

* PGP Signed by an unknown key

On Thu, Nov 24, 2016 at 08:14:31PM +0530, Laxman Dewangan wrote:


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.


Nop, it will not work. The reason is:
include/dt-binding/gpio/tegra186-gpio.h


#define TEGRA_MAIN_GPIO(port, offset) \
 ((TEGRA_MAIN_GPIO_PORT_##port * 8) + offset)


so in your DTS file, if you use this macro for the gpio number then you will
have pin per port as 8.
And so your total GPIO is 23 *8 (Port CC) but in source code ngpio is very
less.

Yes, within the source code, ngpio will be the exact number of pins that
the GPIO controller physically exposes. But that still works fine, feel
free to test the driver if you don't believe me. The translation from
one numberspace to the other is done in tegra186_gpio_of_xlate().


OK, so you are mapping the DT gpio number to new number using xlate.

This method is fine but it complicate the driver and always it needs to 
calculate the base for the port.

If we have one to one mapping then probably, we can have fixed lookup table.

I am just incline to make stuff simple so that we can have better 
maintainability and debugging. This is very common driver and almost all 
BSPS engineer debug here so want to make this driver extremely simple.

Re: [PATCH] gpio: Add Tegra186 support

[Linus Walleij]

On Wed, Nov 23, 2016 at 8:44 PM, Thierry Reding
 wrote:
> On Wed, Nov 23, 2016 at 02:30:45PM +0100, Linus Walleij wrote:
>> On Tue, Nov 22, 2016 at 6:55 PM, Thierry Reding
>>  wrote:
>>
>> > From: Thierry Reding 
>> >
>> > 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 
>>
>> I would prefer if you could try to convert this driver to use
>> CONFIG_GPIOLIB_IRQCHIP and install a chained interrupt
>> handler with gpiochip_irqchio_add() + gpiolib_set_chained_irqchip().
>> It would save us so much trouble and so much complicated
>> code to maintain for this custom irqdomain.
>>
>> I suggest you to look into the mechanisms mentioned in my
>> previous mail for how to poke holes in the single linear
>> irqdomain used by this mechanism.
>>
>> As it seems, you only have one parent interrupt with all
>> these IRQs cascading off it as far as I can tell.
>
> Like I said in other email, I don't think this will work because the
> GPIOLIB_IRQCHIP support seems to be limited to cases where a single
> parent interrupt is used. We could possibly live with a single IRQ
> handler and data, but we definitely need to request multiple IRQs if
> we want to be able to use all GPIOs as interrupts.

Sorry if I missed that.

Actually it works.

If you look at gpiochip_set_chained_irqchip() it just calls
irq_set_chained_handler_and_data() for the parent interrupt.

There is a loop afterwards that call irq_set_parent()
on all child IRQs but to the kernel this is just a noop I
never fully grasped that thing. Maybe it should even be deleted.
It just sets the parent in the irq descriptor, and the IRQ core only
ever use this on threaded interrupts, so maybe it should
really just be set on those (nested) interrupts.

Maybe Marc/tglx could shed some light on the intended
use of irq_set_parent() given that only two MFD drivers
and the gpiolib uses it. I think I need to dig into some
commitlogs.

If it is semantically required to call irq_set_parent() with the
right child/parent, we could add an
gpiochip_set_chained_irqchip_interval(first, last) to set the
parent for just an interval of the offsets.

Yours,
Linus Walleij

Re: [PATCH] gpio: Add Tegra186 support

[Linus Walleij]

On Wed, Nov 23, 2016 at 8:44 PM, Thierry Reding
 wrote:
> On Wed, Nov 23, 2016 at 02:30:45PM +0100, Linus Walleij wrote:
>> On Tue, Nov 22, 2016 at 6:55 PM, Thierry Reding
>>  wrote:
>>
>> > From: Thierry Reding 
>> >
>> > 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 
>>
>> I would prefer if you could try to convert this driver to use
>> CONFIG_GPIOLIB_IRQCHIP and install a chained interrupt
>> handler with gpiochip_irqchio_add() + gpiolib_set_chained_irqchip().
>> It would save us so much trouble and so much complicated
>> code to maintain for this custom irqdomain.
>>
>> I suggest you to look into the mechanisms mentioned in my
>> previous mail for how to poke holes in the single linear
>> irqdomain used by this mechanism.
>>
>> As it seems, you only have one parent interrupt with all
>> these IRQs cascading off it as far as I can tell.
>
> Like I said in other email, I don't think this will work because the
> GPIOLIB_IRQCHIP support seems to be limited to cases where a single
> parent interrupt is used. We could possibly live with a single IRQ
> handler and data, but we definitely need to request multiple IRQs if
> we want to be able to use all GPIOs as interrupts.

Sorry if I missed that.

Actually it works.

If you look at gpiochip_set_chained_irqchip() it just calls
irq_set_chained_handler_and_data() for the parent interrupt.

There is a loop afterwards that call irq_set_parent()
on all child IRQs but to the kernel this is just a noop I
never fully grasped that thing. Maybe it should even be deleted.
It just sets the parent in the irq descriptor, and the IRQ core only
ever use this on threaded interrupts, so maybe it should
really just be set on those (nested) interrupts.

Maybe Marc/tglx could shed some light on the intended
use of irq_set_parent() given that only two MFD drivers
and the gpiolib uses it. I think I need to dig into some
commitlogs.

If it is semantically required to call irq_set_parent() with the
right child/parent, we could add an
gpiochip_set_chained_irqchip_interval(first, last) to set the
parent for just an interval of the offsets.

Yours,
Linus Walleij

Re: [PATCH] gpio: Add Tegra186 support

[Thierry Reding]

On Thu, Nov 24, 2016 at 08:14:31PM +0530, Laxman Dewangan wrote:
> 
> On Thursday 24 November 2016 08:14 PM, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > 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.
> 
> I am saying that you dont need this new APIs, GPIO framework already support
> this via the call gpiochip_get_data(chip); which you provided when adding
> gpiochip().

Okay, it looks like this is the standard way to do this within the GPIO
subsystem. I can switch to that.

> > > > +   gpio->gpio.parent = >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.
> 
> 
> Nop, it will not work. The reason is:
> include/dt-binding/gpio/tegra186-gpio.h
> 
> 
> #define TEGRA_MAIN_GPIO(port, offset) \
> ((TEGRA_MAIN_GPIO_PORT_##port * 8) + offset)
> 
> 
> so in your DTS file, if you use this macro for the gpio number then you will
> have pin per port as 8.
> And so your total GPIO is 23 *8 (Port CC) but in source code ngpio is very
> less.

Yes, within the source code, ngpio will be the exact number of pins that
the GPIO controller physically exposes. But that still works fine, feel
free to test the driver if you don't believe me. The translation from
one numberspace to the other is done in tegra186_gpio_of_xlate().

Thierry


signature.asc
Description: PGP signature

Re: [PATCH] gpio: Add Tegra186 support

[Thierry Reding]

On Thu, Nov 24, 2016 at 08:14:31PM +0530, Laxman Dewangan wrote:
> 
> On Thursday 24 November 2016 08:14 PM, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > 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.
> 
> I am saying that you dont need this new APIs, GPIO framework already support
> this via the call gpiochip_get_data(chip); which you provided when adding
> gpiochip().

Okay, it looks like this is the standard way to do this within the GPIO
subsystem. I can switch to that.

> > > > +   gpio->gpio.parent = >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.
> 
> 
> Nop, it will not work. The reason is:
> include/dt-binding/gpio/tegra186-gpio.h
> 
> 
> #define TEGRA_MAIN_GPIO(port, offset) \
> ((TEGRA_MAIN_GPIO_PORT_##port * 8) + offset)
> 
> 
> so in your DTS file, if you use this macro for the gpio number then you will
> have pin per port as 8.
> And so your total GPIO is 23 *8 (Port CC) but in source code ngpio is very
> less.

Yes, within the source code, ngpio will be the exact number of pins that
the GPIO controller physically exposes. But that still works fine, feel
free to test the driver if you don't believe me. The translation from
one numberspace to the other is done in tegra186_gpio_of_xlate().

Thierry


signature.asc
Description: PGP signature

Re: [PATCH] gpio: Add Tegra186 support

[Laxman Dewangan]

On Thursday 24 November 2016 08:14 PM, Thierry Reding wrote:

* PGP Signed by an unknown key

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.


I am saying that you dont need this new APIs, GPIO framework already 
support this via the call gpiochip_get_data(chip); which you provided 
when adding gpiochip().







+   gpio->gpio.parent = >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.



Nop, it will not work. The reason is:
include/dt-binding/gpio/tegra186-gpio.h


#define TEGRA_MAIN_GPIO(port, offset) \
((TEGRA_MAIN_GPIO_PORT_##port * 8) + offset)


so in your DTS file, if you use this macro for the gpio number then you 
will have pin per port as 8.
And so your total GPIO is 23 *8 (Port CC) but in source code ngpio is 
very less.

Re: [PATCH] gpio: Add Tegra186 support

[Laxman Dewangan]

On Thursday 24 November 2016 08:14 PM, Thierry Reding wrote:

* PGP Signed by an unknown key

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.


I am saying that you dont need this new APIs, GPIO framework already 
support this via the call gpiochip_get_data(chip); which you provided 
when adding gpiochip().







+   gpio->gpio.parent = >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.



Nop, it will not work. The reason is:
include/dt-binding/gpio/tegra186-gpio.h


#define TEGRA_MAIN_GPIO(port, offset) \
((TEGRA_MAIN_GPIO_PORT_##port * 8) + offset)


so in your DTS file, if you use this macro for the gpio number then you 
will have pin per port as 8.
And so your total GPIO is 23 *8 (Port CC) but in source code ngpio is 
very less.

Re: [PATCH] gpio: Add Tegra186 support

[Thierry Reding]

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 = >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 = >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


signature.asc
Description: PGP signature

Re: [PATCH] gpio: Add Tegra186 support

[Thierry Reding]

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 = >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 = >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


signature.asc
Description: PGP signature

Re: [PATCH] gpio: Add Tegra186 support

[Laxman Dewangan]

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.



+
+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 = >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.



+
+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.


+
+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?



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




+   gpio->gpio.parent = >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.






+
+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 =

Re: [PATCH] gpio: Add Tegra186 support

[Laxman Dewangan]

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.



+
+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 = >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.



+
+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.


+
+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?



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




+   gpio->gpio.parent = >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.






+
+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 =

Re: [PATCH] gpio: Add Tegra186 support

[Thierry Reding]

On Wed, Nov 23, 2016 at 02:30:45PM +0100, Linus Walleij wrote:
> On Tue, Nov 22, 2016 at 6:55 PM, Thierry Reding
>  wrote:
> 
> > From: Thierry Reding 
> >
> > 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 
> 
> I would prefer if you could try to convert this driver to use
> CONFIG_GPIOLIB_IRQCHIP and install a chained interrupt
> handler with gpiochip_irqchio_add() + gpiolib_set_chained_irqchip().
> It would save us so much trouble and so much complicated
> code to maintain for this custom irqdomain.
> 
> I suggest you to look into the mechanisms mentioned in my
> previous mail for how to poke holes in the single linear
> irqdomain used by this mechanism.
> 
> As it seems, you only have one parent interrupt with all
> these IRQs cascading off it as far as I can tell.

Like I said in other email, I don't think this will work because the
GPIOLIB_IRQCHIP support seems to be limited to cases where a single
parent interrupt is used. We could possibly live with a single IRQ
handler and data, but we definitely need to request multiple IRQs if
we want to be able to use all GPIOs as interrupts.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH] gpio: Add Tegra186 support

[Thierry Reding]

On Wed, Nov 23, 2016 at 02:30:45PM +0100, Linus Walleij wrote:
> On Tue, Nov 22, 2016 at 6:55 PM, Thierry Reding
>  wrote:
> 
> > From: Thierry Reding 
> >
> > 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 
> 
> I would prefer if you could try to convert this driver to use
> CONFIG_GPIOLIB_IRQCHIP and install a chained interrupt
> handler with gpiochip_irqchio_add() + gpiolib_set_chained_irqchip().
> It would save us so much trouble and so much complicated
> code to maintain for this custom irqdomain.
> 
> I suggest you to look into the mechanisms mentioned in my
> previous mail for how to poke holes in the single linear
> irqdomain used by this mechanism.
> 
> As it seems, you only have one parent interrupt with all
> these IRQs cascading off it as far as I can tell.

Like I said in other email, I don't think this will work because the
GPIOLIB_IRQCHIP support seems to be limited to cases where a single
parent interrupt is used. We could possibly live with a single IRQ
handler and data, but we definitely need to request multiple IRQs if
we want to be able to use all GPIOs as interrupts.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH] gpio: Add Tegra186 support

[Linus Walleij]

On Tue, Nov 22, 2016 at 6:55 PM, Thierry Reding
 wrote:

> From: Thierry Reding 
>
> 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 

I would prefer if you could try to convert this driver to use
CONFIG_GPIOLIB_IRQCHIP and install a chained interrupt
handler with gpiochip_irqchio_add() + gpiolib_set_chained_irqchip().
It would save us so much trouble and so much complicated
code to maintain for this custom irqdomain.

I suggest you to look into the mechanisms mentioned in my
previous mail for how to poke holes in the single linear
irqdomain used by this mechanism.

As it seems, you only have one parent interrupt with all
these IRQs cascading off it as far as I can tell.

Yours,
Linus Walleij

Re: [PATCH] gpio: Add Tegra186 support

[Linus Walleij]

On Tue, Nov 22, 2016 at 6:55 PM, Thierry Reding
 wrote:

> From: Thierry Reding 
>
> 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 

I would prefer if you could try to convert this driver to use
CONFIG_GPIOLIB_IRQCHIP and install a chained interrupt
handler with gpiochip_irqchio_add() + gpiolib_set_chained_irqchip().
It would save us so much trouble and so much complicated
code to maintain for this custom irqdomain.

I suggest you to look into the mechanisms mentioned in my
previous mail for how to poke holes in the single linear
irqdomain used by this mechanism.

As it seems, you only have one parent interrupt with all
these IRQs cascading off it as far as I can tell.

Yours,
Linus Walleij