On 10/20/23 4:22 PM, Peter Rosin wrote:
Hi!
2023-10-20 at 18:43, Andrew Davis wrote:
On 10/20/23 9:28 AM, Peter Rosin wrote:
Hi!
2023-09-11 at 17:10, Andrew Davis wrote:
The DT binding for the reg-mux compatible states it can be used when the
"parent device of mux controller is not syscon device". It also allows
for a reg property. When the reg property is provided, use that to
identify the address space for this mux. If not provided fallback to
using the parent device as a regmap provider.
Signed-off-by: Andrew Davis <afd@xxxxxx>
Reviewed-by: Nishanth Menon <nm@xxxxxx>
---
Changes from v2:
- Rebased on v6.6-rc1
Changes from v1:
- Flip logic as suggested in v1[0]
[0] https://lore.kernel.org/lkml/1c27d9d4-b1cc-c158-90f7-f7e47e02c424@xxxxxx/T/
drivers/mux/mmio.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
index fd1d121a584ba..b6095b7853ed2 100644
--- a/drivers/mux/mmio.c
+++ b/drivers/mux/mmio.c
@@ -44,10 +44,13 @@ static int mux_mmio_probe(struct platform_device *pdev)
int ret;
int i;
- if (of_device_is_compatible(np, "mmio-mux"))
+ if (of_device_is_compatible(np, "mmio-mux")) {
regmap = syscon_node_to_regmap(np->parent);
- else
- regmap = dev_get_regmap(dev->parent, NULL) ?: ERR_PTR(-ENODEV);
+ } else {
+ regmap = device_node_to_regmap(np);
I started digging in device_node_to_regmap() to try to find an error that
could be used to trigger if the failover to dev_get_regmap() should be
tried, instead of always doing the failover on error. I got lost fairly
quickly, but it seems device_node_to_regmap() can return -EDEFER_PROBE.
While I'm not certain that it is applicable, that case should probably
not fall back to dev_get_regmap()...
Are there other error cases that should prevent the failover? I would
guess that it's perhaps just a single error that should trigger trying
the failover path? But I don't know, and which error if that's the case?
Ideally the only error that will be returned is ENOMEM, which happens when
this node does not have a 'reg' property, and this is also the one case we
want to do the failover. So all should be well.
The ideal working case is usually not much of a problem. When I look at what
device_node_to_regmap does, I find, appart from -ENOMEM, possibilities of
-ENOENT (because no clock), and the clock may theoretically fail to prepare
for numerous reasons hidden in clock drivers, but the clock core can
trigger at least -EACCES and -EINPROGRESS via runtime PM.
And it definitely looks like the -EPROBE_DEFER case needs to be addressed.
I.e., why is this call chain not a problem?
mux_mmio_probe
->device_node_to_regmap
-> device_node_get_regmap
-> of_syscon_register
-> of_hwspin_lock_get_id
<- -EPROBE_DEFER
<- ERR_PTR(-EPROBE_DEFER)
<- ERR_PTR(-EPROBE_DEFER)
<- ERR_PTR(-EPROBE_DEFER)
As far as I can tell, if device_node_to_regmap() fails with -EPROBE_DEFER
with your patch, then mux_mmio_probe() misbehaves. It should have aborted
and failed with -EPROBE_DEFER, but instead throws that error away and
goes on to try dev_get_regmap(). That, in turn, is probably futile and
will likely error out in some way, breaking a system that might have been
ok, if the probe had been retried some time later.
This is why I liked the v1 version, dev_get_regmap() just returns a
simple NULL on error, no complex EPROBE_DEFER oddness :)
So is EPROBE_DEFER the only one we think should retry and not go
down the fallback path? I believe that is the normal assumption
for most drivers.
As long as the above is not sufficiently explained away, or fixed, I
consider the patch broken.
How much badness can be caused if syscon_node_to_regmap() fails for some
random obscure reason and the failover path is taken inadvertently? It
certainly smells bad for -EDEFER_PROBE, but do you have any insight in
other cases?
If we take the failover inadvertently then we will check if the parent
node is a syscon, if it is then our offset will most likely be wrong
(parent will not match child 'reg').
And after getting to approx that point a while back, I had other things
to take care of, and this fell off the table. Sorry!
No problem as long as we can find a way to get this in quickly (lot of
DT warning need cleaned up based on this patch).
Hold your horses, I need the above explanation first (and perhaps an
updated patch).
I'm not normally so impatient but this went two whole kernel cycles without
any comment until rc6.. v4 on the way.
Andrew
Cheers,
Peter
Thanks
Andrew
Cheers,
Peter
+ if (IS_ERR(regmap))
+ regmap = dev_get_regmap(dev->parent, NULL) ?: ERR_PTR(-ENODEV);
+ }
if (IS_ERR(regmap)) {
ret = PTR_ERR(regmap);
dev_err(dev, "failed to get regmap: %d\n", ret);
