diff mbox series

[1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander

Message ID 20210109140204.151340-2-angelogioacchino.delregno@somainline.org
State Superseded
Headers show
Series Add Awinic AW9523(B) I2C GPIO Expander driver | expand

Commit Message

AngeloGioacchino Del Regno Jan. 9, 2021, 2:02 p.m. UTC
The Awinic AW9523(B) is a multi-function I2C gpio expander in a
TQFN-24L package, featuring PWM (max 37mA per pin, or total max
power 3.2Watts) for LED driving capability.

It has two ports with 8 pins per port (for a total of 16 pins),
configurable as either PWM with 1/256 stepping or GPIO input/output,
1.8V logic input; each GPIO can be configured as input or output
independently from each other.

This IC also has an internal interrupt controller, which is capable
of generating an interrupt for each GPIO, depending on the
configuration, and will raise an interrupt on the INTN pin to
advertise this to an external interrupt controller.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 drivers/pinctrl/Kconfig          |   17 +
 drivers/pinctrl/Makefile         |    1 +
 drivers/pinctrl/pinctrl-aw9523.c | 1070 ++++++++++++++++++++++++++++++
 3 files changed, 1088 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-aw9523.c

Comments

kernel test robot Jan. 9, 2021, 5:24 p.m. UTC | #1
Hi AngeloGioacchino,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pinctrl/devel]
[also build test ERROR on v5.11-rc2 next-20210108]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/AngeloGioacchino-Del-Regno/Add-Awinic-AW9523-B-I2C-GPIO-Expander-driver/20210109-220525
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: sparc-randconfig-r025-20210110 (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/9cf5980cd08c30aec01a24e284a0553396a1fa64
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review AngeloGioacchino-Del-Regno/Add-Awinic-AW9523-B-I2C-GPIO-Expander-driver/20210109-220525
        git checkout 9cf5980cd08c30aec01a24e284a0553396a1fa64
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sparc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/pinctrl/pinctrl-aw9523.c: In function 'aw9523_pconf_get':
   drivers/pinctrl/pinctrl-aw9523.c:292:11: error: implicit declaration of function 'FIELD_GET'; did you mean 'FOLL_GET'? [-Werror=implicit-function-declaration]
     292 |    val = !FIELD_GET(AW9523_GCR_GPOMD_MASK, val);
         |           ^~~~~~~~~
         |           FOLL_GET
   drivers/pinctrl/pinctrl-aw9523.c: In function 'aw9523_init_irq':
>> drivers/pinctrl/pinctrl-aw9523.c:880:9: error: 'struct gpio_irq_chip' has no member named 'parent_domain'
     880 |  gpioirq->parent_domain = NULL;
         |         ^~
   cc1: some warnings being treated as errors

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for FRAME_POINTER
   Depends on DEBUG_KERNEL && (M68K || UML || SUPERH) || ARCH_WANT_FRAME_POINTERS || MCOUNT
   Selected by
   - LOCKDEP && DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT && !MIPS && !PPC && !ARM && !S390 && !MICROBLAZE && !ARC && !X86
   - FAULT_INJECTION_STACKTRACE_FILTER && FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT && !X86_64 && !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM && !ARC && !X86


vim +880 drivers/pinctrl/pinctrl-aw9523.c

   832	
   833	static int aw9523_init_irq(struct aw9523 *awi, int irq)
   834	{
   835		struct device *dev = awi->dev;
   836		struct gpio_irq_chip *gpioirq;
   837		struct irq_chip *irqchip;
   838		int i, ret;
   839	
   840		if (!device_property_read_bool(dev, "interrupt-controller"))
   841			return 0;
   842	
   843		irqchip = devm_kzalloc(dev, sizeof(*irqchip), GFP_KERNEL);
   844		if (!irqchip)
   845			return -ENOMEM;
   846	
   847		awi->irq = devm_kzalloc(dev, sizeof(*awi->irq), GFP_KERNEL);
   848		if (!awi->irq)
   849			return -ENOMEM;
   850	
   851		irqchip->name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
   852		if (!irqchip->name)
   853			return -ENOMEM;
   854	
   855		irqchip->irq_mask = aw9523_irq_mask;
   856		irqchip->irq_unmask = aw9523_irq_unmask;
   857		irqchip->irq_bus_lock = aw9523_irq_bus_lock;
   858		irqchip->irq_bus_sync_unlock = aw9523_irq_bus_sync_unlock;
   859		irqchip->irq_set_type = aw9523_gpio_irq_type;
   860	
   861		for (i = 0; i < AW9523_NUM_PORTS; i++) {
   862			bitmap_fill(awi->irq->masked[i], AW9523_PINS_PER_PORT);
   863			bitmap_fill(awi->irq->old_masked[i], AW9523_PINS_PER_PORT);
   864		}
   865		awi->irq->irqchip = irqchip;
   866		mutex_init(&awi->irq->lock);
   867	
   868		ret = devm_request_threaded_irq(dev, irq, NULL, aw9523_irq_thread_func,
   869						IRQF_ONESHOT, dev_name(dev), awi);
   870		if (ret) {
   871			dev_err(dev, "Failed to request irq %d\n", irq);
   872			return ret;
   873		}
   874	
   875		gpioirq = &awi->gpio.irq;
   876		gpioirq->chip = irqchip;
   877		gpioirq->parent_handler = NULL;
   878		gpioirq->num_parents = 0;
   879		gpioirq->parents = NULL;
 > 880		gpioirq->parent_domain = NULL;
   881		gpioirq->default_type = IRQ_TYPE_NONE;
   882		gpioirq->handler = handle_simple_irq;
   883		gpioirq->threaded = true;
   884		gpioirq->first = 0;
   885	
   886		return 0;
   887	}
   888	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Linus Walleij Jan. 9, 2021, 10:12 p.m. UTC | #2
On Sat, Jan 9, 2021 at 6:24 PM kernel test robot <lkp@intel.com> wrote:

>  > 880          gpioirq->parent_domain = NULL;

The autobuilder is complaining because your irq chip is not
hierarchical and this is only used for hierarchical irqchips.
I think you can just delete this line.

Yours,
Linus Walleij
Linus Walleij Jan. 10, 2021, 12:24 a.m. UTC | #3
On Sun, Jan 10, 2021 at 12:11 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
> Il 09/01/21 23:11, Linus Walleij ha scritto:

> > The major review comment is that it'd be nice if you look into
> > using regmaps register cache instead of rolling your own,
> > and also possibly using regmaps locking rather than your own
> > as a result of that.
> >
> Actually, I really tried to use regmap's FLAT register cache and after
> many, many tries... I had to give up. I just couldn't get it working. :(

This needs to be root-caused. The register cache in regmap is for
using not for decoration.

What is the problems you are seeing?

If it is fundamentally so that regmap has limitations that is one thing,
but I want to rule out that we're just not using it wrong or that there
is a bug in it that we should fix.

Looping in Mark Brown the regmap maintainer.

Yours,
Linus Walleij
AngeloGioacchino Del Regno Jan. 10, 2021, 2:32 p.m. UTC | #4
Il 10/01/21 01:24, Linus Walleij ha scritto:
> On Sun, Jan 10, 2021 at 12:11 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org> wrote:
>> Il 09/01/21 23:11, Linus Walleij ha scritto:
> 
>>> The major review comment is that it'd be nice if you look into
>>> using regmaps register cache instead of rolling your own,
>>> and also possibly using regmaps locking rather than your own
>>> as a result of that.
>>>
>> Actually, I really tried to use regmap's FLAT register cache and after
>> many, many tries... I had to give up. I just couldn't get it working. :(
> 
> This needs to be root-caused. The register cache in regmap is for
> using not for decoration.
> 
> What is the problems you are seeing?
> 
> If it is fundamentally so that regmap has limitations that is one thing,
> but I want to rule out that we're just not using it wrong or that there
> is a bug in it that we should fix.
> 
> Looping in Mark Brown the regmap maintainer.
> 
> Yours,
> Linus Walleij
> 

Oh don't misunderstand me, I would really be happy to make use of the 
regmap register cache, it's a nice one.

So, I've retried some basic usage of the regcache, relevant snippets here:
static bool aw9523_volatile_reg(struct device *dev, unsigned int reg)

{

	return reg == AW9523_REG_IN_STATE(0) ||

	       reg == AW9523_REG_IN_STATE(AW9523_PINS_PER_PORT) ||

	       reg == AW9523_REG_CHIPID;

}

static const struct regmap_config aw9523_regmap = {

....
	.volatile_reg = aw9523_volatile_reg,


	.cache_type = REGCACHE_FLAT,

....

}

Since REG_IN_STATE is used to read the GPIO input level, it's not 
cacheable, then CHIPID was set as not cacheable for safety: that may be 
avoided, but that may make no sense.. since it's a one-time readout for 
init putposes, it'd be useless to keep it cached.

Then, the set_bit/clear_bit in aw9523_irq_mask(), aw9523_irq_unmask were 
replaced with calls to regmap_update_bits_async, example:

	regmap_update_bits_async(awi->regmap,

				 AW9523_REG_INTR_DIS(d->hwirq),

				 BIT(n), BIT(n));



Where of course the value is either BIT(n) or 0 for mask and unmask 
respectively.
Also, the bus_sync_unlock callback was changed as follows:

static void aw9523_irq_mask(struct irq_data *d)

{

	struct aw9523 *awi =
		gpiochip_get_data(irq_data_get_irq_chip_data(d));

	unsigned int n = d->hwirq % AW9523_PINS_PER_PORT;



	regmap_update_bits_async(awi->regmap,

				 AW9523_REG_INTR_DIS(d->hwirq),

				 BIT(n), BIT(n));

}

One of the biggest / oddest issues that I get when trying to use 
regcache is that I'm getting badbadbad scheduling while atomic warnings 
all over and I don't get why, since regcache_default_sync is just 
calling _regmap_write, which is exactly what (non _prefix) regmap_write 
also calls...

As a reference, this is one out of "many" (as you can imagine) stacktraces:

<3>[    1.061428] BUG: scheduling while atomic: kworker/3:1/119/0x00000000

<4>[    1.061575] Modules linked in:

<4>[    1.061716] CPU: 3 PID: 119 Comm: kworker/3:1 Tainted: G        W 
         5.10.0-rc3-31949-ge1680e3101bc-dirty #1162

<4>[    1.061956] Hardware name: F(x)tec Pro1 (QX1000) (DT)

<4>[    1.062081] Workqueue: events deferred_probe_work_func

<4>[    1.062205] Call trace:

<4>[    1.062333]  dump_backtrace+0x0/0x1e0

<4>[    1.062439]  show_stack+0x14/0x60

<4>[    1.062539]  dump_stack+0xd4/0x12c

<4>[    1.062680]  __schedule_bug+0x50/0x70

<4>[    1.062785]  __schedule+0x618/0x650

<4>[    1.062888]  schedule+0x6c/0xf8

<4>[    1.062985]  schedule_timeout+0x1d0/0x260

<4>[    1.063134]  wait_for_completion_timeout+0x8c/0x110

<4>[    1.063257]  qup_i2c_wait_for_complete.isra.18+0x1c/0x80

<4>[    1.063429]  qup_i2c_xfer_v2_msg+0x2d4/0x3f0

<4>[    1.063543]  qup_i2c_xfer_v2+0x290/0xa28

<4>[    1.063652]  __i2c_transfer+0x16c/0x380

<4>[    1.063798]  i2c_transfer+0x5c/0x138

<4>[    1.063903]  i2c_transfer_buffer_flags+0x58/0x80

<4>[    1.064060]  regmap_i2c_write+0x1c/0x50

<4>[    1.064168]  _regmap_raw_write_impl+0x35c/0x688

<4>[    1.064285]  _regmap_bus_raw_write+0x64/0x80

<4>[    1.064440]  _regmap_write+0x58/0xa8

<4>[    1.064545]  regcache_default_sync+0xcc/0x1a0

<4>[    1.064660]  regcache_sync_region+0xdc/0xe8

<4>[    1.064811]  aw9523_irq_bus_sync_unlock+0x30/0x48

<4>[    1.064931]  __setup_irq+0x798/0x890

<4>[    1.065034]  request_threaded_irq+0xe0/0x198

<4>[    1.065188]  devm_request_threaded_irq+0x78/0xf8

<4>[    1.065311]  gpio_keyboard_probe+0x2a8/0x468

<4>[    1.065465]  platform_drv_probe+0x50/0xa0

<4>[    1.065576]  really_probe+0x290/0x4e8

<4>[    1.065682]  driver_probe_device+0xf4/0x160

<4>[    1.065834]  __device_attach_driver+0x98/0x110

<4>[    1.065950]  bus_for_each_drv+0x64/0xc8

<4>[    1.066063]  __device_attach+0xe4/0x168

<4>[    1.066211]  device_initial_probe+0x10/0x18

<4>[    1.066325]  bus_probe_device+0x90/0x98

<4>[    1.066434]  deferred_probe_work_func+0x88/0xe0

<4>[    1.066591]  process_one_work+0x1e4/0x358

<4>[    1.066702]  worker_thread+0x208/0x478

<4>[    1.073273]  kthread+0x14c/0x150

<4>[    1.079858]  ret_from_fork+0x10/0x18



P.S.: Infinite thanks for being so nice and helpful!
AngeloGioacchino Del Regno Jan. 10, 2021, 2:56 p.m. UTC | #5
Il 10/01/21 15:32, AngeloGioacchino Del Regno ha scritto:
> Il 10/01/21 01:24, Linus Walleij ha scritto:
>> On Sun, Jan 10, 2021 at 12:11 AM AngeloGioacchino Del Regno
>> <angelogioacchino.delregno@somainline.org> wrote:
>>> Il 09/01/21 23:11, Linus Walleij ha scritto:
>>
>>>> The major review comment is that it'd be nice if you look into
>>>> using regmaps register cache instead of rolling your own,
>>>> and also possibly using regmaps locking rather than your own
>>>> as a result of that.
>>>>
>>> Actually, I really tried to use regmap's FLAT register cache and after
>>> many, many tries... I had to give up. I just couldn't get it working. :(
>>
>> This needs to be root-caused. The register cache in regmap is for
>> using not for decoration.
>>
>> What is the problems you are seeing?
>>
>> If it is fundamentally so that regmap has limitations that is one thing,
>> but I want to rule out that we're just not using it wrong or that there
>> is a bug in it that we should fix.
>>
>> Looping in Mark Brown the regmap maintainer.
>>
>> Yours,
>> Linus Walleij
>>
> 
> Oh don't misunderstand me, I would really be happy to make use of the 
> regmap register cache, it's a nice one.
> 
> So, I've retried some basic usage of the regcache, relevant snippets here:
> static bool aw9523_volatile_reg(struct device *dev, unsigned int reg)
> 
> {
> 
>      return reg == AW9523_REG_IN_STATE(0) ||
> 
>             reg == AW9523_REG_IN_STATE(AW9523_PINS_PER_PORT) ||
> 
>             reg == AW9523_REG_CHIPID;
> 
> }
> 
> static const struct regmap_config aw9523_regmap = {
> 
> ....
>      .volatile_reg = aw9523_volatile_reg,
> 
> 
>      .cache_type = REGCACHE_FLAT,
> 
> ....
> 
> }
> 
> Since REG_IN_STATE is used to read the GPIO input level, it's not 
> cacheable, then CHIPID was set as not cacheable for safety: that may be 
> avoided, but that may make no sense.. since it's a one-time readout for 
> init putposes, it'd be useless to keep it cached.
> 
> Then, the set_bit/clear_bit in aw9523_irq_mask(), aw9523_irq_unmask were 
> replaced with calls to regmap_update_bits_async, example:
> 
>      regmap_update_bits_async(awi->regmap,
> 
>                   AW9523_REG_INTR_DIS(d->hwirq),
> 
>                   BIT(n), BIT(n));
> 
> 
> 
> Where of course the value is either BIT(n) or 0 for mask and unmask 
> respectively.
> Also, the bus_sync_unlock callback was changed as follows:
> 
> static void aw9523_irq_mask(struct irq_data *d)
> 
> {
> 
>      struct aw9523 *awi =
>          gpiochip_get_data(irq_data_get_irq_chip_data(d));
> 
>      unsigned int n = d->hwirq % AW9523_PINS_PER_PORT;
> 
> 
> 
>      regmap_update_bits_async(awi->regmap,
> 
>                   AW9523_REG_INTR_DIS(d->hwirq),
> 
>                   BIT(n), BIT(n));
> 
> }
Sorry, wrong paste! This is the right one:
static void aw9523_irq_bus_sync_unlock(struct irq_data *d)

{

	struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));



	regcache_mark_dirty(awi->regmap);

	regcache_sync_region(awi->regmap, AW9523_REG_INTR_DIS(0),

			     AW9523_REG_INTR_DIS(AW9523_PINS_PER_PORT));

	mutex_unlock(&awi->irq->lock);

}

> 
> One of the biggest / oddest issues that I get when trying to use 
> regcache is that I'm getting badbadbad scheduling while atomic warnings 
> all over and I don't get why, since regcache_default_sync is just 
> calling _regmap_write, which is exactly what (non _prefix) regmap_write 
> also calls...
> 
> As a reference, this is one out of "many" (as you can imagine) stacktraces:
> 
> <3>[    1.061428] BUG: scheduling while atomic: kworker/3:1/119/0x00000000
> 
> <4>[    1.061575] Modules linked in:
> 
> <4>[    1.061716] CPU: 3 PID: 119 Comm: kworker/3:1 Tainted: G        W 
>          5.10.0-rc3-31949-ge1680e3101bc-dirty #1162
> 
> <4>[    1.061956] Hardware name: F(x)tec Pro1 (QX1000) (DT)
> 
> <4>[    1.062081] Workqueue: events deferred_probe_work_func
> 
> <4>[    1.062205] Call trace:
> 
> <4>[    1.062333]  dump_backtrace+0x0/0x1e0
> 
> <4>[    1.062439]  show_stack+0x14/0x60
> 
> <4>[    1.062539]  dump_stack+0xd4/0x12c
> 
> <4>[    1.062680]  __schedule_bug+0x50/0x70
> 
> <4>[    1.062785]  __schedule+0x618/0x650
> 
> <4>[    1.062888]  schedule+0x6c/0xf8
> 
> <4>[    1.062985]  schedule_timeout+0x1d0/0x260
> 
> <4>[    1.063134]  wait_for_completion_timeout+0x8c/0x110
> 
> <4>[    1.063257]  qup_i2c_wait_for_complete.isra.18+0x1c/0x80
> 
> <4>[    1.063429]  qup_i2c_xfer_v2_msg+0x2d4/0x3f0
> 
> <4>[    1.063543]  qup_i2c_xfer_v2+0x290/0xa28
> 
> <4>[    1.063652]  __i2c_transfer+0x16c/0x380
> 
> <4>[    1.063798]  i2c_transfer+0x5c/0x138
> 
> <4>[    1.063903]  i2c_transfer_buffer_flags+0x58/0x80
> 
> <4>[    1.064060]  regmap_i2c_write+0x1c/0x50
> 
> <4>[    1.064168]  _regmap_raw_write_impl+0x35c/0x688
> 
> <4>[    1.064285]  _regmap_bus_raw_write+0x64/0x80
> 
> <4>[    1.064440]  _regmap_write+0x58/0xa8
> 
> <4>[    1.064545]  regcache_default_sync+0xcc/0x1a0
> 
> <4>[    1.064660]  regcache_sync_region+0xdc/0xe8
> 
> <4>[    1.064811]  aw9523_irq_bus_sync_unlock+0x30/0x48
> 
> <4>[    1.064931]  __setup_irq+0x798/0x890
> 
> <4>[    1.065034]  request_threaded_irq+0xe0/0x198
> 
> <4>[    1.065188]  devm_request_threaded_irq+0x78/0xf8
> 
> <4>[    1.065311]  gpio_keyboard_probe+0x2a8/0x468
> 
> <4>[    1.065465]  platform_drv_probe+0x50/0xa0
> 
> <4>[    1.065576]  really_probe+0x290/0x4e8
> 
> <4>[    1.065682]  driver_probe_device+0xf4/0x160
> 
> <4>[    1.065834]  __device_attach_driver+0x98/0x110
> 
> <4>[    1.065950]  bus_for_each_drv+0x64/0xc8
> 
> <4>[    1.066063]  __device_attach+0xe4/0x168
> 
> <4>[    1.066211]  device_initial_probe+0x10/0x18
> 
> <4>[    1.066325]  bus_probe_device+0x90/0x98
> 
> <4>[    1.066434]  deferred_probe_work_func+0x88/0xe0
> 
> <4>[    1.066591]  process_one_work+0x1e4/0x358
> 
> <4>[    1.066702]  worker_thread+0x208/0x478
> 
> <4>[    1.073273]  kthread+0x14c/0x150
> 
> <4>[    1.079858]  ret_from_fork+0x10/0x18
> 
> 
> 
> P.S.: Infinite thanks for being so nice and helpful!
Linus Walleij Jan. 10, 2021, 7:35 p.m. UTC | #6
On Sun, Jan 10, 2021 at 3:32 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:

> So, I've retried some basic usage of the regcache, relevant snippets here:
> static bool aw9523_volatile_reg(struct device *dev, unsigned int reg)
> {
>
>         return reg == AW9523_REG_IN_STATE(0) ||
>                reg == AW9523_REG_IN_STATE(AW9523_PINS_PER_PORT) ||
>                reg == AW9523_REG_CHIPID;
> }
(...)
> Since REG_IN_STATE is used to read the GPIO input level, it's not
> cacheable,

Fair enough.

> then CHIPID was set as not cacheable for safety: that may be
> avoided, but that may make no sense.. since it's a one-time readout for
> init putposes, it'd be useless to keep it cached.

I guess.

> Then, the set_bit/clear_bit in aw9523_irq_mask(), aw9523_irq_unmask were
> replaced with calls to regmap_update_bits_async, example:
>
>         regmap_update_bits_async(awi->regmap,
>                                  AW9523_REG_INTR_DIS(d->hwirq),
>                                  BIT(n), BIT(n));
>
> Where of course the value is either BIT(n) or 0 for mask and unmask
> respectively.
> Also, the bus_sync_unlock callback was changed as follows:
>
> static void aw9523_irq_bus_sync_unlock(struct irq_data *d)
>
> {
>       struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
>       regcache_mark_dirty(awi->regmap);
>       regcache_sync_region(awi->regmap, AW9523_REG_INTR_DIS(0),
>                            AW9523_REG_INTR_DIS(AW9523_PINS_PER_PORT));
>        mutex_unlock(&awi->irq->lock);
(...)
> One of the biggest / oddest issues that I get when trying to use
> regcache is that I'm getting badbadbad scheduling while atomic warnings
> all over and I don't get why, since regcache_default_sync is just
> calling _regmap_write, which is exactly what (non _prefix) regmap_write
> also calls...

OK that is the real problem to solve then.

> As a reference, this is one out of "many" (as you can imagine) stacktraces:
>
> <3>[    1.061428] BUG: scheduling while atomic: kworker/3:1/119/0x00000000
(...)
> <4>[    1.063134]  wait_for_completion_timeout+0x8c/0x110
> <4>[    1.063257]  qup_i2c_wait_for_complete.isra.18+0x1c/0x80
> <4>[    1.063429]  qup_i2c_xfer_v2_msg+0x2d4/0x3f0
> <4>[    1.063543]  qup_i2c_xfer_v2+0x290/0xa28
> <4>[    1.063652]  __i2c_transfer+0x16c/0x380
> <4>[    1.063798]  i2c_transfer+0x5c/0x138
> <4>[    1.063903]  i2c_transfer_buffer_flags+0x58/0x80
> <4>[    1.064060]  regmap_i2c_write+0x1c/0x50
> <4>[    1.064168]  _regmap_raw_write_impl+0x35c/0x688
> <4>[    1.064285]  _regmap_bus_raw_write+0x64/0x80
> <4>[    1.064440]  _regmap_write+0x58/0xa8
> <4>[    1.064545]  regcache_default_sync+0xcc/0x1a0
> <4>[    1.064660]  regcache_sync_region+0xdc/0xe8
> <4>[    1.064811]  aw9523_irq_bus_sync_unlock+0x30/0x48
> <4>[    1.064931]  __setup_irq+0x798/0x890
> <4>[    1.065034]  request_threaded_irq+0xe0/0x198
> <4>[    1.065188]  devm_request_threaded_irq+0x78/0xf8
> <4>[    1.065311]  gpio_keyboard_probe+0x2a8/0x468

scheduling while atomic happens when this trace gets called with interrupts
disabled, usually because someone has taken a spinlock.

Looking in __setup_irq() it looks safe.

I would turn on lock debugging (lockdep) and see if I can find it that way.

Yours,
Linus Walleij
Mark Brown Jan. 11, 2021, 1:10 p.m. UTC | #7
On Sun, Jan 10, 2021 at 03:32:47PM +0100, AngeloGioacchino Del Regno wrote:

> Then, the set_bit/clear_bit in aw9523_irq_mask(), aw9523_irq_unmask were
> replaced with calls to regmap_update_bits_async, example:

Why are you trying to use the _async() versions here?  Don't you need
the I/O to complete synchronously, and what would be the benefit of
dealing with the complexity of making things async?  The genirq
framework is definitely going to expect things to be complete before you
return to it...

Note also that this only really does anything for SPI, I2C doesn't offer
an aynchronous API.

> <4>[    1.064060]  regmap_i2c_write+0x1c/0x50

regmap will only use spinlocks if you explictly ask it to do so by
setting fast_io.  You can't use spinlock based locking for your register
cache if your device is attached via I2C, I2C is a very slow bus and I2C
controllers will need to sleep.  I'd be very surprised if there were any
benefit from using a flat cache with such a device frankly, the I/O is
going to be so slow that any performance gain from using a flat cache is
lost in the noise.
AngeloGioacchino Del Regno Jan. 11, 2021, 5:54 p.m. UTC | #8
Il 10/01/21 20:35, Linus Walleij ha scritto:
> On Sun, Jan 10, 2021 at 3:32 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org> wrote:
> 
>> So, I've retried some basic usage of the regcache, relevant snippets here:
>> static bool aw9523_volatile_reg(struct device *dev, unsigned int reg)
>> {
>>
>>          return reg == AW9523_REG_IN_STATE(0) ||
>>                 reg == AW9523_REG_IN_STATE(AW9523_PINS_PER_PORT) ||
>>                 reg == AW9523_REG_CHIPID;
>> }
> (...)
>> Since REG_IN_STATE is used to read the GPIO input level, it's not
>> cacheable,
> 
> Fair enough.
> 
>> then CHIPID was set as not cacheable for safety: that may be
>> avoided, but that may make no sense.. since it's a one-time readout for
>> init putposes, it'd be useless to keep it cached.
> 
> I guess.
> 
>> Then, the set_bit/clear_bit in aw9523_irq_mask(), aw9523_irq_unmask were
>> replaced with calls to regmap_update_bits_async, example:
>>
>>          regmap_update_bits_async(awi->regmap,
>>                                   AW9523_REG_INTR_DIS(d->hwirq),
>>                                   BIT(n), BIT(n));
>>
>> Where of course the value is either BIT(n) or 0 for mask and unmask
>> respectively.
>> Also, the bus_sync_unlock callback was changed as follows:
>>
>> static void aw9523_irq_bus_sync_unlock(struct irq_data *d)
>>
>> {
>>        struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
>>        regcache_mark_dirty(awi->regmap);
>>        regcache_sync_region(awi->regmap, AW9523_REG_INTR_DIS(0),
>>                             AW9523_REG_INTR_DIS(AW9523_PINS_PER_PORT));
>>         mutex_unlock(&awi->irq->lock);
> (...)
>> One of the biggest / oddest issues that I get when trying to use
>> regcache is that I'm getting badbadbad scheduling while atomic warnings
>> all over and I don't get why, since regcache_default_sync is just
>> calling _regmap_write, which is exactly what (non _prefix) regmap_write
>> also calls...
> 
> OK that is the real problem to solve then.
> 
>> As a reference, this is one out of "many" (as you can imagine) stacktraces:
>>
>> <3>[    1.061428] BUG: scheduling while atomic: kworker/3:1/119/0x00000000
> (...)
>> <4>[    1.063134]  wait_for_completion_timeout+0x8c/0x110
>> <4>[    1.063257]  qup_i2c_wait_for_complete.isra.18+0x1c/0x80
>> <4>[    1.063429]  qup_i2c_xfer_v2_msg+0x2d4/0x3f0
>> <4>[    1.063543]  qup_i2c_xfer_v2+0x290/0xa28
>> <4>[    1.063652]  __i2c_transfer+0x16c/0x380
>> <4>[    1.063798]  i2c_transfer+0x5c/0x138
>> <4>[    1.063903]  i2c_transfer_buffer_flags+0x58/0x80
>> <4>[    1.064060]  regmap_i2c_write+0x1c/0x50
>> <4>[    1.064168]  _regmap_raw_write_impl+0x35c/0x688
>> <4>[    1.064285]  _regmap_bus_raw_write+0x64/0x80
>> <4>[    1.064440]  _regmap_write+0x58/0xa8
>> <4>[    1.064545]  regcache_default_sync+0xcc/0x1a0
>> <4>[    1.064660]  regcache_sync_region+0xdc/0xe8
>> <4>[    1.064811]  aw9523_irq_bus_sync_unlock+0x30/0x48
>> <4>[    1.064931]  __setup_irq+0x798/0x890
>> <4>[    1.065034]  request_threaded_irq+0xe0/0x198
>> <4>[    1.065188]  devm_request_threaded_irq+0x78/0xf8
>> <4>[    1.065311]  gpio_keyboard_probe+0x2a8/0x468
> 
> scheduling while atomic happens when this trace gets called with interrupts
> disabled, usually because someone has taken a spinlock.
> 
> Looking in __setup_irq() it looks safe.
> 
> I would turn on lock debugging (lockdep) and see if I can find it that way.
> 
> Yours,
> Linus Walleij
> 

Hey!
Good news around the corner!

So, the issues were relative to the gpio matrix_keypad driver, which is 
protecting with spinlocks (!), "throwing" us in atomic context and 
obviously producing this kind of havoc.

Regarding this, I feel like we should bring this to the attention of the 
matrix_keypad driver maintainer, Marek Vasut, which I'm including to the 
Cc list of this email... but at the same time, that driver seems to be 
largely outdated and for this reason I've decided to make one on-the-fly 
that uses modern APIs instead and also seems to solve slowness issues on 
my KB matrix connected to the AW9523.

Back to our topic, I have solved the issues that were preventing the 
usage of a FLAT regcache, cleaned up a bit and tested the entire thing 
again.
This works even better than before.

The V2 of this series is coming in a few minutes.
A huge thank you for your help!

-- Angelo
diff mbox series

Patch

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 815095326e2d..7d91e75576f1 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -110,6 +110,23 @@  config PINCTRL_AMD
 	  Requires ACPI/FDT device enumeration code to set up a platform
 	  device.
 
+config PINCTRL_AW9523
+	bool "Awinic AW9523/AW9523B I2C GPIO expander pinctrl driver"
+	depends on OF && I2C
+	select PINMUX
+	select PINCONF
+	select GENERIC_PINCONF
+	select GPIOLIB
+	select GPIOLIB_IRQCHIP
+	select REGMAP
+	help
+	  The Awinic AW9523/AW9523B is a multi-function I2C GPIO
+	  expander with PWM functionality. This driver bundles a
+	  pinctrl driver to select the function muxing and a GPIO
+	  driver to handle GPIO, when the GPIO function is selected.
+
+	  Say yes to enable pinctrl and GPIO support for the AW9523(B).
+
 config PINCTRL_BM1880
 	bool "Bitmain BM1880 Pinctrl driver"
 	depends on OF && (ARCH_BITMAIN || COMPILE_TEST)
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index f53933b2ff02..5696cd6593d5 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -14,6 +14,7 @@  obj-$(CONFIG_PINCTRL_AXP209)	+= pinctrl-axp209.o
 obj-$(CONFIG_PINCTRL_AT91)	+= pinctrl-at91.o
 obj-$(CONFIG_PINCTRL_AT91PIO4)	+= pinctrl-at91-pio4.o
 obj-$(CONFIG_PINCTRL_AMD)	+= pinctrl-amd.o
+obj-$(CONFIG_PINCTRL_AW9523)	+= pinctrl-aw9523.o
 obj-$(CONFIG_PINCTRL_BM1880)	+= pinctrl-bm1880.o
 obj-$(CONFIG_PINCTRL_DA850_PUPD) += pinctrl-da850-pupd.o
 obj-$(CONFIG_PINCTRL_DA9062)	+= pinctrl-da9062.o
diff --git a/drivers/pinctrl/pinctrl-aw9523.c b/drivers/pinctrl/pinctrl-aw9523.c
new file mode 100644
index 000000000000..a6004c2a3f8e
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-aw9523.c
@@ -0,0 +1,1070 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Awinic AW9523B i2c pin controller driver
+ * Copyright (c) 2020, AngeloGioacchino Del Regno
+ *                     <angelogioacchino.delregno@somainline.org>
+ */
+
+#include <linux/regmap.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/regulator/consumer.h>
+
+#include "core.h"
+#include "pinconf.h"
+#include "pinctrl-utils.h"
+
+#define AW9523_MAX_FUNCS		2
+#define AW9523_NUM_PORTS		2
+#define AW9523_PINS_PER_PORT		8
+
+/*
+ * HW needs at least 20uS for reset and at least 1-2uS to recover from
+ * reset, but we have to account for eventual board quirks, if any:
+ * for this reason, keep reset asserted for 50uS and wait for 20uS
+ * to recover from the reset.
+ */
+#define AW9523_HW_RESET_US		50
+#define AW9523_HW_RESET_RECOVERY_US	20
+
+/* Port 0: P0_0...P0_7 - Port 1: P1_0...P1_7 */
+#define AW9523_PIN_TO_PORT(pin)		(pin >> 3)
+#define AW9523_REG_IN_STATE(pin)	(0x00 + AW9523_PIN_TO_PORT(pin))
+#define AW9523_REG_OUT_STATE(pin)	(0x02 + AW9523_PIN_TO_PORT(pin))
+#define AW9523_REG_CONF_STATE(pin)	(0x04 + AW9523_PIN_TO_PORT(pin))
+#define AW9523_REG_INTR_DIS(pin)	(0x06 + AW9523_PIN_TO_PORT(pin))
+#define AW9523_REG_CHIPID		0x10
+#define AW9523_VAL_EXPECTED_CHIPID	0x23
+
+#define AW9523_REG_GCR			0x11
+#define AW9523_GCR_ISEL_MASK		GENMASK(0, 1)
+#define AW9523_GCR_GPOMD_MASK		BIT(4)
+
+#define AW9523_REG_PORT_MODE(pin)	(0x12 + AW9523_PIN_TO_PORT(pin))
+#define AW9523_REG_SOFT_RESET		0x7f
+#define AW9523_VAL_RESET		0x00
+
+/*
+ * struct aw9523_irq - Interrupt controller structure
+ * @lock: mutex locking for the irq bus
+ * @irqchip: structure holding irqchip params
+ * @old_masked: stores the previous interrupt mask for bit comparison
+ * @masked: stores the interrupt mask to program to the HW
+ * @cached_gpio: stores the previous gpio status for bit comparison
+ */
+struct aw9523_irq {
+	struct mutex lock;
+	struct irq_chip *irqchip;
+	DECLARE_BITMAP(old_masked[AW9523_NUM_PORTS], AW9523_PINS_PER_PORT);
+	DECLARE_BITMAP(masked[AW9523_NUM_PORTS], AW9523_PINS_PER_PORT);
+	u16 cached_gpio;
+};
+
+/*
+ * struct aw9523_pinmux - Pin mux params
+ * @name: Name of the mux
+ * @grps: Groups of the mux
+ * @num_grps: Number of groups (sizeof array grps)
+ */
+struct aw9523_pinmux {
+	const char *name;
+	const char * const *grps;
+	const u8 num_grps;
+};
+
+/*
+ * struct aw9523 - Main driver structure
+ * @dev: device handle
+ * @regmap: regmap handle for current device
+ * @i2c_lock: Mutex lock for i2c operations
+ * @reset_gpio: Hardware reset (RSTN) signal GPIO
+ * @vio_vreg: VCC regulator (Optional)
+ * @pctl: pinctrl handle for current device
+ * @gpio: structure holding gpiochip params
+ * @irq: Interrupt controller structure
+ */
+struct aw9523 {
+	struct device *dev;
+	struct regmap *regmap;
+	struct mutex i2c_lock;
+	struct gpio_desc *reset_gpio;
+	struct regulator *vio_vreg;
+	struct pinctrl_dev *pctl;
+	struct gpio_chip gpio;
+	struct aw9523_irq *irq;
+	DECLARE_BITMAP(direction_in[AW9523_NUM_PORTS], AW9523_PINS_PER_PORT);
+};
+
+static const struct pinctrl_pin_desc aw9523_pins[] = {
+	/* Port 0 */
+	PINCTRL_PIN(0, "gpio0"),
+	PINCTRL_PIN(1, "gpio1"),
+	PINCTRL_PIN(2, "gpio2"),
+	PINCTRL_PIN(3, "gpio3"),
+	PINCTRL_PIN(4, "gpio4"),
+	PINCTRL_PIN(5, "gpio5"),
+	PINCTRL_PIN(6, "gpio6"),
+	PINCTRL_PIN(7, "gpio7"),
+
+	/* Port 1 */
+	PINCTRL_PIN(8, "gpio8"),
+	PINCTRL_PIN(9, "gpio9"),
+	PINCTRL_PIN(10, "gpio10"),
+	PINCTRL_PIN(11, "gpio11"),
+	PINCTRL_PIN(12, "gpio12"),
+	PINCTRL_PIN(13, "gpio13"),
+	PINCTRL_PIN(14, "gpio14"),
+	PINCTRL_PIN(15, "gpio15"),
+};
+
+static int aw9523_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	return ARRAY_SIZE(aw9523_pins);
+}
+
+static const char *aw9523_pinctrl_get_group_name(struct pinctrl_dev *pctldev,
+						 unsigned int selector)
+{
+	return aw9523_pins[selector].name;
+}
+
+static int aw9523_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
+					 unsigned int selector,
+					 const unsigned int **pins,
+					 unsigned int *num_pins)
+{
+	*pins = &aw9523_pins[selector].number;
+	*num_pins = 1;
+	return 0;
+}
+
+static const struct pinctrl_ops aw9523_pinctrl_ops = {
+	.get_groups_count = aw9523_pinctrl_get_groups_count,
+	.get_group_pins = aw9523_pinctrl_get_group_pins,
+	.get_group_name = aw9523_pinctrl_get_group_name,
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
+	.dt_free_map = pinconf_generic_dt_free_map,
+};
+
+static const char * const gpio_pwm_groups[] = {
+	"gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5",
+	"gpio6", "gpio7", "gpio8", "gpio9", "gpio10", "gpio11",
+	"gpio12", "gpio13", "gpio14", "gpio15"
+};
+
+/* Warning: Do NOT reorder this array */
+static const struct aw9523_pinmux aw9523_pmx[] = {
+	{
+		.name = "pwm",
+		.grps = gpio_pwm_groups,
+		.num_grps = ARRAY_SIZE(gpio_pwm_groups),
+	},
+	{
+		.name = "gpio",
+		.grps = gpio_pwm_groups,
+		.num_grps = ARRAY_SIZE(gpio_pwm_groups),
+	},
+};
+
+static int aw9523_pmx_get_funcs_count(struct pinctrl_dev *pctl)
+{
+	return ARRAY_SIZE(aw9523_pmx);
+}
+
+static const char *aw9523_pmx_get_fname(struct pinctrl_dev *pctl,
+					unsigned int sel)
+{
+	return aw9523_pmx[sel].name;
+}
+
+static int aw9523_pmx_get_groups(struct pinctrl_dev *pctl, unsigned int sel,
+				 const char * const **groups,
+				 unsigned int * const num_groups)
+{
+	*groups = aw9523_pmx[sel].grps;
+	*num_groups = aw9523_pmx[sel].num_grps;
+	return 0;
+}
+
+static int aw9523_pmx_set_mux(struct pinctrl_dev *pctl, unsigned int fsel,
+			      unsigned int grp)
+{
+	struct aw9523 *awi = pinctrl_dev_get_drvdata(pctl);
+	int ret, pin = aw9523_pins[grp].number % AW9523_PINS_PER_PORT;
+
+	if (fsel >= ARRAY_SIZE(aw9523_pmx))
+		return -EINVAL;
+
+	/*
+	 * This maps directly to the aw9523_pmx array: programming a
+	 * high bit means "gpio" and a low bit means "pwm".
+	 */
+	mutex_lock(&awi->i2c_lock);
+	ret = regmap_update_bits(awi->regmap, AW9523_REG_PORT_MODE(pin),
+				 BIT(pin), (fsel ? BIT(pin) : 0));
+	mutex_unlock(&awi->i2c_lock);
+	return ret;
+}
+
+static const struct pinmux_ops aw9523_pinmux_ops = {
+	.get_functions_count	= aw9523_pmx_get_funcs_count,
+	.get_function_name	= aw9523_pmx_get_fname,
+	.get_function_groups	= aw9523_pmx_get_groups,
+	.set_mux		= aw9523_pmx_set_mux,
+};
+
+static int aw9523_pcfg_param_to_reg(enum pin_config_param pcp, int pin, u8 *r)
+{
+	u8 reg;
+
+	switch (pcp) {
+	case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+	case PIN_CONFIG_BIAS_PULL_UP:
+		reg = AW9523_REG_IN_STATE(pin);
+		break;
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		reg = AW9523_REG_GCR;
+		break;
+	case PIN_CONFIG_INPUT_ENABLE:
+	case PIN_CONFIG_OUTPUT_ENABLE:
+		reg = AW9523_REG_CONF_STATE(pin);
+		break;
+	case PIN_CONFIG_OUTPUT:
+		reg = AW9523_REG_OUT_STATE(pin);
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+	*r = reg;
+
+	return 0;
+}
+
+static int aw9523_pconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
+			    unsigned long *config)
+{
+	struct aw9523 *awi = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	int hw_pin = pin % AW9523_PINS_PER_PORT;
+	unsigned int val;
+	u8 reg;
+	int rc;
+
+	rc = aw9523_pcfg_param_to_reg(param, pin, &reg);
+	if (rc)
+		return rc;
+
+	mutex_lock(&awi->i2c_lock);
+	rc = regmap_read(awi->regmap, reg, &val);
+	mutex_unlock(&awi->i2c_lock);
+	if (rc)
+		return rc;
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_PULL_UP:
+	case PIN_CONFIG_INPUT_ENABLE:
+	case PIN_CONFIG_OUTPUT:
+		val &= BIT(hw_pin);
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+	case PIN_CONFIG_OUTPUT_ENABLE:
+		val &= BIT(hw_pin);
+		val = !val;
+		break;
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		if (pin >= AW9523_PINS_PER_PORT)
+			val = 0;
+		else
+			val = !FIELD_GET(AW9523_GCR_GPOMD_MASK, val);
+		break;
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		if (pin >= AW9523_PINS_PER_PORT)
+			val = 1;
+		else
+			val = FIELD_GET(AW9523_GCR_GPOMD_MASK, val);
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+	if (val < 1)
+		return -EINVAL;
+
+	*config = pinconf_to_config_packed(param, !!val);
+
+	return rc;
+}
+
+static int aw9523_pconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
+			    unsigned long *configs, unsigned int num_configs)
+{
+	struct aw9523 *awi = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param;
+	int hw_pin = pin % AW9523_PINS_PER_PORT;
+	u32 arg;
+	u8 reg;
+	unsigned int mask, val;
+	int i, rc;
+
+	mutex_lock(&awi->i2c_lock);
+	for (i = 0; i < num_configs; i++) {
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
+
+		rc = aw9523_pcfg_param_to_reg(param, pin, &reg);
+		if (rc) {
+			goto end;
+		}
+
+		switch (param) {
+		case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+		case PIN_CONFIG_OUTPUT_ENABLE:
+			mask = BIT(hw_pin);
+			val = arg ? 0 : BIT(hw_pin);
+			break;
+		case PIN_CONFIG_OUTPUT:
+			/* First, enable pin output */
+			rc = regmap_update_bits(awi->regmap,
+						AW9523_REG_CONF_STATE(pin),
+						BIT(hw_pin), 0);
+			if (rc)
+				goto end;
+			/* Then, fall through to config output level */
+			fallthrough;
+		case PIN_CONFIG_BIAS_PULL_UP:
+		case PIN_CONFIG_INPUT_ENABLE:
+			mask = BIT(hw_pin);
+			val = arg ? BIT(hw_pin) : 0;
+			break;
+		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+			/* Open-Drain is supported only on port 0 */
+			if (pin >= AW9523_PINS_PER_PORT) {
+				rc = -ENOTSUPP;
+				goto end;
+			}
+			mask = AW9523_GCR_GPOMD_MASK;
+			val = 0;
+			break;
+		case PIN_CONFIG_DRIVE_PUSH_PULL:
+			/* Port 1 is always Push-Pull */
+			if (pin >= AW9523_PINS_PER_PORT) {
+				mask = 0;
+				val = 0;
+				continue;
+			}
+			mask = AW9523_GCR_GPOMD_MASK;
+			val = AW9523_GCR_GPOMD_MASK;
+			break;
+		default:
+			rc = -ENOTSUPP;
+			goto end;
+		}
+
+		rc = regmap_update_bits(awi->regmap, reg, mask, val);
+		if (rc)
+			goto end;
+	}
+end:
+	mutex_unlock(&awi->i2c_lock);
+	return rc;
+}
+
+static const struct pinconf_ops aw9523_pinconf_ops = {
+	.pin_config_get = aw9523_pconf_get,
+	.pin_config_set = aw9523_pconf_set,
+	.is_generic = true,
+};
+
+/*
+ * __aw9523_gpio_get_direction - Get pin direction
+ * @regmap: Regmap structure
+ * @pin: gpiolib pin number
+ * @hwp: pin index in port register
+ *
+ * Return: Pin direction for success or negative number for error
+ */
+static int __aw9523_gpio_get_direction(struct regmap *regmap, u8 pin, u8 hwp)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(regmap, AW9523_REG_CONF_STATE(pin), &val);
+	if (ret)
+		return ret;
+
+	return val & BIT(hwp) ? GPIO_LINE_DIRECTION_IN :
+				GPIO_LINE_DIRECTION_OUT;
+}
+
+/*
+ * __aw5923_get_port_state - Get input or output state for entire port
+ * @regmap: Regmap structure
+ * @pin: gpiolib pin number
+ * @hw_pin: hw pin index, used to retrieve port number
+ * @state: returned port state
+ *
+ * Return: Zero for success or negative number for error
+ */
+static int __aw9523_get_port_state(struct regmap *regmap, u8 pin,
+				   u8 hw_pin, unsigned int *state)
+{
+	u8 reg;
+	int dir;
+
+	dir = __aw9523_gpio_get_direction(regmap, pin, hw_pin);
+	if (dir < 0)
+		return dir;
+
+	if (dir == GPIO_LINE_DIRECTION_IN)
+		reg = AW9523_REG_IN_STATE(pin);
+	else
+		reg = AW9523_REG_OUT_STATE(pin);
+
+	return regmap_read(regmap, reg, state);
+}
+
+static int aw9523_gpio_irq_type(struct irq_data *d, unsigned int type)
+{
+	switch (type) {
+	case IRQ_TYPE_NONE:
+	case IRQ_TYPE_LEVEL_MASK:
+	case IRQ_TYPE_LEVEL_HIGH:
+	case IRQ_TYPE_LEVEL_LOW:
+	case IRQ_TYPE_EDGE_BOTH:
+	case IRQ_TYPE_EDGE_RISING:
+	case IRQ_TYPE_EDGE_FALLING:
+		return 0;
+	default:
+		return -EINVAL;
+	};
+}
+
+/*
+ * aw9523_irq_mask - Mask interrupt
+ * @d: irq data
+ *
+ * Sets which interrupt to mask in the bitmap;
+ * The interrupt will be masked when unlocking the irq bus.
+ */
+static void aw9523_irq_mask(struct irq_data *d)
+{
+	struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
+	unsigned int n = d->hwirq % AW9523_PINS_PER_PORT;
+	int port = AW9523_PIN_TO_PORT(d->hwirq);
+
+	set_bit(n, awi->irq->masked[port]);
+}
+
+/*
+ * aw9523_irq_unmask - Unmask interrupt
+ * @d: irq data
+ *
+ * Sets which interrupt to unmask in the bitmap;
+ * The interrupt will be masked when unlocking the irq bus.
+ */
+static void aw9523_irq_unmask(struct irq_data *d)
+{
+	struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
+	unsigned int n = d->hwirq % AW9523_PINS_PER_PORT;
+	int port = AW9523_PIN_TO_PORT(d->hwirq);
+
+	clear_bit(n, awi->irq->masked[port]);
+}
+
+static irqreturn_t aw9523_irq_thread_func(int irq, void *dev_id)
+{
+	struct aw9523 *awi = (struct aw9523 *)dev_id;
+	unsigned long n, val = 0;
+	unsigned long changed_gpio;
+	unsigned int tmp, port_pin, i, ret;
+
+	for (i = 0; i < AW9523_NUM_PORTS; i++) {
+		port_pin = i * AW9523_PINS_PER_PORT;
+		ret = regmap_read(awi->regmap,
+				  AW9523_REG_IN_STATE(port_pin),
+				  &tmp);
+		if (ret)
+			return ret;
+
+		val |= (u8)tmp << (i * 8);
+	}
+
+	/* Handle GPIO input release interrupt as well */
+	changed_gpio = awi->irq->cached_gpio ^ val;
+	awi->irq->cached_gpio = val;
+
+	/*
+	 * To avoid up to four *slow* i2c reads from any driver hooked
+	 * up to our interrupts, just check for the irq_find_mapping
+	 * result: if the interrupt is not mapped, then we don't want
+	 * to care about it.
+	 */
+	for_each_set_bit(n, &changed_gpio, awi->gpio.ngpio) {
+		tmp = irq_find_mapping(awi->gpio.irq.domain, n);
+		if (tmp <= 0)
+			continue;
+		handle_nested_irq(tmp);
+	}
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * aw9523_irq_bus_lock - Grab lock for interrupt operation
+ * @d: irq data
+ */
+static void aw9523_irq_bus_lock(struct irq_data *d)
+{
+	struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
+
+	mutex_lock(&awi->irq->lock);
+}
+
+/*
+ * aw9523_irq_bus_sync_unlock - Synchronize state and unlock
+ * @d: irq data
+ *
+ * Writes the interrupt mask bits (found in the bit map) to the
+ * hardware, then unlocks the bus.
+ */
+static void aw9523_irq_bus_sync_unlock(struct irq_data *d)
+{
+	struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
+	int i;
+
+	for (i = 0; i < AW9523_NUM_PORTS; i++) {
+		if (bitmap_equal(awi->irq->masked[i], awi->irq->old_masked[i],
+				 AW9523_PINS_PER_PORT))
+			continue;
+		regmap_write(awi->regmap,
+			     AW9523_REG_INTR_DIS(AW9523_PINS_PER_PORT * i),
+			     *awi->irq->masked[i]);
+		bitmap_copy(awi->irq->old_masked[i], awi->irq->masked[i],
+			    AW9523_PINS_PER_PORT);
+	}
+	mutex_unlock(&awi->irq->lock);
+}
+
+static int aw9523_gpio_get_direction(struct gpio_chip *chip,
+				     unsigned int offset)
+{
+	struct aw9523 *awi = gpiochip_get_data(chip);
+	u8 hw_pin = offset % AW9523_PINS_PER_PORT;
+
+	return __aw9523_gpio_get_direction(awi->regmap, offset, hw_pin);
+}
+
+static int aw9523_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct aw9523 *awi = gpiochip_get_data(chip);
+	u8 hw_pin = offset % AW9523_PINS_PER_PORT;
+	unsigned int val;
+	int ret;
+
+	mutex_lock(&awi->i2c_lock);
+	ret = __aw9523_get_port_state(awi->regmap, offset, hw_pin, &val);
+	mutex_unlock(&awi->i2c_lock);
+	if (ret)
+		return ret;
+
+	return !!(val & BIT(hw_pin));
+}
+
+/**
+ * _aw9523_gpio_get_multiple - Get I/O state for an entire port
+ * @regmap: Regmap structure
+ * @pin: gpiolib pin number
+ * @hw_pin: hw pin index, used to retrieve port number
+ * @state: returned port I/O state
+ *
+ * Return: Zero for success or negative number for error
+ */
+static int _aw9523_gpio_get_multiple(struct aw9523 *awi, u8 hw_pin,
+				     u8 *state, u8 mask)
+{
+	u32 dir_in, val;
+	u8 m;
+	int ret;
+
+	/* Registers are 8-bits wide */
+	ret = regmap_read(awi->regmap, AW9523_REG_CONF_STATE(hw_pin), &dir_in);
+	if (ret)
+		return ret;
+	*state = 0;
+
+	m = mask & dir_in;
+	if (m) {
+		ret = regmap_read(awi->regmap, AW9523_REG_IN_STATE(hw_pin),
+				  &val);
+		if (ret)
+			return ret;
+		*state |= (u8)val & m;
+	}
+
+	m = mask & ~dir_in;
+	if (m) {
+		ret = regmap_read(awi->regmap, AW9523_REG_OUT_STATE(hw_pin),
+				  &val);
+		if (ret)
+			return ret;
+		*state |= (u8)val & m;
+	}
+
+	return 0;
+}
+
+static int aw9523_gpio_get_multiple(struct gpio_chip *chip,
+				    unsigned long *mask,
+				    unsigned long *bits)
+{
+	struct aw9523 *awi = gpiochip_get_data(chip);
+	u8 m, state = 0;
+	int ret;
+
+	mutex_lock(&awi->i2c_lock);
+
+	/* Port 0 (gpio 0-7) */
+	m = *mask & U8_MAX;
+	if (m) {
+		ret = _aw9523_gpio_get_multiple(awi, 0, &state, m);
+		if (ret)
+			goto out;
+	}
+	*bits = state;
+
+	/* Port 1 (gpio 8-15) */
+	m = (*mask >> 8) & U8_MAX;
+	if (m) {
+		ret = _aw9523_gpio_get_multiple(awi, AW9523_PINS_PER_PORT,
+						&state, m);
+		if (ret)
+			goto out;
+
+		*bits |= (state << 8);
+	}
+out:
+	mutex_unlock(&awi->i2c_lock);
+	return ret;
+}
+
+static void aw9523_gpio_set_multiple(struct gpio_chip *chip,
+				    unsigned long *mask,
+				    unsigned long *bits)
+{
+	struct aw9523 *awi = gpiochip_get_data(chip);
+	u8 mask_lo, mask_hi, bits_lo, bits_hi;
+	unsigned int reg;
+	int ret = 0;
+
+	mask_lo = *mask & U8_MAX;
+	mask_hi = (*mask >> 8) & U8_MAX;
+	mutex_lock(&awi->i2c_lock);
+	if (mask_hi) {
+		reg = AW9523_REG_OUT_STATE(AW9523_PINS_PER_PORT);
+		bits_hi = (*bits >> 8) & U8_MAX;
+
+		ret = regmap_write_bits(awi->regmap, reg, mask_hi, bits_hi);
+		if (ret) {
+			dev_warn(awi->dev, "Cannot write port1 out level\n");
+			goto out;
+		}
+	}
+	if (mask_lo) {
+		reg = AW9523_REG_OUT_STATE(0);
+		bits_lo = *bits & U8_MAX;
+		ret = regmap_write_bits(awi->regmap, reg, mask_lo, bits_lo);
+		if (ret)
+			dev_warn(awi->dev, "Cannot write port0 out level\n");
+	}
+out:
+	mutex_unlock(&awi->i2c_lock);
+}
+
+static void aw9523_gpio_set(struct gpio_chip *chip,
+			    unsigned int offset, int value)
+{
+	struct aw9523 *awi = gpiochip_get_data(chip);
+	u8 hw_pin = offset % AW9523_PINS_PER_PORT;
+
+	mutex_lock(&awi->i2c_lock);
+	regmap_update_bits(awi->regmap, AW9523_REG_OUT_STATE(offset),
+			   BIT(hw_pin), value ? BIT(hw_pin) : 0);
+	mutex_unlock(&awi->i2c_lock);
+}
+
+
+static int aw9523_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	struct aw9523 *awi = gpiochip_get_data(chip);
+	u8 hw_pin = offset % AW9523_PINS_PER_PORT;
+	int port = AW9523_PIN_TO_PORT(offset);
+
+	set_bit(offset, awi->direction_in[port]);
+
+	return regmap_update_bits(awi->regmap, AW9523_REG_CONF_STATE(offset),
+				  BIT(hw_pin), BIT(hw_pin));
+}
+
+static int aw9523_direction_output(struct gpio_chip *chip,
+				   unsigned int offset, int value)
+{
+	struct aw9523 *awi = gpiochip_get_data(chip);
+	u8 hw_pin = offset % AW9523_PINS_PER_PORT;
+	int port = AW9523_PIN_TO_PORT(offset);
+	int ret;
+
+	ret = regmap_update_bits(awi->regmap, AW9523_REG_OUT_STATE(offset),
+				 BIT(hw_pin), value ? BIT(hw_pin) : 0);
+	if (ret)
+		return ret;
+
+	if (test_bit(offset, awi->direction_in[port])) {
+		ret = regmap_update_bits(awi->regmap,
+					 AW9523_REG_CONF_STATE(offset),
+					 BIT(hw_pin), 0);
+		clear_bit(offset, awi->direction_in[port]);
+	}
+
+	return ret;
+}
+
+static int __aw9523_hw_reset(struct aw9523 *awi)
+{
+	unsigned int chip_id;
+	int ret;
+
+	/*
+	 * If the chip is already configured for any reason, then we
+	 * will probably succeed in sending the soft reset signal to
+	 * the hardware through I2C: this operation takes less time
+	 * compared to a full HW reset and it gives the same results.
+	 */
+	ret = regmap_write(awi->regmap, AW9523_REG_SOFT_RESET, 0);
+	if (ret == 0)
+		goto done;
+
+	dev_dbg(awi->dev, "Cannot execute soft reset: trying hard reset\n");
+	ret = gpiod_direction_output(awi->reset_gpio, 0);
+	if (ret)
+		return ret;
+
+	/* The reset pulse has to be longer than 20uS due to deglitch */
+	usleep_range(AW9523_HW_RESET_US, AW9523_HW_RESET_US + 1);
+
+	ret = gpiod_direction_output(awi->reset_gpio, 1);
+	if (ret)
+		return ret;
+done:
+	/* The HW needs at least 1uS to reliably recover after reset */
+	usleep_range(AW9523_HW_RESET_RECOVERY_US,
+		     AW9523_HW_RESET_RECOVERY_US + 1);
+
+	/* Check the ChipID */
+	ret = regmap_read(awi->regmap, AW9523_REG_CHIPID, &chip_id);
+	if (ret) {
+		dev_err(awi->dev, "Cannot read Chip ID: %d\n", ret);
+		return ret;
+	}
+	if (chip_id != AW9523_VAL_EXPECTED_CHIPID) {
+		dev_err(awi->dev, "Bad ChipID; read 0x%x, expected 0x%x\n",
+			chip_id, AW9523_VAL_EXPECTED_CHIPID);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int aw9523_hw_reset(struct aw9523 *awi)
+{
+	int ret, max_retries = 2;
+
+	/* Sometimes the chip needs more than one reset cycle */
+	do {
+		ret = __aw9523_hw_reset(awi);
+		if (ret == 0)
+			break;
+		max_retries--;
+	} while (max_retries);
+
+	return ret;
+}
+
+static int aw9523_init_gpiochip(struct aw9523 *awi, unsigned int npins)
+{
+	struct device *dev = awi->dev;
+	struct gpio_chip *gpiochip = &awi->gpio;
+
+	gpiochip->label = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
+	if (!gpiochip->label)
+		return -ENOMEM;
+
+	gpiochip->base = -1;
+	gpiochip->ngpio = npins;
+	gpiochip->get_direction = aw9523_gpio_get_direction;
+	gpiochip->direction_input = aw9523_direction_input;
+	gpiochip->direction_output = aw9523_direction_output;
+	gpiochip->get = aw9523_gpio_get;
+	gpiochip->get_multiple = aw9523_gpio_get_multiple;
+	gpiochip->set = aw9523_gpio_set;
+	gpiochip->set_multiple = aw9523_gpio_set_multiple;
+	gpiochip->set_config = gpiochip_generic_config;
+	gpiochip->parent = dev;
+	gpiochip->of_node = dev->of_node;
+	gpiochip->owner = THIS_MODULE;
+	gpiochip->can_sleep = true;
+
+	return 0;
+}
+
+static int aw9523_init_irq(struct aw9523 *awi, int irq)
+{
+	struct device *dev = awi->dev;
+	struct gpio_irq_chip *gpioirq;
+	struct irq_chip *irqchip;
+	int i, ret;
+
+	if (!device_property_read_bool(dev, "interrupt-controller"))
+		return 0;
+
+	irqchip = devm_kzalloc(dev, sizeof(*irqchip), GFP_KERNEL);
+	if (!irqchip)
+		return -ENOMEM;
+
+	awi->irq = devm_kzalloc(dev, sizeof(*awi->irq), GFP_KERNEL);
+	if (!awi->irq)
+		return -ENOMEM;
+
+	irqchip->name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
+	if (!irqchip->name)
+		return -ENOMEM;
+
+	irqchip->irq_mask = aw9523_irq_mask;
+	irqchip->irq_unmask = aw9523_irq_unmask;
+	irqchip->irq_bus_lock = aw9523_irq_bus_lock;
+	irqchip->irq_bus_sync_unlock = aw9523_irq_bus_sync_unlock;
+	irqchip->irq_set_type = aw9523_gpio_irq_type;
+
+	for (i = 0; i < AW9523_NUM_PORTS; i++) {
+		bitmap_fill(awi->irq->masked[i], AW9523_PINS_PER_PORT);
+		bitmap_fill(awi->irq->old_masked[i], AW9523_PINS_PER_PORT);
+	}
+	awi->irq->irqchip = irqchip;
+	mutex_init(&awi->irq->lock);
+
+	ret = devm_request_threaded_irq(dev, irq, NULL, aw9523_irq_thread_func,
+					IRQF_ONESHOT, dev_name(dev), awi);
+	if (ret) {
+		dev_err(dev, "Failed to request irq %d\n", irq);
+		return ret;
+	}
+
+	gpioirq = &awi->gpio.irq;
+	gpioirq->chip = irqchip;
+	gpioirq->parent_handler = NULL;
+	gpioirq->num_parents = 0;
+	gpioirq->parents = NULL;
+	gpioirq->parent_domain = NULL;
+	gpioirq->default_type = IRQ_TYPE_NONE;
+	gpioirq->handler = handle_simple_irq;
+	gpioirq->threaded = true;
+	gpioirq->first = 0;
+
+	return 0;
+}
+
+static int aw9523_hw_init(struct aw9523 *awi)
+{
+	u8 p1_pin = AW9523_PINS_PER_PORT;
+	unsigned int val;
+	int i, ret;
+
+	/* Bring up the chip */
+	ret = aw9523_hw_reset(awi);
+	if (ret) {
+		dev_err(awi->dev, "HW Reset failed: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * This is the expected chip and it is running: it's time to
+	 * set a safe default configuration in case the user doesn't
+	 * configure (all of the available) pins in this chip.
+	 */
+
+	/* Set all pins as GPIO */
+	ret = regmap_write(awi->regmap, AW9523_REG_PORT_MODE(0), U8_MAX);
+	if (ret)
+		return ret;
+	ret = regmap_write(awi->regmap, AW9523_REG_PORT_MODE(p1_pin), U8_MAX);
+	if (ret)
+		return ret;
+
+	/* Set Open-Drain mode on Port 0 (Port 1 is always P-P) */
+	ret = regmap_update_bits(awi->regmap, AW9523_REG_GCR,
+				 AW9523_GCR_GPOMD_MASK, 0);
+	if (ret)
+		return ret;
+
+	/* Set all pins as inputs */
+	ret = regmap_write(awi->regmap, AW9523_REG_CONF_STATE(0), U8_MAX);
+	if (ret)
+		return ret;
+	ret = regmap_write(awi->regmap, AW9523_REG_CONF_STATE(p1_pin), U8_MAX);
+	if (ret)
+		return ret;
+	for (i = 0; i < AW9523_NUM_PORTS; i++)
+		bitmap_fill(awi->direction_in[i], AW9523_PINS_PER_PORT);
+
+	/* Disable all interrupts to avoid unreasoned wakeups */
+	ret = regmap_write(awi->regmap, AW9523_REG_INTR_DIS(0), U8_MAX);
+	if (ret)
+		return ret;
+	ret = regmap_write(awi->regmap, AW9523_REG_INTR_DIS(p1_pin), U8_MAX);
+	if (ret)
+		return ret;
+
+	/* Clear setup-generated interrupts by performing a port state read */
+	ret = __aw9523_get_port_state(awi->regmap, 0, 0, &val);
+	if (ret)
+		return ret;
+	ret = __aw9523_get_port_state(awi->regmap, p1_pin, 0, &val);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct regmap_config aw9523_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.cache_type = REGCACHE_NONE,
+	.disable_locking = true,
+
+	.max_register = AW9523_REG_SOFT_RESET,
+};
+
+static int aw9523_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct pinctrl_desc *pdesc;
+	struct aw9523 *awi;
+	int ret;
+
+	awi = devm_kzalloc(dev, sizeof(*awi), GFP_KERNEL);
+	if (!awi)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, awi);
+
+	awi->dev = dev;
+	awi->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(awi->reset_gpio))
+		return PTR_ERR(awi->reset_gpio);
+	gpiod_set_consumer_name(awi->reset_gpio, "aw9523 reset");
+
+	awi->regmap = devm_regmap_init_i2c(client, &aw9523_regmap);
+	if (IS_ERR(awi->regmap))
+		return PTR_ERR(awi->regmap);
+
+	awi->vio_vreg = devm_regulator_get_optional(dev, "vio");
+	if (IS_ERR(awi->vio_vreg)) {
+		if (PTR_ERR(awi->vio_vreg) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		awi->vio_vreg = NULL;
+	} else {
+		ret = regulator_enable(awi->vio_vreg);
+		if (ret)
+			return ret;
+	}
+
+
+	mutex_init(&awi->i2c_lock);
+	lockdep_set_subclass(&awi->i2c_lock,
+			     i2c_adapter_depth(client->adapter));
+
+	pdesc = devm_kzalloc(dev, sizeof(*pdesc), GFP_KERNEL);
+	if (!pdesc)
+		return -ENOMEM;
+
+	ret = aw9523_hw_init(awi);
+	if (ret)
+		goto err_disable_vregs;
+
+	pdesc->name = dev_name(dev);
+	pdesc->owner = THIS_MODULE;
+	pdesc->pctlops = &aw9523_pinctrl_ops;
+	pdesc->pmxops  = &aw9523_pinmux_ops;
+	pdesc->confops = &aw9523_pinconf_ops;
+	pdesc->pins = aw9523_pins;
+	pdesc->npins = ARRAY_SIZE(aw9523_pins);
+
+	ret = aw9523_init_gpiochip(awi, pdesc->npins);
+	if (ret)
+		goto err_disable_vregs;
+
+	if (client->irq) {
+		ret = aw9523_init_irq(awi, client->irq);
+		if (ret)
+			goto err_disable_vregs;
+	}
+
+	awi->pctl = devm_pinctrl_register(dev, pdesc, awi);
+	if (IS_ERR(awi->pctl)) {
+		ret = PTR_ERR(awi->pctl);
+		dev_err(dev, "Cannot register pinctrl: %d", ret);
+		goto err_disable_vregs;
+	}
+
+	ret = devm_gpiochip_add_data(dev, &awi->gpio, awi);
+	if (ret)
+		goto err_disable_vregs;
+
+	return ret;
+
+err_disable_vregs:
+	if (awi->vio_vreg)
+		regulator_disable(awi->vio_vreg);
+	return ret;
+}
+
+static const struct i2c_device_id aw9523_i2c_id_table[] = {
+	{ "aw9523_i2c", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, aw9523_i2c_id_table);
+
+static const struct of_device_id of_aw9523_i2c_match[] = {
+	{ .compatible = "awinic,aw9523-pinctrl", },
+};
+MODULE_DEVICE_TABLE(of, of_aw9523_i2c_match);
+
+static struct i2c_driver aw9523_driver = {
+	.driver = {
+		.name = "aw9523-pinctrl",
+		.of_match_table = of_aw9523_i2c_match,
+	},
+	.probe = aw9523_probe,
+	.id_table = aw9523_i2c_id_table,
+};
+module_i2c_driver(aw9523_driver);
+
+MODULE_DESCRIPTION("Awinic AW9523 I2C GPIO Expander driver");
+MODULE_AUTHOR("AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:aw9523");