Re: [PATCH v3] i2c: mux: reg: use device property accessors

[Abdurrahman Hussain]

On Mon May 18, 2026 at 1:15 PM PDT, Abdurrahman Hussain wrote:
> On Mon May 18, 2026 at 6:59 AM PDT, Peter Rosin wrote:
>>
>> Hi!
>>
>> After reading v2/v3, I finally got why you removed if (!mux->data.reg).
>> I.e., since you removed the io-remapping from i2c_mux_reg_probe_fw()
>> that part can now be unconditional in i2c_mux_reg_probe(). But,
>> keeping the if just to accomodate potential out-of-tree code that uses
>> the platform data hooking is not needed so v1 feels better than v2/v3.
>> Sorry for being dense. However, see below.
>>
>
> Agreed -- v4 will drop the guard and make
> devm_platform_get_and_ioremap_resource() unconditional.
>
> Follow-up question while we're pruning the platdata path: with the
> guard gone, the .reg and .reg_size fields in
> struct i2c_mux_reg_platform_data are functionally dead (any value a
> platdata user supplied through them is overwritten by
> devm_platform_get_and_ioremap_resource()).  Should I drop those two
> fields from the struct in the same patch, or leave them as silent
> no-ops in the public header?  Happy either way.
>

Scratch that, sorry -- right after sending I noticed
drivers/platform/mellanox/mlx-platform.c is an in-tree platdata
user.  It registers i2c-mux-reg platform_devices via
platform_device_register_resndata(..., res=NULL, num_res=0, ...)
and supplies a pre-set .reg (a (void __iomem *) cast of an LPC
register address) and .reg_size=1 through the platdata struct
(mlxplat_default_mux_data[] and friends around line 474).

Without the if (!mux->data.reg) guard,
devm_platform_get_and_ioremap_resource() would run unconditionally,
fail because the pdev carries no memory resource, and break
mlx-platform's mux probe.  v4 will therefore keep the guard, and
the .reg / .reg_size fields stay in the public platdata struct.

The other two changes stand:

  - i2c_get_adapter_by_fwnode() -> i2c_find_adapter_by_fwnode() +
    put_device() in probe_fw(), matching the OF original.

  - In probe(), move the if (!mux->data.reg) ioremap block --
    together with the reg_size validation that depends on it --
    above the i2c_get_adapter() call.  fwnode users get back the
    master-side ordering of "ioremap before parent-adapter get";
    platdata users see the size check run a bit earlier, but
    mux->data.reg_size is set from platdata by then, so it's
    order-neutral for them.

Cheers,
Abdurrahman

>>> -	adapter = of_find_i2c_adapter_by_node(adapter_np);
>>> -	of_node_put(adapter_np);
>>> +
>>> +	adapter = i2c_get_adapter_by_fwnode(fwnode);
>>> +	fwnode_handle_put(fwnode);
>>
>> Why did you not change to i2c_find_adapter_by_fwnode()? It would be
>> closer to the original, but maybe that doesn't work for ACPI for some
>> reason?
>>
>
> It works for ACPI -- both _find and _get bottom out in
> i2c_find_adapter_by_fwnode().  My reason for picking the _get
> variant was simply that it does try_module_get(adapter->owner) on
> top.  But i2c_get_adapter() further down in probe() already does
> the same try_module_get() and -EPROBE_DEFERs on failure, so the
> extra acquire-then-immediately-release in probe_fw() isn't earning
> us anything.  v4 will switch to i2c_find_adapter_by_fwnode() +
> put_device(), matching the OF original more closely.
>
>>> -	/* map address from "reg" if exists */
>>> -	if (of_address_to_resource(np, 0, &res) == 0) {
>>> -		mux->data.reg_size = resource_size(&res);
>>> -		mux->data.reg = devm_ioremap_resource(&pdev->dev, &res);
>>> -		if (IS_ERR(mux->data.reg))
>>> -			return PTR_ERR(mux->data.reg);
>>> -	}
>>> -
>>
>> By not doing the io-remapping here, the ordering is now subtly altered.
>> Previously we had io-remapping before the call to i2c_get_adapter().
>> With this change we instead have the call to i2c_get_adapter() before
>> the io-remapping.
>>
>> Since it is apparently difficult the get this patch tested for the
>> existing use cases, we should avoid as many of these innocent looking
>> but subtle changes as possible.
>>
>> Cheers,
>> Peter
>>
>
> Will fix that in v4 by reordering probe() so the
> devm_platform_get_and_ioremap_resource() call comes before
> i2c_get_adapter(), matching the master sequence.  No need to push
> the ioremap back into probe_fw() -- the helper signature stays
> cleaner if probe() owns the call.
>
> Thanks for the careful re-read.
>
> Cheers,
> Abdurrahman