Re: [PATCH v8 08/14] mfd: rk8xx: add rk806 support
From: Neil Armstrong
Date: Thu Nov 16 2023 - 03:41:13 EST
Hi,
On 15/11/2023 19:00, Sebastian Reichel wrote:
Hi Neil,
On Wed, Nov 15, 2023 at 06:17:50PM +0100, Neil Armstrong wrote:
Hi Sebastian,
On 04/05/2023 19:36, Sebastian Reichel wrote:
Add support for SPI connected rk806, which is used by the RK3588
evaluation boards. The PMIC is advertised to support I2C and SPI,
but the evaluation boards all use SPI. Thus only SPI support is
added here.
Acked-for-MFD-by: Lee Jones <lee@xxxxxxxxxx>
Tested-by: Diederik de Haas <didi.debian@xxxxxxxxx> # Rock64, Quartz64 Model A + B
Tested-by: Vincent Legoll <vincent.legoll@xxxxxxxxx> # Pine64 QuartzPro64
Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
---
drivers/mfd/Kconfig | 14 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/rk8xx-core.c | 69 ++++++-
drivers/mfd/rk8xx-spi.c | 124 ++++++++++++
include/linux/mfd/rk808.h | 409 ++++++++++++++++++++++++++++++++++++++
5 files changed, 614 insertions(+), 3 deletions(-)
create mode 100644 drivers/mfd/rk8xx-spi.c
<snip>
- ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
- cells, nr_cells, NULL, 0,
+ ret = devm_mfd_add_devices(dev, 0, cells, nr_cells, NULL, 0,
regmap_irq_get_domain(rk808->irq_data));
It seems you replaced PLATFORM_DEVID_NONE by 0, triggering again the bug preventing
having multiples RK pmics on the same system I fixed earlier at [1].
All cells have PLATFORM_DEVID_NONE specified and thus are registered
without an ID. I changed this bit to avoid overriding the
information, since I did not want to have PLATFORM_DEVID_NONE for
rk806.
This gives (again):
<4>[ 0.664107] sysfs: cannot create duplicate filename '/bus/platform/devices/rk808-clkout'
Which means, you do not want PLATFORM_DEVID_NONE (-1), but
PLATFORM_DEVID_AUTO (-2). The above path is the expected path
for PLATFORM_DEVID_NONE.
<4>[ 0.664120] CPU: 3 PID: 97 Comm: kworker/u12:2 Not tainted 6.6.1 #1
<4>[ 0.664131] Hardware name: Hardkernel ODROID-GO-Ultra (DT)
<4>[ 0.664139] Workqueue: events_unbound deferred_probe_work_func
<4>[ 0.664160] Call trace:
<4>[ 0.664165] dump_backtrace+0x9c/0x11c
<4>[ 0.664181] show_stack+0x18/0x24
<4>[ 0.664193] dump_stack_lvl+0x78/0xc4
<4>[ 0.664205] dump_stack+0x18/0x24
<4>[ 0.664215] sysfs_warn_dup+0x64/0x80
<4>[ 0.664227] sysfs_do_create_link_sd+0xf0/0xf8
<4>[ 0.664239] sysfs_create_link+0x20/0x40
<4>[ 0.664250] bus_add_device+0x114/0x160
<4>[ 0.664259] device_add+0x3f0/0x7cc
<4>[ 0.664267] platform_device_add+0x180/0x270
<4>[ 0.664278] mfd_add_device+0x390/0x4a8
<4>[ 0.664290] devm_mfd_add_devices+0xb0/0x150
<4>[ 0.664301] rk8xx_probe+0x26c/0x410
<4>[ 0.664312] rk8xx_i2c_probe+0x64/0x98
<4>[ 0.664323] i2c_device_probe+0x104/0x2e8
<4>[ 0.664333] really_probe+0x184/0x3c8
<4>[ 0.664342] __driver_probe_device+0x7c/0x16c
<4>[ 0.664351] driver_probe_device+0x3c/0x10c
<4>[ 0.664360] __device_attach_driver+0xbc/0x158
<4>[ 0.664369] bus_for_each_drv+0x80/0xdc
<4>[ 0.664377] __device_attach+0x9c/0x1ac
<4>[ 0.664386] device_initial_probe+0x14/0x20
<4>[ 0.664395] bus_probe_device+0xac/0xb0
<4>[ 0.664403] deferred_probe_work_func+0xa0/0xf4
<4>[ 0.664412] process_one_work+0x1bc/0x378
<4>[ 0.664421] worker_thread+0x1dc/0x3d4
<4>[ 0.664429] kthread+0x104/0x118
<4>[ 0.664440] ret_from_fork+0x10/0x20
<3>[ 0.664494] rk8xx-i2c 0-001c: error -EEXIST: failed to add MFD devices
<4>[ 0.666769] rk8xx-i2c: probe of 0-001c failed with error -17
I didn't notice when working on rk806, but after analyzing it now:
Your patch effectively set the cells to PLATFORM_DEVID_AUTO, because
you set all cells to PLATFORM_DEVID_NONE (-1) and additionally used
PLATFORM_DEVID_NONE (-1) for the devm_mfd_add_devices() call. But
that uses the sum of both IDs. Adding -1 to -1 is -2 and thus
PLATFORM_DEVID_AUTO. This is of course very confusing and just
worked by chance. There are two options:
1. Modify all cells to use PLATFORM_DEVID_AUTO instead of
PLATFORM_DEVID_NONE
2. Drop the .id from all cells and use PLATFORM_DEVID_AUTO in the
call to devm_mfd_add_devices()
Note, that switching from PLATFORM_DEVID_NONE to PLATFORM_DEVID_AUTO
modifies sysfs paths and thus might break people's scripts; that's why
I tried not to modify any existing platform. I will let you deal
with that, since I cannot even test any !rk806 platform supported by
this driver :)
Yes it will modify sysfs path, but it's a regression since this before this patch
everything was registered with PLATFORM_DEVID_AUTO anyway,
so I'll provide a fix adding back PLATFORM_DEVID_NONE to devm_mfd_add_devices
in a first time...
Also mfd_add_device should probably get special handling for
PLATFORM_DEVID_NONE, just like it already has special handling
for PLATFORM_DEVID_AUTO.
... and yes thanks for the great analysis I'll provide a change cleaning the mess.
Thanks,
Neil
Greetings,
-- Sebastian