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@xxxxxxxxxxxxxx> 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!
