Hi, On Thu, Nov 16, 2023 at 09:41:03AM +0100, Neil Armstrong wrote: > On 15/11/2023 19:00, Sebastian Reichel wrote: > > On Wed, Nov 15, 2023 at 06:17:50PM +0100, Neil Armstrong wrote: > > > 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... If you just add back PLATFORM_DEVID_NONE to devm_mfd_add_devices(), it will break rk806, since that correctly states PLATFORM_DEVID_AUTO in its .id and that would become -3. > > 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, -- Sebastian
Attachment:
signature.asc
Description: PGP signature
