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,