[PATCH v2 0/3] pinctrl: Qualcomm 8x74 pinctrl driver

[PATCH v2 0/3] pinctrl: Qualcomm 8x74 pinctrl driver

[Linus Walleij]

On Fri, Dec 6, 2013 at 3:10 AM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:

> This series adds a pinctrl, pinmux, pinconf and gpiolib driver for the
> Qualcomm TLMM block found in recent Qualcomm SoCs (8x60 and newer).
> It designed with both v2 and v3 of the block in mind and comes with initial
> definitions for the 8x74 SoCs.
> This should be filled in with more data and definitions for more SoCs.
>
> Regards,
> Bjorn
>
> v2
> --
> * Replaced subnode dt parsing with generic pinctrl function
> * Installing irq_handler as chained irq handler
> * Added call to handle_bad_irq() when no irqs are pending in irq handler
> * Creating irq mapping at probe() and cleaned up the code accordingly
> * Replaced pinctrl_add_gpio_range with gpiochip_add_pin_range
> * Other minor updates based on review comments

I can't see any problem with these patches, and they only use standard
bindings, so I've applied all three so we get some linux-next rotation
and build coverage. Any remaining problems can certainly be fixed
with incremental patches.

Thanks!
Linus Walleij

[PATCH v2 2/3] pinctrl: Add msm8x74 configuration

[Stephen Boyd]

On 12/05/13 18:10, Bjorn Andersson wrote:
> Add initial definition of parameters for pinctrl-msm for the msm8x74
> platform.

Hmm. We've tried to remove 'x' from our code because it isn't really
accurate and leads to more confusion. For example, 8660 and 8960 are
vastly different with respect to SoC architecture and it was a mistake
to name it 8x60 in the code. We should have stuck with 8660 and 8260 to
avoid any confusion with 8960 but we didn't know that 8960 was going to
be made at the time. While it seems convenient to just throw in the x,
who knows what marketing will do later on and it could break the whole
scheme.

>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
>
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index d0b6846..31c6c4a 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -208,6 +208,10 @@ config PINCTRL_MSM
>  	select PINCONF
>  	select GENERIC_PINCONF
>  
> +config PINCTRL_MSM8X74
> +	bool "Qualcomm 8x74 pin controller driver"
> +	select PINCTRL_MSM

No help?

> +
>  config PINCTRL_NOMADIK
>  	bool "Nomadik pin controller driver"
>  	depends on ARCH_U8500 || ARCH_NOMADIK
> diff --git a/drivers/pinctrl/pinctrl-msm8x74.c b/drivers/pinctrl/pinctrl-msm8x74.c
> new file mode 100644
> index 0000000..762552b
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-msm8x74.c
> @@ -0,0 +1,636 @@
[snip]
> +
> +#define FUNCTION(fname)					\
> +	[MSM_MUX_##fname] = {				\
> +		.name = #fname,				\
> +		.groups = fname##_groups,		\
> +		.ngroups = ARRAY_SIZE(fname##_groups),	\
> +	}
> +
> +#define PINGROUP(id, f1, f2, f3, f4, f5, f6, f7)	\
> +	{						\
> +		.name = "gpio" #id,			\
> +		.pins = gpio##id##_pins,		\
> +		.npins = ARRAY_SIZE(gpio##id##_pins),	\
> +		.funcs = {				\
> +			MSM_MUX_NA, /* gpio mode */	\
> +			MSM_MUX_##f1,			\
> +			MSM_MUX_##f2,			\
> +			MSM_MUX_##f3,			\
> +			MSM_MUX_##f4,			\
> +			MSM_MUX_##f5,			\
> +			MSM_MUX_##f6,			\
> +			MSM_MUX_##f7			\
> +		},					\
> +		.ctl_reg = 0x1000 + 0x10 * id ,		\

Weird trailing space here.

Also, do we ever plan to have anything more than the gpio pins and the
sdc pins? It seems like we spend a lot of space describing exactly the
same thing in these structs for each of the 146 gpio pins when we could
just know that range 0 to 146 is gpio pins and have different code for
that part vs the 6 or something sd pins.

> +
> +static struct of_device_id msm8x74_pinctrl_of_match[] = {

const?

>
> +
> +static int __init msm8x74_pinctrl_init(void)
> +{
> +	return platform_driver_register(&msm8x74_pinctrl_driver);
> +}
> +arch_initcall(msm8x74_pinctrl_init);
> +
> +static void __exit msm8x74_pinctrl_exit(void)
> +{
> +	platform_driver_unregister(&msm8x74_pinctrl_driver);
> +}
> +module_exit(msm8x74_pinctrl_exit);

Why not module_platform_driver()? I thought pinctrl supported deferred
probing?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v2 2/3] pinctrl: Add msm8x74 configuration

[Linus Walleij]

On Fri, Dec 6, 2013 at 11:22 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 12/05/13 18:10, Bjorn Andersson wrote:

As the driver is merged I expect fixes to come in as additional patches.

>> Add initial definition of parameters for pinctrl-msm for the msm8x74
>> platform.
>
> Hmm. We've tried to remove 'x' from our code because it isn't really
> accurate and leads to more confusion.

So does this pin controller have a real name in the data sheet?

I usually prefer to name the drivers after the name of the IP
block rather than the SoC if possible.

Or should it just be named pinctrl-msm.c?

>> +#define PINGROUP(id, f1, f2, f3, f4, f5, f6, f7)     \
>> +     {                                               \
>> +             .name = "gpio" #id,                     \
>> +             .pins = gpio##id##_pins,                \
>> +             .npins = ARRAY_SIZE(gpio##id##_pins),   \
>> +             .funcs = {                              \
>> +                     MSM_MUX_NA, /* gpio mode */     \
>> +                     MSM_MUX_##f1,                   \
>> +                     MSM_MUX_##f2,                   \
>> +                     MSM_MUX_##f3,                   \
>> +                     MSM_MUX_##f4,                   \
>> +                     MSM_MUX_##f5,                   \
>> +                     MSM_MUX_##f6,                   \
>> +                     MSM_MUX_##f7                    \
>> +             },                                      \
>> +             .ctl_reg = 0x1000 + 0x10 * id ,         \
>
> Weird trailing space here.

Please send patches.

> Also, do we ever plan to have anything more than the gpio pins and the
> sdc pins? It seems like we spend a lot of space describing exactly the
> same thing in these structs for each of the 146 gpio pins when we could
> just know that range 0 to 146 is gpio pins and have different code for
> that part vs the 6 or something sd pins.

Some platforms use the .gpio_request_enable()/.gpio_disable_free()
instead of one group per pin for this very reason.

>> +static struct of_device_id msm8x74_pinctrl_of_match[] = {
>
> const?

Please send patches.

>> +static int __init msm8x74_pinctrl_init(void)
>> +{
>> +     return platform_driver_register(&msm8x74_pinctrl_driver);
>> +}
>> +arch_initcall(msm8x74_pinctrl_init);
>> +
>> +static void __exit msm8x74_pinctrl_exit(void)
>> +{
>> +     platform_driver_unregister(&msm8x74_pinctrl_driver);
>> +}
>> +module_exit(msm8x74_pinctrl_exit);
>
> Why not module_platform_driver()? I thought pinctrl supported deferred
> probing?

It does. Tested patches accepted.

Yours,
Linus Walleij

[PATCH v2 2/3] pinctrl: Add msm8x74 configuration

[Stephen Boyd]

On 12/09/13 00:18, Linus Walleij wrote:
> On Fri, Dec 6, 2013 at 11:22 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> On 12/05/13 18:10, Bjorn Andersson wrote:
> As the driver is merged I expect fixes to come in as additional patches.
>
>>> Add initial definition of parameters for pinctrl-msm for the msm8x74
>>> platform.
>> Hmm. We've tried to remove 'x' from our code because it isn't really
>> accurate and leads to more confusion.
> So does this pin controller have a real name in the data sheet?

We call it TLMM (top-level mode mux). So far there have been three major
revisions of the hardware and 8974 uses the latest version.

>
> I usually prefer to name the drivers after the name of the IP
> block rather than the SoC if possible.
>
> Or should it just be named pinctrl-msm.c?

Leaving the pinctrl-msm.c file as in these patches is fine. For the SoC
specific data files we should use the base chip name which in this case
is 8974. I suppose the goal of pinctrl-msm8x74.c is to support 8974 and
8074 SoCs in the same file because they're almost exactly the same minus
some pins losing their "modem" functions. Given that, the file names are
fine but the binding and the data structures should be specific about
which SoC we're talking about.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v2 2/3] pinctrl: Add msm8x74 configuration

[Bjorn Andersson]

On Mon 09 Dec 13:37 PST 2013, Stephen Boyd wrote:

> On 12/09/13 00:18, Linus Walleij wrote:
> > On Fri, Dec 6, 2013 at 11:22 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> >> On 12/05/13 18:10, Bjorn Andersson wrote:
> > As the driver is merged I expect fixes to come in as additional patches.
> >
> >>> Add initial definition of parameters for pinctrl-msm for the msm8x74
> >>> platform.
> >> Hmm. We've tried to remove 'x' from our code because it isn't really
> >> accurate and leads to more confusion.
> > So does this pin controller have a real name in the data sheet?
> 
> We call it TLMM (top-level mode mux). So far there have been three major
> revisions of the hardware and 8974 uses the latest version.
> 

The pinctrl-msm.c driver would be the TLMM (v2 and v3) driver...

> >
> > I usually prefer to name the drivers after the name of the IP
> > block rather than the SoC if possible.
> >
> > Or should it just be named pinctrl-msm.c?
> 
> Leaving the pinctrl-msm.c file as in these patches is fine. For the SoC
> specific data files we should use the base chip name which in this case
> is 8974. I suppose the goal of pinctrl-msm8x74.c is to support 8974 and
> 8074 SoCs in the same file because they're almost exactly the same minus
> some pins losing their "modem" functions. Given that, the file names are
> fine but the binding and the data structures should be specific about
> which SoC we're talking about.

...and the pinctrl-msm8x74.c would be the data for SoCs matching that name.

Currently we know that 8074 is a 8974 without modem and the document I have
describing this incarnation of the SoC describes 8274, 8674 and 8974.

So as Stephen suggest; we could keep the name pinctrl-msm8x74.c as is and
we change the compatible to be explicit about this is qcom,msm8974-pinctrl.

That would give us room to handle any differences between 8074 and 8974 within
this file.

What do you say? Should I prepare a patch for this?

Regards,
Bjorn

[PATCH v2 2/3] pinctrl: Add msm8x74 configuration

[Bjorn Andersson]

On Fri 06 Dec 14:22 PST 2013, Stephen Boyd wrote:

> > +config PINCTRL_MSM8X74
> > +	bool "Qualcomm 8x74 pin controller driver"
> > +	select PINCTRL_MSM
> 
> No help?
> 

I could write something up, although I guess someone will add a
select PINCTRL_MSM8X74 in the mach-msm Kconfig and then we're done
with it.

> > +		.ctl_reg = 0x1000 + 0x10 * id ,		\
> 
> Weird trailing space here.
> 

Oops.

> Also, do we ever plan to have anything more than the gpio pins and the
> sdc pins? It seems like we spend a lot of space describing exactly the
> same thing in these structs for each of the 146 gpio pins when we could
> just know that range 0 to 146 is gpio pins and have different code for
> that part vs the 6 or something sd pins.
> 

We have to have a pingroup list, so I did look at having that list
referencing a common object with the gpio/sdc specific data.

But just having it in one large blob is the way that other pinctrl
drivers does it and hence a common problem to pinctrl and multi-platform
arm support.

I have not seen any code in the open referencing the other pins, so I
left them out for now.

> > +
> > +static struct of_device_id msm8x74_pinctrl_of_match[] = {
> 
> const?
> 

Of course.

> >
> > +
> > +static int __init msm8x74_pinctrl_init(void)
> > +{
> > +	return platform_driver_register(&msm8x74_pinctrl_driver);
> > +}
> > +arch_initcall(msm8x74_pinctrl_init);
> > +
> > +static void __exit msm8x74_pinctrl_exit(void)
> > +{
> > +	platform_driver_unregister(&msm8x74_pinctrl_driver);
> > +}
> > +module_exit(msm8x74_pinctrl_exit);
> 
> Why not module_platform_driver()? I thought pinctrl supported deferred
> probing?
> 

As this pinctrl/gpiochip/interrupt-controller is a fairly central part
of the system there's plenty of things that would just end up being deferred.

Most other pinctrl driver does arch_initcall or core_initcall, so I followed
suit and brought it in early.

Regards,
Bjorn

[PATCH v2 2/3] pinctrl: Add msm8x74 configuration

[Stephen Boyd]

On 12/10/13 00:41, Bjorn Andersson wrote:
> On Fri 06 Dec 14:22 PST 2013, Stephen Boyd wrote:
>
>>> +config PINCTRL_MSM8X74
>>> +	bool "Qualcomm 8x74 pin controller driver"
>>> +	select PINCTRL_MSM
>> No help?
>>
> I could write something up, although I guess someone will add a
> select PINCTRL_MSM8X74 in the mach-msm Kconfig and then we're done
> with it.

I hope nobody selects the PINCTRL_MSM8X74 config. It's certainly not
necessary to boot into a ramdisk. I thought checkpatch complained about
missing help for visible options.

>
>> Also, do we ever plan to have anything more than the gpio pins and the
>> sdc pins? It seems like we spend a lot of space describing exactly the
>> same thing in these structs for each of the 146 gpio pins when we could
>> just know that range 0 to 146 is gpio pins and have different code for
>> that part vs the 6 or something sd pins.
>>
> We have to have a pingroup list, so I did look at having that list
> referencing a common object with the gpio/sdc specific data.
>
> But just having it in one large blob is the way that other pinctrl
> drivers does it and hence a common problem to pinctrl and multi-platform
> arm support.

I don't follow what Linus is recommending. How could
.gpio_request_enable()/.gpio_disable_free() help us here?

>
> I have not seen any code in the open referencing the other pins, so I
> left them out for now.
>
>


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v2 2/3] pinctrl: Add msm8x74 configuration

[Linus Walleij]

On Wed, Dec 11, 2013 at 2:49 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 12/10/13 00:41, Bjorn Andersson wrote:
>> On Fri 06 Dec 14:22 PST 2013, Stephen Boyd wrote:

>>> Also, do we ever plan to have anything more than the gpio pins and the
>>> sdc pins? It seems like we spend a lot of space describing exactly the
>>> same thing in these structs for each of the 146 gpio pins when we could
>>> just know that range 0 to 146 is gpio pins and have different code for
>>> that part vs the 6 or something sd pins.
>>>
>> We have to have a pingroup list, so I did look at having that list
>> referencing a common object with the gpio/sdc specific data.
>>
>> But just having it in one large blob is the way that other pinctrl
>> drivers does it and hence a common problem to pinctrl and multi-platform
>> arm support.
>
> I don't follow what Linus is recommending. How could
> .gpio_request_enable()/.gpio_disable_free() help us here?

That removes the need to define a single group for each pin that
can be used as GPIO.

It is described in Documentation/pinctrl.txt under the heading
"Pin control interaction with the GPIO subsystem"

On a related key I guess drivers/gpio/gpio-msm-v2.c needs
to be patched to call pinctrl_request_gpio()/
pinctrl_free_gpio()/pinctrl_gpio_direction_input()/
pinctrl_gpio_direction_output() before the GPIO back-end
works at all so there is still time to change this (plus it is
not part of the DT binding or anything so it can be changed
at any time).

Yours,
Linus Walleij

[PATCH v2 2/3] pinctrl: Add msm8x74 configuration

[Linus Walleij]

On Thu, Dec 12, 2013 at 8:15 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> On a related key I guess drivers/gpio/gpio-msm-v2.c needs
> to be patched to call pinctrl_request_gpio()/
> pinctrl_free_gpio()/pinctrl_gpio_direction_input()/
> pinctrl_gpio_direction_output() before the GPIO back-end
> works at all so there is still time to change this (plus it is
> not part of the DT binding or anything so it can be changed
> at any time).

I think Bj?rn answered this in his other mail - so the new
pin control driver replaces the GPIO drivers' functionality
and that's just fine.

Yours,
Linus Walleij

[PATCH v2 2/3] pinctrl: Add msm8x74 configuration

[Bjorn Andersson]

On Thu 12 Dec 11:15 PST 2013, Linus Walleij wrote:

> On Wed, Dec 11, 2013 at 2:49 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
...
> > I don't follow what Linus is recommending. How could
> > .gpio_request_enable()/.gpio_disable_free() help us here?
> 
> That removes the need to define a single group for each pin that
> can be used as GPIO.

For the TLMM chip the pin can not be used as GPIO when another function is
selected for the pin. And upon leaving a state with a function defined the
choosen mux is disabled, turning the pin back to a GPIO pin.
So I don't think this is applicable for us, any unused pin is a GPIO pin.


The reason why there are 1 pingroup per pin is because a pingroup represent
a configurable entity, i.e. a config register in the TLMM chip; which is one
per pin for the GPIO pins.

Regards,
Bjorn

[PATCH v2 2/3] pinctrl: Add msm8x74 configuration

[Linus Walleij]

On Fri, Dec 13, 2013 at 5:24 AM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> On Thu 12 Dec 11:15 PST 2013, Linus Walleij wrote:
>> On Wed, Dec 11, 2013 at 2:49 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> ...
>> > I don't follow what Linus is recommending. How could
>> > .gpio_request_enable()/.gpio_disable_free() help us here?
>>
>> That removes the need to define a single group for each pin that
>> can be used as GPIO.
>
> For the TLMM chip the pin can not be used as GPIO when another function is
> selected for the pin. And upon leaving a state with a function defined the
> choosen mux is disabled, turning the pin back to a GPIO pin.
> So I don't think this is applicable for us, any unused pin is a GPIO pin.

Well it can be used, it is not imperative to exploit the fact that the
subsystem this way allows you to use a pin as a function and GPIO
at the same time, and that is actually not why it is there.

It is there to avoid adding too many one-pin gpio groups :-)

If you still want to have the "check" that we don't enable GPIO
on something that is used by another function or vice versa
that can be NACKed by the driver, for example by using a
bitmap to indicate if the pin is used by a GPIO and NACKing
the mux setting on that pin in this case (etc).

However *I'm* OK with one pin group per pin if you prefer
it this way, it's just an option, no way is it compulsory, to
do it with the special callbacks.

Yours,
Linus Walleij

[PATCH v2 1/3] pinctrl: Add Qualcomm TLMM driver

[Stephen Boyd]

General nitpick: There seems to be a lot of checks for invalid input in
the op functions. I hope that they're all unnecessary debugging that can
be removed.

On 12/05/13 18:10, Bjorn Andersson wrote:
> This adds a pinctrl, pinmux, pinconf and gpiolib driver for the
> Qualcomm TLMM block.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
>  drivers/pinctrl/Kconfig       |    6 +
>  drivers/pinctrl/Makefile      |    1 +
>  drivers/pinctrl/pinctrl-msm.c | 1028 +++++++++++++++++++++++++++++++++++++++++
>  drivers/pinctrl/pinctrl-msm.h |  123 +++++
>  4 files changed, 1158 insertions(+)
>  create mode 100644 drivers/pinctrl/pinctrl-msm.c
>  create mode 100644 drivers/pinctrl/pinctrl-msm.h
>
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 33f9dc1..d0b6846 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -202,6 +202,12 @@ config PINCTRL_IMX28
>  	bool
>  	select PINCTRL_MXS
>  
> +config PINCTRL_MSM
> +	bool

Why not tristate?

> +	select PINMUX
> +	select PINCONF
> +	select GENERIC_PINCONF
> +
>  config PINCTRL_NOMADIK
>  	bool "Nomadik pin controller driver"
>  	depends on ARCH_U8500 || ARCH_NOMADIK
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index 4f7be29..a525f8b 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -35,6 +35,7 @@ obj-$(CONFIG_PINCTRL_FALCON)	+= pinctrl-falcon.o
>  obj-$(CONFIG_PINCTRL_MXS)	+= pinctrl-mxs.o
>  obj-$(CONFIG_PINCTRL_IMX23)	+= pinctrl-imx23.o
>  obj-$(CONFIG_PINCTRL_IMX28)	+= pinctrl-imx28.o
> +obj-$(CONFIG_PINCTRL_MSM)	+= pinctrl-msm.o
>  obj-$(CONFIG_PINCTRL_NOMADIK)	+= pinctrl-nomadik.o
>  obj-$(CONFIG_PINCTRL_STN8815)	+= pinctrl-nomadik-stn8815.o
>  obj-$(CONFIG_PINCTRL_DB8500)	+= pinctrl-nomadik-db8500.o
> diff --git a/drivers/pinctrl/pinctrl-msm.c b/drivers/pinctrl/pinctrl-msm.c
> new file mode 100644
> index 0000000..28b90ab
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-msm.c
> @@ -0,0 +1,1028 @@
> +/*
> + * Copyright (c) 2013, Sony Mobile Communications AB.
> + * Copyright (c) 2013, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/irqdomain.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/machine.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/slab.h>
> +#include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/of_irq.h>
> +#include <linux/spinlock.h>
> +
> +#include "core.h"
> +#include "pinconf.h"
> +#include "pinctrl-msm.h"
> +#include "pinctrl-utils.h"
> +
> +/**
> + * struct msm_pinctrl - state for a pinctrl-msm device
> + * @dev:            device handle.
> + * @pctrl:          pinctrl handle.
> + * @domain:         irqdomain handle.
> + * @chip:           gpiochip handle.
> + * @irq:            parent irq for the TLMM irq_chip.
> + * @lock:           Spinlock to protect register resources as well
> + *                  as msm_pinctrl data structures.
> + * @enabled_irqs:   Bitmap of currently enabled irqs.
> + * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
> + *                  detection.
> + * @wake_irqs:      Bitmap of irqs with requested as wakeup source.
> + * @soc;            Reference to soc_data of platform specific data.
> + * @regs:           Base address for the TLMM register map.
> + */
> +struct msm_pinctrl {
> +	struct device *dev;
> +	struct pinctrl_dev *pctrl;
> +	struct irq_domain *domain;
> +	struct gpio_chip chip;
> +	unsigned irq;
> +
> +	spinlock_t lock;
> +
> +	unsigned long *enabled_irqs;
> +	unsigned long *dual_edge_irqs;
> +	unsigned long *wake_irqs;

In the gpio driver we went with a static upper limit on the bitmap. That
allowed us to avoid small allocations fragmenting the heap. Please do
the same here (look at gpio-msm-v2.c).

> +
> +	const struct msm_pinctrl_soc_data *soc;
> +	void __iomem *regs;
> +};
> +
> +static int msm_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pctrl->soc->ngroups;
> +}
> +
> +static const char *msm_get_group_name(struct pinctrl_dev *pctldev,
> +				      unsigned group)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pctrl->soc->groups[group].name;
> +}
> +
> +static int msm_get_group_pins(struct pinctrl_dev *pctldev,
> +			      unsigned group,
> +			      const unsigned **pins,
> +			      unsigned *num_pins)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	*pins = pctrl->soc->groups[group].pins;
> +	*num_pins = pctrl->soc->groups[group].npins;
> +	return 0;
> +}
> +
> +static struct pinctrl_ops msm_pinctrl_ops = {

const?

> +	.get_groups_count	= msm_get_groups_count,
> +	.get_group_name		= msm_get_group_name,
> +	.get_group_pins		= msm_get_group_pins,
> +	.dt_node_to_map		= pinconf_generic_dt_node_to_map_group,
> +	.dt_free_map		= pinctrl_utils_dt_free_map,
> +};
> +
> +static int msm_get_functions_count(struct pinctrl_dev *pctldev)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pctrl->soc->nfunctions;
> +}
> +
> +static const char *msm_get_function_name(struct pinctrl_dev *pctldev,
> +					 unsigned function)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pctrl->soc->functions[function].name;
> +}
> +
> +static int msm_get_function_groups(struct pinctrl_dev *pctldev,
> +				   unsigned function,
> +				   const char * const **groups,
> +				   unsigned * const num_groups)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	*groups = pctrl->soc->functions[function].groups;
> +	*num_groups = pctrl->soc->functions[function].ngroups;
> +	return 0;
> +}
> +
> +static int msm_pinmux_enable(struct pinctrl_dev *pctldev,
> +			     unsigned function,
> +			     unsigned group)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	const struct msm_pingroup *g;
> +	unsigned long flags;
> +	u32 val;
> +	int i;
> +
> +	g = &pctrl->soc->groups[group];
> +
> +	if (WARN_ON(g->mux_bit < 0))
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(g->funcs); i++) {
> +		if (g->funcs[i] == function)
> +			break;
> +	}
> +
> +	if (WARN_ON(i == ARRAY_SIZE(g->funcs)))
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	val = readl(pctrl->regs + g->ctl_reg);
> +	val &= ~(0x7 << g->mux_bit);
> +	val |= i << g->mux_bit;
> +	writel(val, pctrl->regs + g->ctl_reg);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	return 0;
> +}
> +
> +static void msm_pinmux_disable(struct pinctrl_dev *pctldev,
> +			       unsigned function,
> +			       unsigned group)
> +{
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	const struct msm_pingroup *g;
> +	unsigned long flags;
> +	u32 val;
> +
> +	g = &pctrl->soc->groups[group];
> +
> +	if (WARN_ON(g->mux_bit < 0))
> +		return;
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	/* Clear the mux bits to select gpio mode */
> +	val = readl(pctrl->regs + g->ctl_reg);
> +	val &= ~(0x7 << g->mux_bit);
> +	writel(val, pctrl->regs + g->ctl_reg);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +}
> +
> +static struct pinmux_ops msm_pinmux_ops = {

const?

> +	.get_functions_count	= msm_get_functions_count,
> +	.get_function_name	= msm_get_function_name,
> +	.get_function_groups	= msm_get_function_groups,
> +	.enable			= msm_pinmux_enable,
> +	.disable		= msm_pinmux_disable,
> +};
> +
> +static int msm_config_reg(struct msm_pinctrl *pctrl,
> +			  const struct msm_pingroup *g,
> +			  unsigned param,
> +			  unsigned *reg,
> +			  unsigned *mask,
> +			  unsigned *bit)
> +{
> +	switch (param) {
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		*reg = g->ctl_reg;
> +		*bit = g->pull_bit;
> +		*mask = 3;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		*reg = g->ctl_reg;
> +		*bit = g->pull_bit;
> +		*mask = 3;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		*reg = g->ctl_reg;
> +		*bit = g->pull_bit;
> +		*mask = 3;
> +		break;
> +	case PIN_CONFIG_DRIVE_STRENGTH:
> +		*reg = g->ctl_reg;
> +		*bit = g->drv_bit;
> +		*mask = 7;
> +		break;
> +	default:
> +		dev_err(pctrl->dev, "Invalid config param %04x\n", param);
> +		return -ENOTSUPP;
> +	}
> +
> +	if (*reg < 0) {
> +		dev_err(pctrl->dev, "Config param %04x not supported on group %s\n",
> +			param, g->name);
> +		return -ENOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int msm_config_get(struct pinctrl_dev *pctldev,
> +			  unsigned int pin,
> +			  unsigned long *config)
> +{
> +	dev_err(pctldev->dev, "pin_config_set op not supported\n");
> +	return -ENOTSUPP;
> +}
> +
> +static int msm_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
> +				unsigned long *configs, unsigned num_configs)
> +{
> +	dev_err(pctldev->dev, "pin_config_set op not supported\n");
> +	return -ENOTSUPP;
> +}
> +
> +#define MSM_NO_PULL	0
> +#define MSM_PULL_DOWN	1
> +#define MSM_PULL_UP	3
> +
> +static const unsigned msm_regval_to_drive[] = { 2, 4, 6, 8, 10, 12, 14, 16 };
> +static const unsigned msm_drive_to_regval[] = { -1, -1, 0, -1, 1, -1, 2, -1, 3, -1, 4, -1, 5, -1, 6, -1, 7 };
> +
> +static int msm_config_group_get(struct pinctrl_dev *pctldev,
> +				unsigned int group,
> +				unsigned long *config)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	unsigned param = pinconf_to_config_param(*config);
> +	unsigned mask;
> +	unsigned arg;
> +	unsigned bit;
> +	unsigned reg;
> +	int ret;
> +	u32 val;
> +
> +	g = &pctrl->soc->groups[group];
> +
> +	ret = msm_config_reg(pctrl, g, param, &reg, &mask, &bit);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = readl(pctrl->regs + reg);
> +	arg = (val >> bit) & mask;
> +
> +	/* Convert register value to pinconf value */
> +	switch (param) {
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		arg = arg == MSM_NO_PULL;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		arg = arg == MSM_PULL_DOWN;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		arg = arg == MSM_PULL_UP;
> +		break;
> +	case PIN_CONFIG_DRIVE_STRENGTH:
> +		arg = msm_regval_to_drive[arg];
> +		break;
> +	default:
> +		dev_err(pctrl->dev, "Unsupported config parameter: %x\n",
> +			param);
> +		return -EINVAL;
> +	}
> +
> +	*config = pinconf_to_config_packed(param, arg);
> +
> +	return 0;
> +}
> +
> +static int msm_config_group_set(struct pinctrl_dev *pctldev,
> +				unsigned group,
> +				unsigned long *configs,
> +				unsigned num_configs)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +	unsigned long flags;
> +	unsigned param;
> +	unsigned mask;
> +	unsigned arg;
> +	unsigned bit;
> +	unsigned reg;
> +	int ret;
> +	u32 val;
> +	int i;
> +
> +	g = &pctrl->soc->groups[group];
> +
> +	for (i = 0; i < num_configs; i++) {
> +		param = pinconf_to_config_param(configs[i]);
> +		arg = pinconf_to_config_argument(configs[i]);
> +
> +		ret = msm_config_reg(pctrl, g, param, &reg, &mask, &bit);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Convert pinconf values to register values */
> +		switch (param) {
> +		case PIN_CONFIG_BIAS_DISABLE:
> +			arg = MSM_NO_PULL;
> +			break;
> +		case PIN_CONFIG_BIAS_PULL_DOWN:
> +			arg = MSM_PULL_DOWN;
> +			break;
> +		case PIN_CONFIG_BIAS_PULL_UP:
> +			arg = MSM_PULL_UP;
> +			break;
> +		case PIN_CONFIG_DRIVE_STRENGTH:
> +			/* Check for invalid values */
> +			if (arg > ARRAY_SIZE(msm_drive_to_regval))
> +				arg = -1;
> +			else
> +				arg = msm_drive_to_regval[arg];
> +			break;
> +		default:
> +			dev_err(pctrl->dev, "Unsupported config parameter: %x\n",
> +				param);
> +			return -EINVAL;
> +		}
> +
> +		/* Range-check user-supplied value */
> +		if (arg & ~mask) {
> +			dev_err(pctrl->dev, "config %x: %x is invalid\n", param, arg);
> +			return -EINVAL;
> +		}
> +
> +		spin_lock_irqsave(&pctrl->lock, flags);
> +		val = readl(pctrl->regs + reg);
> +		val &= ~(mask << bit);
> +		val |= arg << bit;
> +		writel(val, pctrl->regs + reg);
> +		spin_unlock_irqrestore(&pctrl->lock, flags);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct pinconf_ops msm_pinconf_ops = {

const?

> +	.pin_config_get		= msm_config_get,
> +	.pin_config_set		= msm_config_set,
> +	.pin_config_group_get	= msm_config_group_get,
> +	.pin_config_group_set	= msm_config_group_set,
> +};
> +
> +static struct pinctrl_desc msm_pinctrl_desc = {
> +	.pctlops = &msm_pinctrl_ops,
> +	.pmxops = &msm_pinmux_ops,
> +	.confops = &msm_pinconf_ops,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int msm_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> +	unsigned long flags;
> +	u32 val;
> +
> +	if (WARN_ON(offset >= pctrl->soc->ngroups))
> +		return -EINVAL;
> +
> +	g = &pctrl->soc->groups[offset];
> +
> +	if (WARN_ON(g->oe_bit < 0))
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	val = readl(pctrl->regs + g->ctl_reg);
> +	val &= ~BIT(g->oe_bit);
> +	writel(val, pctrl->regs + g->ctl_reg);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int msm_gpio_direction_output(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> +	unsigned long flags;
> +	u32 val;
> +
> +	if (WARN_ON(offset >= pctrl->soc->ngroups))
> +		return -EINVAL;

How is this possible?

> +
> +	g = &pctrl->soc->groups[offset];
> +
> +	if (WARN_ON(g->oe_bit < 0))
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	writel(value ? BIT(g->out_bit) : 0, pctrl->regs + g->io_reg);
> +
> +	val = readl(pctrl->regs + g->ctl_reg);
> +	val |= BIT(g->oe_bit);
> +	writel(val, pctrl->regs + g->ctl_reg);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int msm_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> +	u32 val;
> +
> +	if (WARN_ON(offset >= pctrl->soc->ngroups))
> +		return -EINVAL;
> +
> +	g = &pctrl->soc->groups[offset];
> +
> +	val = readl(pctrl->regs + g->io_reg);
> +	return !!(val & BIT(g->in_bit));
> +}
> +
> +static void msm_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> +	unsigned long flags;
> +	u32 val;
> +
> +	if (WARN_ON(offset >= pctrl->soc->ngroups))
> +		return;
> +
> +	g = &pctrl->soc->groups[offset];
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	val = readl(pctrl->regs + g->io_reg);
> +	val |= BIT(g->out_bit);
> +	writel(val, pctrl->regs + g->io_reg);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +}
> +
> +static int msm_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> +
> +	return irq_find_mapping(pctrl->domain, offset);
> +}
> +
> +static int msm_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +	int gpio = chip->base + offset;
> +	return pinctrl_request_gpio(gpio);
> +}
> +
> +static void msm_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +	int gpio = chip->base + offset;
> +	return pinctrl_free_gpio(gpio);
> +}
> +
> +#ifdef CONFIG_DEBUG_FS
> +#include <linux/seq_file.h>
> +
> +static void msm_gpio_dbg_show_one(struct seq_file *s,
> +				  struct pinctrl_dev *pctldev,
> +				  struct gpio_chip *chip,
> +				  unsigned offset,
> +				  unsigned gpio)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> +	unsigned func;
> +	int is_out;
> +	int drive;
> +	int pull;
> +	u32 ctl_reg;
> +
> +	const char *pulls[] = {

static const char * const ?

> +		"no pull",
> +		"pull down",
> +		"keeper",
> +		"pull up"
> +	};
> +
> +	g = &pctrl->soc->groups[offset];
> +	ctl_reg = readl(pctrl->regs + g->ctl_reg);
> +
> +	is_out = !!(ctl_reg & BIT(g->oe_bit));
> +	func = (ctl_reg >> g->mux_bit) & 7;
> +	drive = (ctl_reg >> g->drv_bit) & 7;
> +	pull = (ctl_reg >> g->pull_bit) & 3;
> +
> +	seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" : "in", func);
> +	seq_printf(s, " %dmA", msm_regval_to_drive[drive]);
> +	seq_printf(s, " %s", pulls[pull]);
> +}
> +
> +static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> +{
> +	unsigned gpio = chip->base;
> +	unsigned i;
> +
> +	for (i = 0; i < chip->ngpio; i++, gpio++) {
> +		msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
> +		seq_printf(s, "\n");

seq_puts()?

> +	}
> +}
> +
> +#else
> +#define msm_gpio_dbg_show NULL
> +#endif
> +
> +static struct gpio_chip msm_gpio_template = {
> +	.direction_input  = msm_gpio_direction_input,
> +	.direction_output = msm_gpio_direction_output,
> +	.get              = msm_gpio_get,
> +	.set              = msm_gpio_set,
> +	.to_irq           = msm_gpio_to_irq,
> +	.request          = msm_gpio_request,
> +	.free             = msm_gpio_free,
> +	.dbg_show         = msm_gpio_dbg_show,
> +};
> +
> +/* For dual-edge interrupts in software, since some hardware has no
> + * such support:
> + *
> + * At appropriate moments, this function may be called to flip the polarity
> + * settings of both-edge irq lines to try and catch the next edge.
> + *
> + * The attempt is considered successful if:
> + * - the status bit goes high, indicating that an edge was caught, or
> + * - the input value of the gpio doesn't change during the attempt.
> + * If the value changes twice during the process, that would cause the first
> + * test to fail but would force the second, as two opposite
> + * transitions would cause a detection no matter the polarity setting.
> + *
> + * The do-loop tries to sledge-hammer closed the timing hole between
> + * the initial value-read and the polarity-write - if the line value changes
> + * during that window, an interrupt is lost, the new polarity setting is
> + * incorrect, and the first success test will fail, causing a retry.
> + *
> + * Algorithm comes from Google's msmgpio driver.
> + */
> +static void msm_gpio_update_dual_edge_pos(struct msm_pinctrl *pctrl,
> +					  const struct msm_pingroup *g,
> +					  struct irq_data *d)
> +{
> +	int loop_limit = 100;
> +	unsigned val, val2, intstat;
> +	unsigned pol;
> +
> +	do {
> +		val = readl(pctrl->regs + g->io_reg) & BIT(g->in_bit);
> +
> +		pol = readl(pctrl->regs + g->intr_cfg_reg);
> +		pol ^= BIT(g->intr_polarity_bit);
> +		writel(pol, pctrl->regs + g->intr_cfg_reg);
> +
> +		val2 = readl(pctrl->regs + g->io_reg) & BIT(g->in_bit);
> +		intstat = readl(pctrl->regs + g->intr_status_reg);
> +		if (intstat || (val == val2))
> +			return;
> +	} while (loop_limit-- > 0);
> +	dev_err(pctrl->dev, "dual-edge irq failed to stabilize, %#08x != %#08x\n",
> +		val, val2);
> +}
> +
> +static void msm_gpio_irq_mask(struct irq_data *d)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl;
> +	unsigned long flags;
> +	u32 val;
> +
> +	pctrl = irq_data_get_irq_chip_data(d);
> +	if (!pctrl)
> +		return;
> +
> +	if (WARN_ON(d->hwirq >= pctrl->soc->ngroups))
> +		return;
> +
> +	g = &pctrl->soc->groups[d->hwirq];
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	val = readl(pctrl->regs + g->intr_cfg_reg);
> +	val &= ~BIT(g->intr_enable_bit);
> +	writel(val, pctrl->regs + g->intr_cfg_reg);
> +
> +	clear_bit(d->hwirq, pctrl->enabled_irqs);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +}
> +
> +static void msm_gpio_irq_unmask(struct irq_data *d)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl;
> +	unsigned long flags;
> +	u32 val;
> +
> +	pctrl = irq_data_get_irq_chip_data(d);
> +	if (!pctrl)
> +		return;
> +
> +	if (WARN_ON(d->hwirq >= pctrl->soc->ngroups))
> +		return;
> +
> +	g = &pctrl->soc->groups[d->hwirq];
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	val = readl(pctrl->regs + g->intr_status_reg);
> +	val &= ~BIT(g->intr_status_bit);
> +	writel(val, pctrl->regs + g->intr_status_reg);
> +
> +	val = readl(pctrl->regs + g->intr_cfg_reg);
> +	val |= BIT(g->intr_enable_bit);
> +	writel(val, pctrl->regs + g->intr_cfg_reg);
> +
> +	set_bit(d->hwirq, pctrl->enabled_irqs);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +}
> +
> +static void msm_gpio_irq_ack(struct irq_data *d)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl;
> +	unsigned long flags;
> +	u32 val;
> +
> +	pctrl = irq_data_get_irq_chip_data(d);
> +	if (!pctrl)
> +		return;
> +
> +	if (WARN_ON(d->hwirq >= pctrl->soc->ngroups))
> +		return;
> +
> +	g = &pctrl->soc->groups[d->hwirq];
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	val = readl(pctrl->regs + g->intr_status_reg);
> +	val &= ~BIT(g->intr_status_bit);
> +	writel(val, pctrl->regs + g->intr_status_reg);
> +
> +	if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
> +		msm_gpio_update_dual_edge_pos(pctrl, g, d);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +}
> +
> +#define INTR_TARGET_PROC_APPS    4
> +
> +static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl;
> +	unsigned long flags;
> +	u32 val;
> +
> +	pctrl = irq_data_get_irq_chip_data(d);
> +	if (!pctrl)
> +		return -EINVAL;

How is this possible?

> +
> +	if (WARN_ON(d->hwirq >= pctrl->soc->ngroups))
> +		return -EINVAL;
> +
> +	g = &pctrl->soc->groups[d->hwirq];
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	/*
> +	 * For hw without possibility of detecting both edges
> +	 */
> +	if (g->intr_detection_width == 1 && type == IRQ_TYPE_EDGE_BOTH)
> +		set_bit(d->hwirq, pctrl->dual_edge_irqs);
> +	else
> +		clear_bit(d->hwirq, pctrl->dual_edge_irqs);
> +
> +	/* Route interrupts to application cpu */
> +	val = readl(pctrl->regs + g->intr_target_reg);
> +	val &= ~(7 << g->intr_target_bit);
> +	val |= INTR_TARGET_PROC_APPS << g->intr_target_bit;
> +	writel(val, pctrl->regs + g->intr_target_reg);
> +
> +	/* Update configuration for gpio.
> +	 * RAW_STATUS_EN is left on for all gpio irqs. Due to the
> +	 * internal circuitry of TLMM, toggling the RAW_STATUS
> +	 * could cause the INTR_STATUS to be set for EDGE interrupts.
> +	 */
> +	val = readl(pctrl->regs + g->intr_cfg_reg);
> +	val |= BIT(g->intr_raw_status_bit);
> +	if (g->intr_detection_width == 2) {
> +		val &= ~(3 << g->intr_detection_bit);
> +		val &= ~(1 << g->intr_polarity_bit);
> +		switch (type) {
> +		case IRQ_TYPE_EDGE_RISING:
> +			val |= 1 << g->intr_detection_bit;
> +			val |= BIT(g->intr_polarity_bit);
> +			break;
> +		case IRQ_TYPE_EDGE_FALLING:
> +			val |= 2 << g->intr_detection_bit;
> +			val |= BIT(g->intr_polarity_bit);
> +			break;
> +		case IRQ_TYPE_EDGE_BOTH:
> +			val |= 3 << g->intr_detection_bit;
> +			val |= BIT(g->intr_polarity_bit);
> +			break;
> +		case IRQ_TYPE_LEVEL_LOW:
> +			break;
> +		case IRQ_TYPE_LEVEL_HIGH:
> +			val |= BIT(g->intr_polarity_bit);
> +			break;
> +		}
> +	} else if (g->intr_detection_width == 1) {
> +		val &= ~(1 << g->intr_detection_bit);
> +		val &= ~(1 << g->intr_polarity_bit);
> +		switch (type) {
> +		case IRQ_TYPE_EDGE_RISING:
> +			val |= BIT(g->intr_detection_bit);
> +			val |= BIT(g->intr_polarity_bit);
> +			break;
> +		case IRQ_TYPE_EDGE_FALLING:
> +			val |= BIT(g->intr_detection_bit);
> +			break;
> +		case IRQ_TYPE_EDGE_BOTH:
> +			val |= BIT(g->intr_detection_bit);
> +			break;
> +		case IRQ_TYPE_LEVEL_LOW:
> +			break;
> +		case IRQ_TYPE_LEVEL_HIGH:
> +			val |= BIT(g->intr_polarity_bit);
> +			break;
> +		}
> +	} else {
> +		BUG();
> +	}

This would be better written as a collection of ifs so that
IRQ_TYPE_EDGE_BOTH doesn't have to be tested and we duplicate less code.

> +	writel(val, pctrl->regs + g->intr_cfg_reg);
> +
> +	if (test_bit(d->hwirq, pctrl->dual_edge_irqs))
> +		msm_gpio_update_dual_edge_pos(pctrl, g, d);
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
> +		__irq_set_handler_locked(d->irq, handle_level_irq);
> +	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
> +		__irq_set_handler_locked(d->irq, handle_edge_irq);
> +
> +	return 0;
> +}
> +
> +static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
> +{
> +	struct msm_pinctrl *pctrl;
> +	unsigned long flags;
> +	unsigned ngpio;
> +
> +	pctrl = irq_data_get_irq_chip_data(d);
> +	if (!pctrl)
> +		return -EINVAL;
> +
> +	ngpio = pctrl->chip.ngpio;
> +
> +	spin_lock_irqsave(&pctrl->lock, flags);
> +
> +	if (on) {
> +		if (bitmap_empty(pctrl->wake_irqs, ngpio))
> +			enable_irq_wake(pctrl->irq);
> +		set_bit(d->hwirq, pctrl->wake_irqs);
> +	} else {
> +		clear_bit(d->hwirq, pctrl->wake_irqs);
> +		if (bitmap_empty(pctrl->wake_irqs, ngpio))
> +			disable_irq_wake(pctrl->irq);
> +	}
> +
> +	spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +	return 0;
> +}
> +
> +static unsigned int msm_gpio_irq_startup(struct irq_data *d)
> +{
> +	struct msm_pinctrl *pctrl = irq_data_get_irq_chip_data(d);
> +
> +	if (gpio_lock_as_irq(&pctrl->chip, d->hwirq)) {
> +		dev_err(pctrl->dev, "unable to lock HW IRQ %lu for IRQ\n",
> +			d->hwirq);
> +	}
> +	msm_gpio_irq_unmask(d);
> +	return 0;
> +}
> +
> +static void msm_gpio_irq_shutdown(struct irq_data *d)
> +{
> +	struct msm_pinctrl *pctrl = irq_data_get_irq_chip_data(d);
> +
> +	msm_gpio_irq_mask(d);
> +	gpio_unlock_as_irq(&pctrl->chip, d->hwirq);
> +}
> +
> +static struct irq_chip msm_gpio_irq_chip = {
> +	.name           = "msmgpio",
> +	.irq_mask       = msm_gpio_irq_mask,
> +	.irq_unmask     = msm_gpio_irq_unmask,
> +	.irq_ack        = msm_gpio_irq_ack,
> +	.irq_set_type   = msm_gpio_irq_set_type,
> +	.irq_set_wake   = msm_gpio_irq_set_wake,
> +	.irq_startup	= msm_gpio_irq_startup,
> +	.irq_shutdown	= msm_gpio_irq_shutdown,
> +};
> +
> +static void msm_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> +	const struct msm_pingroup *g;
> +	struct msm_pinctrl *pctrl = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_get_chip(irq);
> +	int irq_pin;
> +	int handled = 0;
> +	u32 val;
> +	int i;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	/*
> +	 * Each pin have it's own IRQ status register, so use

s/have/has/

> +	 * enabled_irq bitmap to limit the number of reads.
> +	 */
> +	for_each_set_bit(i, pctrl->enabled_irqs, pctrl->chip.ngpio) {
> +		g = &pctrl->soc->groups[i];
> +		val = readl(pctrl->regs + g->intr_status_reg);
> +		if (val & BIT(g->intr_status_bit)) {
> +			irq_pin = irq_find_mapping(pctrl->domain, i);
> +			generic_handle_irq(irq_pin);
> +			handled++;
> +		}
> +	}
> +
> +	/* No interrutps where flagged */

s/where/were/
s/interrutps/interrupts/
> +	if (handled == 0)
> +		handle_bad_irq(irq, desc);
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static int msm_gpio_init(struct msm_pinctrl *pctrl)
> +{
> +	struct gpio_chip *chip;
> +	int irq;
> +	int ret;
> +	int i;
> +	int r;
> +
> +	chip = &pctrl->chip;
> +	chip->base = 0;
> +	chip->ngpio = pctrl->soc->ngpios;
> +	chip->label = dev_name(pctrl->dev);
> +	chip->dev = pctrl->dev;
> +	chip->owner = THIS_MODULE;
> +	chip->of_node = pctrl->dev->of_node;
> +
> +	pctrl->enabled_irqs = devm_kzalloc(pctrl->dev,
> +					   sizeof(unsigned long) * BITS_TO_LONGS(chip->ngpio),
> +					   GFP_KERNEL);

If you don't agree with how gpio-msm-v2.c does it, please use
devm_kcalloc().

> +	if (!pctrl->enabled_irqs) {
> +		dev_err(pctrl->dev, "Failed to allocate enabled_irqs bitmap\n");
> +		return -ENOMEM;
> +	}
> +
> +	pctrl->dual_edge_irqs = devm_kzalloc(pctrl->dev,
> +					     sizeof(unsigned long) * BITS_TO_LONGS(chip->ngpio),
> +					     GFP_KERNEL);
> +	if (!pctrl->dual_edge_irqs) {
> +		dev_err(pctrl->dev, "Failed to allocate dual_edge_irqs bitmap\n");
> +		return -ENOMEM;
> +	}
> +
> +	pctrl->wake_irqs = devm_kzalloc(pctrl->dev,
> +					sizeof(unsigned long) * BITS_TO_LONGS(chip->ngpio),
> +					GFP_KERNEL);
> +	if (!pctrl->wake_irqs) {
> +		dev_err(pctrl->dev, "Failed to allocate wake_irqs bitmap\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = gpiochip_add(&pctrl->chip);
> +	if (ret) {
> +		dev_err(pctrl->dev, "Failed register gpiochip\n");
> +		return ret;
> +	}
> +
> +	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
> +	if (ret) {
> +		dev_err(pctrl->dev, "Failed to add pin range\n");
> +		return ret;
> +	}
> +
> +	pctrl->domain = irq_domain_add_linear(pctrl->dev->of_node, chip->ngpio,
> +					      &irq_domain_simple_ops, NULL);
> +	if (!pctrl->domain) {
> +		dev_err(pctrl->dev, "Failed to register irq domain\n");
> +		r = gpiochip_remove(&pctrl->chip);
> +		return -ENOSYS;
> +	}
> +
> +	for (i = 0; i < chip->ngpio; i++) {
> +		irq = irq_create_mapping(pctrl->domain, i);
> +		irq_set_chip_and_handler(irq, &msm_gpio_irq_chip, handle_edge_irq);
> +		irq_set_chip_data(irq, pctrl);
> +	}
> +
> +	irq_set_handler_data(pctrl->irq, pctrl);
> +	irq_set_chained_handler(pctrl->irq, msm_gpio_irq_handler);
> +
> +	return 0;
> +}
> +
> +int msm_pinctrl_probe(struct platform_device *pdev,
> +		      const struct msm_pinctrl_soc_data *soc_data)
> +{
> +	struct msm_pinctrl *pctrl;
> +	struct resource *res;
> +	int ret;
> +
> +	pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
> +	if (!pctrl) {
> +		dev_err(&pdev->dev, "Can't allocate msm_pinctrl\n");
> +		return -ENOMEM;
> +	}
> +	pctrl->dev = &pdev->dev;
> +	pctrl->soc = soc_data;
> +	pctrl->chip = msm_gpio_template;
> +
> +	spin_lock_init(&pctrl->lock);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	pctrl->regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(pctrl->regs))
> +		return PTR_ERR(pctrl->regs);
> +
> +	pctrl->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);

Why not use platform_get_irq()?

> +	if (pctrl->irq < 0) {
> +		dev_err(&pdev->dev, "No interrupt defined for msmgpio\n");
> +		return pctrl->irq;
> +	}
> +
> +	msm_pinctrl_desc.name = dev_name(&pdev->dev);
> +	msm_pinctrl_desc.pins = pctrl->soc->pins;
> +	msm_pinctrl_desc.npins = pctrl->soc->npins;
> +	pctrl->pctrl = pinctrl_register(&msm_pinctrl_desc, &pdev->dev, pctrl);
> +	if (!pctrl->pctrl) {
> +		dev_err(&pdev->dev, "Couldn't register pinctrl driver\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = msm_gpio_init(pctrl);
> +	if (ret) {
> +		pinctrl_unregister(pctrl->pctrl);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, pctrl);
> +
> +	dev_dbg(&pdev->dev, "Probed Qualcomm pinctrl driver\n");
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(msm_pinctrl_probe);
> +
> +int msm_pinctrl_remove(struct platform_device *pdev)
> +{
> +	struct msm_pinctrl *pctrl = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	irq_set_chained_handler(pctrl->irq, NULL);
> +	irq_domain_remove(pctrl->domain);
> +	ret = gpiochip_remove(&pctrl->chip);
> +	pinctrl_unregister(pctrl->pctrl);
> +
> +	return 0;

return ret?

> +}
> +EXPORT_SYMBOL(msm_pinctrl_remove);
> +
> diff --git a/drivers/pinctrl/pinctrl-msm.h b/drivers/pinctrl/pinctrl-msm.h
> new file mode 100644
> index 0000000..13b7463
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-msm.h
> @@ -0,0 +1,123 @@
> +/*
> + * Copyright (c) 2013, Sony Mobile Communications AB.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#ifndef __PINCTRL_MSM_H__
> +#define __PINCTRL_MSM_H__
> +
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/machine.h>

Are any of these includes actually necessary? Can't we just forward
declare struct pinctrl_pin_desc?

> +
>
> +struct msm_pingroup {
> +	const char *name;
> +	const unsigned *pins;
> +	unsigned npins;
> +
> +	unsigned funcs[8];
> +
> +	s16 ctl_reg;
> +	s16 io_reg;
> +	s16 intr_cfg_reg;
> +	s16 intr_status_reg;
> +	s16 intr_target_reg;

Why are these signed? Because the macros assign -1 to them? Perhaps make
them unsigned and make the macro assign ~0U?

> +
> +	unsigned mux_bit:5;
> +
> +	unsigned pull_bit:5;
> +	unsigned drv_bit:5;
> +
> +	unsigned oe_bit:5;
> +	unsigned in_bit:5;
> +	unsigned out_bit:5;
> +
> +	unsigned intr_enable_bit:5;
> +	unsigned intr_status_bit:5;
> +
> +	unsigned intr_target_bit:5;
> +	unsigned intr_raw_status_bit:5;
> +	unsigned intr_polarity_bit:5;
> +	unsigned intr_detection_bit:5;
> +	unsigned intr_detection_width:5;
> +};
> +
> +/**
> + * struct msm_pinctrl_soc_data - Qualcomm pin controller driver configuration
> + * @pins:       An array describing all pins the pin controller affects.
> + * @npins:      The number of entries in @pins.
> + * @functions:  An array describing all mux functions the SoC supports.
> + * @nfunctions: The number of entries in @functions.
> + * @groups:     An array describing all pin groups the pin SoC supports.
> + * @ngroups:    The numbmer of entries in @groups.
> + * @ngpio:      The number of pingroups the driver should expose as GPIOs.
> + */
> +struct msm_pinctrl_soc_data {
> +	const struct pinctrl_pin_desc *pins;
> +	unsigned npins;
> +	const struct msm_function *functions;
> +	unsigned nfunctions;
> +	const struct msm_pingroup *groups;
> +	unsigned ngroups;
> +	unsigned ngpios;
> +};
> +
> +int msm_pinctrl_probe(struct platform_device *pdev,
> +		      const struct msm_pinctrl_soc_data *soc_data);
> +int msm_pinctrl_remove(struct platform_device *pdev);
> +
> +#endif
> +


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v2 1/3] pinctrl: Add Qualcomm TLMM driver

[Bjorn Andersson]

On Fri 06 Dec 13:40 PST 2013, Stephen Boyd wrote:

> General nitpick: There seems to be a lot of checks for invalid input in
> the op functions. I hope that they're all unnecessary debugging that can
> be removed.
> 

Most of them checks that a gpio number is not larger than the number of pingroups.
This would be much cleaner to just replace with a validation in probe().

...
> >
> > +config PINCTRL_MSM
> > +     bool
> 
> Why not tristate?
> 

I have a hard time seeing an situation where you would like to build a system
with this driver as a module; it would force almost the entire system to be
loaded at a later time...

> > +/**
> > + * struct msm_pinctrl - state for a pinctrl-msm device
> > + * @dev:            device handle.
> > + * @pctrl:          pinctrl handle.
> > + * @domain:         irqdomain handle.
> > + * @chip:           gpiochip handle.
> > + * @irq:            parent irq for the TLMM irq_chip.
> > + * @lock:           Spinlock to protect register resources as well
> > + *                  as msm_pinctrl data structures.
> > + * @enabled_irqs:   Bitmap of currently enabled irqs.
> > + * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
> > + *                  detection.
> > + * @wake_irqs:      Bitmap of irqs with requested as wakeup source.
> > + * @soc;            Reference to soc_data of platform specific data.
> > + * @regs:           Base address for the TLMM register map.
> > + */
> > +struct msm_pinctrl {
> > +     struct device *dev;
> > +     struct pinctrl_dev *pctrl;
> > +     struct irq_domain *domain;
> > +     struct gpio_chip chip;
> > +     unsigned irq;
> > +
> > +     spinlock_t lock;
> > +
> > +     unsigned long *enabled_irqs;
> > +     unsigned long *dual_edge_irqs;
> > +     unsigned long *wake_irqs;
> 
> In the gpio driver we went with a static upper limit on the bitmap. That
> allowed us to avoid small allocations fragmenting the heap. Please do
> the same here (look at gpio-msm-v2.c).
> 

Sounds reasonable.

> > +static struct pinctrl_ops msm_pinctrl_ops = {
> 
> const?
> 

Of course.

> > +static struct pinmux_ops msm_pinmux_ops = {
> 
> const?
> 

Of course.

> > +static struct pinconf_ops msm_pinconf_ops = {
> 
> const?
> 

Of course.

> > +static int msm_gpio_direction_output(struct gpio_chip *chip, unsigned offset, int value)
> > +{
> > +     const struct msm_pingroup *g;
> > +     struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> > +     unsigned long flags;
> > +     u32 val;
> > +
> > +     if (WARN_ON(offset >= pctrl->soc->ngroups))
> > +             return -EINVAL;
> 
> How is this possible?
> 

If the soc specific ngpios is greater than ngroups this would happen. But it's much better
to catch that earlier!

> > +static void msm_gpio_dbg_show_one(struct seq_file *s,
> > +                               struct pinctrl_dev *pctldev,
> > +                               struct gpio_chip *chip,
> > +                               unsigned offset,
> > +                               unsigned gpio)
> > +{
> > +     const struct msm_pingroup *g;
> > +     struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> > +     unsigned func;
> > +     int is_out;
> > +     int drive;
> > +     int pull;
> > +     u32 ctl_reg;
> > +
> > +     const char *pulls[] = {
> 
> static const char * const ?
> 

Makes sense.
> > +             "no pull",
> > +             "pull down",
> > +             "keeper",
> > +             "pull up"
> > +     };
> > +
> > +     g = &pctrl->soc->groups[offset];
> > +     ctl_reg = readl(pctrl->regs + g->ctl_reg);
> > +
> > +     is_out = !!(ctl_reg & BIT(g->oe_bit));
> > +     func = (ctl_reg >> g->mux_bit) & 7;
> > +     drive = (ctl_reg >> g->drv_bit) & 7;
> > +     pull = (ctl_reg >> g->pull_bit) & 3;
> > +
> > +     seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" : "in", func);
> > +     seq_printf(s, " %dmA", msm_regval_to_drive[drive]);
> > +     seq_printf(s, " %s", pulls[pull]);
> > +}
> > +
> > +static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> > +{
> > +     unsigned gpio = chip->base;
> > +     unsigned i;
> > +
> > +     for (i = 0; i < chip->ngpio; i++, gpio++) {
> > +             msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
> > +             seq_printf(s, "\n");
> 
> seq_puts()?
> 

OK.

> > +static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> > +{
> > +     const struct msm_pingroup *g;
> > +     struct msm_pinctrl *pctrl;
> > +     unsigned long flags;
> > +     u32 val;
> > +
> > +     pctrl = irq_data_get_irq_chip_data(d);
> > +     if (!pctrl)
> > +             return -EINVAL;
> 
> How is this possible?
> 

As long as probe() is single threaded this should never be an issue, so I think
it makes sense to drop it.

> > +     val = readl(pctrl->regs + g->intr_cfg_reg);
> > +     val |= BIT(g->intr_raw_status_bit);
> > +     if (g->intr_detection_width == 2) {
> > +             val &= ~(3 << g->intr_detection_bit);
> > +             val &= ~(1 << g->intr_polarity_bit);
> > +             switch (type) {
> > +             case IRQ_TYPE_EDGE_RISING:
> > +                     val |= 1 << g->intr_detection_bit;
> > +                     val |= BIT(g->intr_polarity_bit);
> > +                     break;
> > +             case IRQ_TYPE_EDGE_FALLING:
> > +                     val |= 2 << g->intr_detection_bit;
> > +                     val |= BIT(g->intr_polarity_bit);
> > +                     break;
> > +             case IRQ_TYPE_EDGE_BOTH:
> > +                     val |= 3 << g->intr_detection_bit;
> > +                     val |= BIT(g->intr_polarity_bit);
> > +                     break;
> > +             case IRQ_TYPE_LEVEL_LOW:
> > +                     break;
> > +             case IRQ_TYPE_LEVEL_HIGH:
> > +                     val |= BIT(g->intr_polarity_bit);
> > +                     break;
> > +             }
> > +     } else if (g->intr_detection_width == 1) {
> > +             val &= ~(1 << g->intr_detection_bit);
> > +             val &= ~(1 << g->intr_polarity_bit);
> > +             switch (type) {
> > +             case IRQ_TYPE_EDGE_RISING:
> > +                     val |= BIT(g->intr_detection_bit);
> > +                     val |= BIT(g->intr_polarity_bit);
> > +                     break;
> > +             case IRQ_TYPE_EDGE_FALLING:
> > +                     val |= BIT(g->intr_detection_bit);
> > +                     break;
> > +             case IRQ_TYPE_EDGE_BOTH:
> > +                     val |= BIT(g->intr_detection_bit);
> > +                     break;
> > +             case IRQ_TYPE_LEVEL_LOW:
> > +                     break;
> > +             case IRQ_TYPE_LEVEL_HIGH:
> > +                     val |= BIT(g->intr_polarity_bit);
> > +                     break;
> > +             }
> > +     } else {
> > +             BUG();
> > +     }
> 
> This would be better written as a collection of ifs so that
> IRQ_TYPE_EDGE_BOTH doesn't have to be tested and we duplicate less code.
> 

I've rewritten this numerous times and this is the cleanest way I can find
to do this in. Yes, there's some duplication but it has a cleaner structure
and easier to follow than the nested if/elseif/else statements.

So I would like to keep it as is.

> > +     /*
> > +      * Each pin have it's own IRQ status register, so use
> 
> s/have/has/
> 

Thanks.

> > +     /* No interrutps where flagged */
> 
> s/where/were/
> s/interrutps/interrupts/

Thanks.

> > +     pctrl->enabled_irqs = devm_kzalloc(pctrl->dev,
> > +                                        sizeof(unsigned long) * BITS_TO_LONGS(chip->ngpio),
> > +                                        GFP_KERNEL);
> 
> If you don't agree with how gpio-msm-v2.c does it, please use
> devm_kcalloc().
> 

If we're to cause churn I think it's better to go with your suggested
approach.

> > +     pctrl->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> 
> Why not use platform_get_irq()?
> 

I just followed suit, but as I have *pdev here there's no reason to call
irq_of_parse_and_map yet again.

> > +int msm_pinctrl_remove(struct platform_device *pdev)
> > +{
> > +     struct msm_pinctrl *pctrl = platform_get_drvdata(pdev);
> > +     int ret;
> > +
> > +     irq_set_chained_handler(pctrl->irq, NULL);
> > +     irq_domain_remove(pctrl->domain);
> > +     ret = gpiochip_remove(&pctrl->chip);
> > +     pinctrl_unregister(pctrl->pctrl);
> > +
> > +     return 0;
> 
> return ret?
> 

I was intentionally ignoring the return value of gpiochip_remove, but that's of
course incorrect. I'll restructure this to make sense, and care about
gpiochip_remove returning e.g EBUSY.

> > +#include <linux/pinctrl/pinctrl.h>
> > +#include <linux/pinctrl/pinmux.h>
> > +#include <linux/pinctrl/pinconf.h>
> > +#include <linux/pinctrl/machine.h>
> 
> Are any of these includes actually necessary? Can't we just forward
> declare struct pinctrl_pin_desc?
> 

None of them are needed in the current set of patches, as these are already
included in the c-files before including this.

But the right set should be: platform_device.h and pinctrl.h.

> > +
> >
> > +struct msm_pingroup {
> > +     const char *name;
> > +     const unsigned *pins;
> > +     unsigned npins;
> > +
> > +     unsigned funcs[8];
> > +
> > +     s16 ctl_reg;
> > +     s16 io_reg;
> > +     s16 intr_cfg_reg;
> > +     s16 intr_status_reg;
> > +     s16 intr_target_reg;
> 
> Why are these signed? Because the macros assign -1 to them? Perhaps make
> them unsigned and make the macro assign ~0U?
> 

Only reason is that I prefer to have invalid values falling outside the
possibly valid memory range.


Thanks for you review Stephen.

Regards,
Bjorn

[PATCH v2 1/3] pinctrl: Add Qualcomm TLMM driver

[Stephen Boyd]

On 12/10/13 00:10, Bjorn Andersson wrote:
> On Fri 06 Dec 13:40 PST 2013, Stephen Boyd wrote:
>>> +config PINCTRL_MSM
>>> +     bool
>> Why not tristate?
>>
> I have a hard time seeing an situation where you would like to build a system
> with this driver as a module; it would force almost the entire system to be
> loaded at a later time...

We're going to need to build everything but the essentials as modules
for the multi-platform kernel because we're going to run into the
problem where the kernel can't link anymore. Data heavy drivers such as
this are  targets for that because they waste more space being built-in
and they're not absolutely necessary to boot into an initrd that holds
the modules for a particular device.

>
>
>>> +     val = readl(pctrl->regs + g->intr_cfg_reg);
>>> +     val |= BIT(g->intr_raw_status_bit);
>>> +     if (g->intr_detection_width == 2) {
>>> +             val &= ~(3 << g->intr_detection_bit);
>>> +             val &= ~(1 << g->intr_polarity_bit);
>>> +             switch (type) {
>>> +             case IRQ_TYPE_EDGE_RISING:
>>> +                     val |= 1 << g->intr_detection_bit;
>>> +                     val |= BIT(g->intr_polarity_bit);
>>> +                     break;
>>> +             case IRQ_TYPE_EDGE_FALLING:
>>> +                     val |= 2 << g->intr_detection_bit;
>>> +                     val |= BIT(g->intr_polarity_bit);
>>> +                     break;
>>> +             case IRQ_TYPE_EDGE_BOTH:
>>> +                     val |= 3 << g->intr_detection_bit;
>>> +                     val |= BIT(g->intr_polarity_bit);
>>> +                     break;
>>> +             case IRQ_TYPE_LEVEL_LOW:
>>> +                     break;
>>> +             case IRQ_TYPE_LEVEL_HIGH:
>>> +                     val |= BIT(g->intr_polarity_bit);
>>> +                     break;
>>> +             }
>>> +     } else if (g->intr_detection_width == 1) {
>>> +             val &= ~(1 << g->intr_detection_bit);
>>> +             val &= ~(1 << g->intr_polarity_bit);
>>> +             switch (type) {
>>> +             case IRQ_TYPE_EDGE_RISING:
>>> +                     val |= BIT(g->intr_detection_bit);
>>> +                     val |= BIT(g->intr_polarity_bit);
>>> +                     break;
>>> +             case IRQ_TYPE_EDGE_FALLING:
>>> +                     val |= BIT(g->intr_detection_bit);
>>> +                     break;
>>> +             case IRQ_TYPE_EDGE_BOTH:
>>> +                     val |= BIT(g->intr_detection_bit);
>>> +                     break;
>>> +             case IRQ_TYPE_LEVEL_LOW:
>>> +                     break;
>>> +             case IRQ_TYPE_LEVEL_HIGH:
>>> +                     val |= BIT(g->intr_polarity_bit);
>>> +                     break;
>>> +             }
>>> +     } else {
>>> +             BUG();
>>> +     }
>> This would be better written as a collection of ifs so that
>> IRQ_TYPE_EDGE_BOTH doesn't have to be tested and we duplicate less code.
>>
> I've rewritten this numerous times and this is the cleanest way I can find
> to do this in. Yes, there's some duplication but it has a cleaner structure
> and easier to follow than the nested if/elseif/else statements.
>
> So I would like to keep it as is.

Isn't this the same?

+     val = readl(pctrl->regs + g->intr_cfg_reg);
+     val |= BIT(g->intr_raw_status_bit);
+
+     detection_mask = BIT(g->intr_detection_width) - 1;
+     val &= ~(detection_mask << g->intr_detection_bit);
+     val &= ~BIT(g->intr_polarity_bit);
+
+     if (type & IRQ_TYPE_EDGE_RISING)
+             val |= BIT(g->intr_detection_bit);
+
+     if (type & IRQ_TYPE_EDGE_FALLING)
+             val |= g->intr_detection_width << g->intr_detection_bit;
+
+     if (!(type & IRQ_TYPE_LEVEL_LOW))
+             val |= BIT(g->intr_polarity_bit);
+

>
>
>
>>> +#include <linux/pinctrl/pinctrl.h>
>>> +#include <linux/pinctrl/pinmux.h>
>>> +#include <linux/pinctrl/pinconf.h>
>>> +#include <linux/pinctrl/machine.h>
>> Are any of these includes actually necessary? Can't we just forward
>> declare struct pinctrl_pin_desc?
>>
> None of them are needed in the current set of patches, as these are already
> included in the c-files before including this.
>
> But the right set should be: platform_device.h and pinctrl.h.

We should be able to get away with forward declaring the structs we care
about. C files that include this header should be including the
pinctrl.h header file anyway, no?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v2 1/3] pinctrl: Add Qualcomm TLMM driver

[Linus Walleij]

On Wed, Dec 11, 2013 at 2:42 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 12/10/13 00:10, Bjorn Andersson wrote:
>> On Fri 06 Dec 13:40 PST 2013, Stephen Boyd wrote:
>>>> +config PINCTRL_MSM
>>>> +     bool
>>> Why not tristate?
>>>
>> I have a hard time seeing an situation where you would like to build a system
>> with this driver as a module; it would force almost the entire system to be
>> loaded at a later time...
>
> We're going to need to build everything but the essentials as modules
> for the multi-platform kernel because we're going to run into the
> problem where the kernel can't link anymore. Data heavy drivers such as
> this are  targets for that because they waste more space being built-in
> and they're not absolutely necessary to boot into an initrd that holds
> the modules for a particular device.

This is a quite generic problem rather than a pin control problem.

We're actually going to have to compile quite a few drivers into the
kernel as well, like all irqchips and all clock sources...

I think we have toyed with the idea of tagging code and data so
that it will be discarded on non-matched platforms, I have no
idea how that would look in practice :-(

There are quite a few pin control drivers compiled into any
present multiplatform kernels, so I wouldn't say that one
more or less hurts us today.

>>>> +#include <linux/pinctrl/pinctrl.h>
>>>> +#include <linux/pinctrl/pinmux.h>
>>>> +#include <linux/pinctrl/pinconf.h>
>>>> +#include <linux/pinctrl/machine.h>
>>> Are any of these includes actually necessary? Can't we just forward
>>> declare struct pinctrl_pin_desc?
>>>
>> None of them are needed in the current set of patches, as these are already
>> included in the c-files before including this.
>>
>> But the right set should be: platform_device.h and pinctrl.h.
>
> We should be able to get away with forward declaring the structs we care
> about. C files that include this header should be including the
> pinctrl.h header file anyway, no?

Patches accepted...

Yours,
Linus Walleij

[PATCH v2 1/3] pinctrl: Add Qualcomm TLMM driver

[Timur Tabi]

On Thu, Dec 5, 2013 at 8:10 PM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
>
> +static int msm_gpio_init(struct msm_pinctrl *pctrl)
> +{
> +       struct gpio_chip *chip;
> +       int irq;
> +       int ret;
> +       int i;
> +       int r;
> +
> +       chip = &pctrl->chip;
> +       chip->base = 0;
> +       chip->ngpio = pctrl->soc->ngpios;

I know this patch is a year old, but I'm wondering if this line is correct.

The original version of your patch from 11/23/13 said this:

+       chip->ngpio = pctrl->soc->gpio_range->npins;

and today, the line is this:

    unsigned ngpio = pctrl->soc->ngpios;

I'm wondering if this line should be instead:

    unsigned ngpio = pctrl->soc->npins;

I'm confused about the difference between msm_pinctrl_soc_data.npins
and msm_pinctrl_soc_data.ngpios.  Variable "ngpio" is used by
gpiochip_add(), so I think it's not concerned with pin control.
msm_pinctrl_soc_data.npins appears to be the number of GPIOs, whereas
msm_pinctrl_soc_data.ngpios appears to be the number of pin groups.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH v2 1/3] pinctrl: Add Qualcomm TLMM driver

[Bjorn Andersson]

On Tue, Nov 25, 2014 at 11:55 AM, Timur Tabi <timur@codeaurora.org> wrote:
> On Thu, Dec 5, 2013 at 8:10 PM, Bjorn Andersson
> <bjorn.andersson@sonymobile.com> wrote:
>>
>> +static int msm_gpio_init(struct msm_pinctrl *pctrl)
>> +{
>> +       struct gpio_chip *chip;
>> +       int irq;
>> +       int ret;
>> +       int i;
>> +       int r;
>> +
>> +       chip = &pctrl->chip;
>> +       chip->base = 0;
>> +       chip->ngpio = pctrl->soc->ngpios;
>
> I know this patch is a year old, but I'm wondering if this line is correct.
>

Hi Timur,

It's always good to review old code, so don't worry about its age of
the patch or code.

> The original version of your patch from 11/23/13 said this:
>
> +       chip->ngpio = pctrl->soc->gpio_range->npins;
>

If you look in patchset 2 (the addition of 8974) you can see that I
passed a pinctrl_gpio_range from the 8974 driver to the common driver;
but I only use this to pass the number of gpios (NUM_GPIO_PINGROUPS).

> and today, the line is this:
>
>     unsigned ngpio = pctrl->soc->ngpios;
>

Most likely based on some review comments this was replaced with just
an unsigned 'ngpios'.

> I'm wondering if this line should be instead:
>
>     unsigned ngpio = pctrl->soc->npins;
>
> I'm confused about the difference between msm_pinctrl_soc_data.npins
> and msm_pinctrl_soc_data.ngpios.  Variable "ngpio" is used by
> gpiochip_add(), so I think it's not concerned with pin control.
> msm_pinctrl_soc_data.npins appears to be the number of GPIOs, whereas
> msm_pinctrl_soc_data.ngpios appears to be the number of pin groups.
>

The 'ngpio' specifies how many gpio pins/groups (they are 1:1 in the
qcom case) the tlmm block sports, while 'npins' specifies how many
pingroups can be controlled by pinctrl/pinconf/pinmux.

So 'npins' will be 'ngpio' plus the other things that can be
controlled, e.g. sdcc.


The original patch assigns ngpio to be "the number of pinctrl pins in
the gpio range", i.e. a subset of all pins.

Regards,
Bjorn