On 10/28/2016 11:54 AM, [email protected] wrote:
> From: Woojung Huh <[email protected]>
>
> To utilize phylib with interrupt fully than handling some of phy stuff in the
> MAC driver,
> create irq_domain for USB interrupt EP of phy interrupt and
> pass the irq number to phy_connect_direct() instead of PHY_IGNORE_INTERRUPT.
>
> Idea comes from drivers/gpio/gpio-dl2.c
>
> Signed-off-by: Woojung Huh <[email protected]>
> ---
> +static void lan78xx_irq_mask(struct irq_data *irqd)
> +{
> + struct irq_domain_data *data = irq_data_get_irq_chip_data(irqd);
> + struct lan78xx_net *dev =
> + container_of(data, struct lan78xx_net, domain_data);
> + u32 buf;
> +
> + lan78xx_read_reg(dev, INT_EP_CTL, &buf);
lan78xx_read_reg() uses kmalloc() with GFP_KERNEL, while
irq_mask/irq_unmask can be called in atomic context AFAIR, you may need
to pass down a specific gfp_t to lan78xx_read_reg.
What about usb_submit_urb(), can that work in atomic context? Do you
need to have lan78xx_read_reg() acquire a raw spinlock or something to
serialize them?
> + buf &= ~INT_EP_PHY_INT_EN_;
Even though you may have only one interrupt line to deal with at the
moment, better make this bit derived from irqd->hwirq instead of hard
coding it here.
> + lan78xx_write_reg(dev, INT_EP_CTL, buf);
> +}
> +
> +static void lan78xx_irq_unmask(struct irq_data *irqd)
> +{
> + struct irq_domain_data *data = irq_data_get_irq_chip_data(irqd);
> + struct lan78xx_net *dev =
> + container_of(data, struct lan78xx_net, domain_data);
> + u32 buf;
> +
> + lan78xx_read_reg(dev, INT_EP_CTL, &buf);
> + buf |= INT_EP_PHY_INT_EN_;
Same here, this should come from irqd->hwirq.
> + lan78xx_write_reg(dev, INT_EP_CTL, buf);
> +}
> +
> +static struct irq_chip lan78xx_irqchip = {
> + .name = "lan78xx-phyirq",
> + .irq_mask = lan78xx_irq_mask,
> + .irq_unmask = lan78xx_irq_unmask,
> +};
> +
> +static int lan78xx_setup_irq_domain(struct lan78xx_net *dev)
> +{
> + struct device_node *of_node;
> + struct irq_domain *irqdomain;
> + unsigned int irq_base = 0;
> + int ret = 0;
> +
> + of_node = dev->udev->dev.parent->of_node;
> +
> + dev->domain_data.irqchip = &lan78xx_irqchip;
> + dev->domain_data.irq_handler = handle_simple_irq;
> +
> + irqdomain = irq_domain_add_simple(of_node, 1, 0, &chip_domain_ops,
> + &dev->domain_data);
Is there really just one interrupt associated with this peripheral here?
>
> + if (lan78xx_setup_irq_domain(dev) < 0) {
> + netdev_warn(dev->net, "lan78xx_setup_irq_domain() failed");
> + return -EIO;
> + }
Any reason not to propagate the error code from
lan78xx_setup_irq_domain() here?
--
Florian
