Re: [PATCH v3 3/3] spmi: spmi-pmic-arb: add support for PMIC arbiter v8

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 11/1/25 3:22 AM, Jishnu Prakash wrote:
> Hi Konrad,
> 
> On 10/27/2025 5:47 PM, Konrad Dybcio wrote:
>> On 10/24/25 11:33 AM, Jishnu Prakash wrote:
>>> From: David Collins <david.collins@oss.qualcomm.com>
>>>
>>> PMIC arbiter v8 supports up to 4 SPMI buses and up to 8192 PMIC
>>> peripherals.  Its register map differs from v7 as several fields
>>> increased in size. Add support for PMIC arbiter version 8.
>>>
>>> Signed-off-by: David Collins <david.collins@oss.qualcomm.com>
>>> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
>>> Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
>>> ---

[...]

>>> +static int pmic_arb_get_bus_resources_v8(struct platform_device *pdev,
>>> +					 struct device_node *node,
>>> +					 struct spmi_pmic_arb_bus *bus)
>>> +{
>>> +	int index;
>>> +
>>> +	index = of_property_match_string(node, "reg-names", "chnl_owner");
>>> +	if (index < 0) {
>>> +		dev_err(&pdev->dev, "chnl_owner reg region missing\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	bus->apid_owner = devm_of_iomap(&pdev->dev, node, index, NULL);
>>> +
>>> +	return PTR_ERR_OR_ZERO(bus->apid_owner);
>>
>> Is this any different chan using devm_platform_ioremap_resource_byname()
>> like you did above?
> 
> devm_platform_ioremap_resource_byname() would work for mapping resources
> directly under the main device node, like"chnl_map" , but in this case, the
> resource "chnl_owner" is under a child node of the the main node, so we
> need to use devm_of_iomap() here.

Oh, you're right

[...]

>>> +	apidd = &bus->apid_data[bus->base_apid];
>>> +	apid_max = bus->base_apid + bus->apid_count;
>>> +	for (i = bus->base_apid; i < apid_max; i++, apidd++) {
>>> +		offset = pmic_arb->ver_ops->apid_map_offset(i);
>>> +		regval = readl_relaxed(pmic_arb->apid_map + offset);
>>> +		if (!regval)
>>> +			continue;
>>> +		ppid = regval & PMIC_ARB_V8_PPID_MASK;
>>> +		is_irq_ee = PMIC_ARB_V8_CHAN_IS_IRQ_OWNER(regval);
>>
>> [...]
>>
>>
>> If you parametrize the masks, the diff against pmic_arb_read_apid_map_v5
>> is 3 lines of code instead
> 
> Are you suggesting we should try to have a common helper function for v5/v7
> and v8, which does the bulk of these actions and can be given different
> input arguments to distinguish the versions?
> 
> There is at least one more difference which I don't think we can easily
> accommodate with parameters:
> 
> In pmic_arb_read_apid_map_v5(), we have:
> regval = readl_relaxed(pmic_arb->core + offset);
> 
> In pmic_arb_read_apid_map_v8(), we have:
> regval = readl_relaxed(pmic_arb->apid_map + offset);

You can make the function accept this region base as a parameter, rename
it to something like _pmic_arb_ppid_to_apid_v5()..

> But in any case, we would have to add `if` checks to distinguish arbiter
> versions to use a helper function. This itself might not be a good idea as it
> would break the abstraction already implemented in the probe, by having
> PMIC version checking happen only in the probe, to select the correct set
> of ver_ops functions for a particular version.

..and then create a much thinner pmic_arb_ppid_to_apid_v5/v8() wrappers
that will call _pmic_arb_ppid_to_apid_v5()

>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int pmic_arb_init_apid_v8(struct spmi_pmic_arb_bus *bus, int index)
>>> +{
>>> +	struct spmi_pmic_arb *pmic_arb = bus->pmic_arb;
>>> +	int ret, i;
>>> +
>>> +	if (index < 0 || index >= PMIC_ARB_MAX_BUSES_V8) {
>>> +		dev_err(&bus->spmic->dev, "Unsupported bus index %d detected\n",
>>> +			index);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	bus->base_apid = 0;
>>> +	bus->apid_count = 0;
>>> +	for (i = 0; i <= index; i++) {
>>> +		bus->base_apid += bus->apid_count;
>>> +		bus->apid_count = readl_relaxed(pmic_arb->core + PMIC_ARB_FEATURES + i * 4) &
>>> +						PMIC_ARB_FEATURES_V8_PERIPH_MASK;
>>
>> You can almost replace pmic_arb_init_apid_v7 with this impl
> 
> They are not exactly the same - v7 arbiter supports at most 2 buses,
> so pmic_arb_init_apid_v7 needs to restrict the max value of index to 2.
> 
> Here too, using a common helper function would require us to add more
> `if` checks for the version, which leads to the same kind of issue as above.

Add pmic_arb_ver_ops.max_buses, set it to 1/2/1234 as needed. I really
don't want you to add a copy of an existing function with a single
comparison argument changed

>> [...]
>>
>>> +static void __iomem *
>>> +pmic_arb_apid_owner_v8(struct spmi_pmic_arb_bus *bus, u16 n)
>>> +{
>>> +	return bus->apid_owner + 0x4 * (n - bus->base_apid);
>>> +}
>>
>> This is identical to pmic_arb_apid_owner_v7
> 
> This is not exactly right, pmic_arb_apid_owner_v7 uses bus->cnfg
> and pmic_arb_apid_owner_v8 uses bus->apid_owner at the same place.
> They are both already single line functions, would it really help
> to try to simplify them further?

Sorry, I misread and thought they were *actually the same*, scratch
that

Konrad