Re: [PATCH RFC v2] spmi: pmic-arb: Add support for multiple buses

[Neil Armstrong]

On 13/02/2024 15:41, Abel Vesa wrote:
> The v7 HW supports currently 2 buses. So register each bus as a separate
> spmi controller and adapt all ops to use the bus instead of the
> arbitrator. Legacy mode is still supported as long as there is no child
> node that represents a bus, instead all nodes are expected to be actual
> slave devices.
>
> Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx>
> ---
> Changes in v2:
> - Reworked it so that it registers a spmi controller for each bus
>    rather than relying on the generic framework to pass on the bus
>    (master) id.

Thanks, I think this is better.

> - Link to v1: https://lore.kernel.org/r/20240207-spmi-multi-master-support-v1-0-ce57f301c7fd@xxxxxxxxxx
> ---
>   drivers/spmi/spmi-pmic-arb.c | 950 ++++++++++++++++++++++++++-----------------
>   1 file changed, 585 insertions(+), 365 deletions(-)
>
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 9ed1180fe31f..eced35b712b4 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c

<snip>


> +static int spmi_pmic_arb_bus_init(struct platform_device *pdev,
> +				  struct device_node *node,
> +				  struct spmi_pmic_arb *pmic_arb)
> +{
> +	int bus_index = pmic_arb->buses_available;
> +	struct spmi_pmic_arb_bus *bus = &pmic_arb->buses[bus_index];
> +	struct device *dev = &pdev->dev;
> +	struct spmi_controller *ctrl;
> +	void __iomem *intr;
> +	void __iomem *cnfg;
> +	int index, ret;
> +	u32 irq;
> +
> +	ctrl = devm_spmi_controller_alloc(dev, sizeof(*ctrl));
> +	if (IS_ERR(ctrl))
> +		return PTR_ERR(ctrl);
> +
> +	ctrl->cmd = pmic_arb_cmd;
> +	ctrl->read_cmd = pmic_arb_read_cmd;
> +	ctrl->write_cmd = pmic_arb_write_cmd;
> +
> +	bus = spmi_controller_get_drvdata(ctrl);
> +	bus->spmic = ctrl;
> +
> +	bus->ppid_to_apid = devm_kcalloc(dev, PMIC_ARB_MAX_PPID,
> +					 sizeof(*bus->ppid_to_apid),
> +					 GFP_KERNEL);
> +	if (!bus->ppid_to_apid)
> +		return -ENOMEM;
> +
> +	bus->apid_data = devm_kcalloc(dev, pmic_arb->max_periphs,
> +				      sizeof(*bus->apid_data),
> +				      GFP_KERNEL);
> +	if (!bus->apid_data)
> +		return -ENOMEM;
> +
> +	/* Optional property for v7: */
> +	of_property_read_u32(node, "qcom,bus-id", &bus_index);

Not sure what bindings you plan to use, but this should be the reg property.

> +	if (bus_index != pmic_arb->buses_available) {
> +		dev_err(dev, "wrong bus-id value");
> +		return -EINVAL;
> +	}
> +
> +	index = of_property_match_string(node, "reg-names", "cnfg");
> +	if (index < 0) {
> +		dev_err(dev, "cnfg reg region missing");
> +		return -EINVAL;
> +	}
> +
> +	cnfg = devm_of_iomap(dev, node, index, NULL);
> +	if (IS_ERR(cnfg))
> +		return PTR_ERR(cnfg);
> +
> +	index = of_property_match_string(node, "reg-names", "intr");
> +	if (index < 0) {
> +		dev_err(dev, "intr reg region missing");
> +		return -EINVAL;
> +	}
> +
> +	intr = devm_of_iomap(dev, node, index, NULL);
> +	if (IS_ERR(intr))
> +		return PTR_ERR(intr);
> +
> +	irq = of_irq_get_byname(node, "periph_irq");
> +	if (irq < 0)
> +		return irq;
> +
> +	bus->pmic_arb = pmic_arb;
> +	bus->intr = intr;
> +	bus->cnfg = cnfg;
> +	bus->irq = irq;
> +	bus->id = bus_index;
> +
> +	ret = pmic_arb->ver_ops->init_apid(bus, index);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(&pdev->dev, "adding irq domain for bus %d\n", bus_index);
> +
> +	bus->domain = irq_domain_add_tree(dev->of_node,
> +					  &pmic_arb_irq_domain_ops, bus);
> +	if (!bus->domain) {
> +		dev_err(&pdev->dev, "unable to create irq_domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	irq_set_chained_handler_and_data(bus->irq,
> +					 pmic_arb_chained_irq, bus);
> +
> +	bus->spmic->dev.of_node = node;
> +	dev_set_name(&bus->spmic->dev, "spmi-%d", bus_index);
> +
> +	ret = devm_spmi_controller_add(dev, bus->spmic);
> +	if (ret)
> +		return ret;
> +
> +	pmic_arb->buses_available++;
> +
> +	return 0;
> +}
> +
> +static int spmi_pmic_arb_register_buses(struct spmi_pmic_arb *pmic_arb,
> +					struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct device_node *child;
> +	int ret;
> +
> +	for_each_available_child_of_node(node, child)
> +		if (of_node_name_eq(child, "bus")) {

It seems you use "bus" subnodes, it seems you should also submit a new
bindings scheme for v7 controllers with subnodes

> +			ret = spmi_pmic_arb_bus_init(pdev, child, pmic_arb);
> +			if (ret)
> +				return ret;
> +		}
> +
> +	if (!pmic_arb->buses_available)
> +		ret = spmi_pmic_arb_bus_init(pdev, node, pmic_arb);
> +
> +	return ret;
> +}
> +
> +static void spmi_pmic_arb_deregister_buses(struct spmi_pmic_arb *pmic_arb)
> +{
> +	int i;
> +
> +	for (i = 0; i < pmic_arb->buses_available; i++) {
> +		struct spmi_pmic_arb_bus *bus = &pmic_arb->buses[i];
> +
> +		irq_set_chained_handler_and_data(bus->irq,
> +						 NULL, NULL);
> +		irq_domain_remove(bus->domain);
> +	}
> +}
> +

<snip>

Overall the patch is __huge__, could you split it ? Like move the bus handling
then add the multi-bus support in separate patches ?

But first please add new bindings first so we know what you expect from DT.

Thanks,
Neil

> ---
> base-commit: 445a555e0623387fa9b94e68e61681717e70200a
> change-id: 20240207-spmi-multi-master-support-832a704b779b
>
> Best regards,