Messages in this thread | | | Date | Mon, 19 Dec 2022 14:38:17 +0200 | From | Vladimir Oltean <> | Subject | Re: [Patch net-next v4 05/13] net: dsa: microchip: ptp: enable interrupt for timestamping |
| |
On Mon, Dec 12, 2022 at 03:56:31PM +0530, Arun Ramadoss wrote: > +int ksz_ptp_irq_setup(struct dsa_switch *ds, u8 p) > +{ > + struct ksz_device *dev = ds->priv; > + const struct ksz_dev_ops *ops = dev->dev_ops; > + struct ksz_port *port = &dev->ports[p]; > + struct ksz_irq *ptpirq = &port->ptpirq; > + int ret; > + > + ptpirq->dev = dev; > + ptpirq->masked = 0; > + ptpirq->nirqs = 3; > + ptpirq->reg_mask = ops->get_port_addr(p, REG_PTP_PORT_TX_INT_ENABLE__2); > + ptpirq->reg_status = ops->get_port_addr(p, > + REG_PTP_PORT_TX_INT_STATUS__2); > + snprintf(ptpirq->name, sizeof(ptpirq->name), "ptp_irq-%d", p); > + > + ptpirq->irq_num = irq_find_mapping(port->pirq.domain, PORT_SRC_PTP_INT); > + if (ptpirq->irq_num < 0) > + return ptpirq->irq_num; > + > + ptpirq->domain = irq_domain_add_simple(dev->dev->of_node, ptpirq->nirqs, > + 0, &ksz_ptp_irq_domain_ops, > + ptpirq);
If I look at Documentation/core-api/irq/irq-domain.rst, I read this:
| Legacy | ------ | | :: | | irq_domain_add_simple() | irq_domain_add_legacy() | irq_domain_create_simple() | irq_domain_create_legacy() | | The Legacy mapping is a special case for drivers that already have a | range of irq_descs allocated for the hwirqs. It is used when the | driver cannot be immediately converted to use the linear mapping. For | example, many embedded system board support files use a set of #defines | for IRQ numbers that are passed to struct device registrations. In that | case the Linux IRQ numbers cannot be dynamically assigned and the legacy | mapping should be used. | | As the name implies, the \*_legacy() functions are deprecated and only | exist to ease the support of ancient platforms. No new users should be | added. Same goes for the \*_simple() functions when their use results | in the legacy behaviour. | | The legacy map assumes a contiguous range of IRQ numbers has already | been allocated for the controller and that the IRQ number can be | calculated by adding a fixed offset to the hwirq number, and | visa-versa. The disadvantage is that it requires the interrupt | controller to manage IRQ allocations and it requires an irq_desc to be | allocated for every hwirq, even if it is unused. | | The legacy map should only be used if fixed IRQ mappings must be | supported. For example, ISA controllers would use the legacy map for | mapping Linux IRQs 0-15 so that existing ISA drivers get the correct IRQ | numbers. | | Most users of legacy mappings should use irq_domain_add_simple() or | irq_domain_create_simple() which will use a legacy domain only if an IRQ range | is supplied by the system and will otherwise use a linear domain mapping. | The semantics of this call are such that if an IRQ range is specified then | descriptors will be allocated on-the-fly for it, and if no range is | specified it will fall through to irq_domain_add_linear() or | irq_domain_create_linear() which means *no* irq descriptors will be allocated.
I think you should be absolutely fine with using irq_domain_add_linear().
> + if (!ptpirq->domain) > + return -ENOMEM; > + > + ret = request_threaded_irq(ptpirq->irq_num, NULL, ksz_ptp_irq_thread_fn, > + IRQF_ONESHOT, ptpirq->name, ptpirq); > + if (ret) > + goto out; > + > + ret = ksz_ptp_msg_irq_setup(port); > + if (ret) > + goto out;
I think the error path is a bit shaky. Since request_threaded_irq() is not the devres variant (devm_request_threaded_irq()), there should be a free_irq() call on error.
> + > + return 0; > + > +out: > + irq_dispose_mapping(ptpirq->irq_num); > + > + return ret; > +}
| |