Re: [Patch net-next v2 4/5] net: dsa: microchip: move interrupt handling logic from lan937x to ksz_common

[Arun.Ramadoss]

Hi Russel,
Thanks for the comment.

On Wed, 2022-09-14 at 12:27 +0100, Russell King (Oracle) wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Hi,
> 
> Some suggestions for a few improvements in a future patch:
> 
> On Wed, Sep 14, 2022 at 09:22:22AM +0530, Arun Ramadoss wrote:
> > +static int ksz_irq_phy_setup(struct ksz_device *dev)
> > +{
> > +     struct dsa_switch *ds = dev->ds;
> > +     int phy, err_phy;
> > +     int irq;
> > +     int ret;
> > +
> > +     for (phy = 0; phy < KSZ_MAX_NUM_PORTS; phy++) {
> > +             if (BIT(phy) & ds->phys_mii_mask) {
> > +                     irq = irq_find_mapping(dev-
> > >ports[phy].pirq.domain,
> > +                                            PORT_SRC_PHY_INT);
> > +                     if (irq < 0) {
> > +                             ret = irq;
> > +                             goto out;
> > +                     }
> > +                     ds->slave_mii_bus->irq[phy] = irq;
> > +             }
> > +     }
> > +     return 0;
> > +out:
> > +     err_phy = phy;
> > +
> > +     for (phy = 0; phy < err_phy; phy++)
> > +             if (BIT(phy) & ds->phys_mii_mask)
> > +                     irq_dispose_mapping(ds->slave_mii_bus-
> > >irq[phy]);
> 
>         while (phy--)
>                 if (BIT(phy) & ds->phys_mii_mask)
>                         irq_dispose_mapping(ds->slave_mii_bus-
> >irq[phy]);

Ok. I will update.

> 
> ?
> 
> > +static void ksz_girq_mask(struct irq_data *d)
> > +{
> > +     struct ksz_device *dev = irq_data_get_irq_chip_data(d);
> > +     unsigned int n = d->hwirq;
> > +
> > +     dev->girq.masked |= (1 << n);
> 
>         dev->girq.masked |= BIT(d->hwirq);
> 
> ?
> 
> > +}
> > +
> > +static void ksz_girq_unmask(struct irq_data *d)
> > +{
> > +     struct ksz_device *dev = irq_data_get_irq_chip_data(d);
> > +     unsigned int n = d->hwirq;
> > +
> > +     dev->girq.masked &= ~(1 << n);
> 
>         dev->girq.masked &= ~BIT(d->hw_irq);

Ok. I will replace them with macros.

> 
> ?
> 
> > +}
> > +
> > +static void ksz_girq_bus_lock(struct irq_data *d)
> > +{
> > +     struct ksz_device *dev = irq_data_get_irq_chip_data(d);
> > +
> > +     mutex_lock(&dev->lock_irq);
> > +}
> > +
> > +static void ksz_girq_bus_sync_unlock(struct irq_data *d)
> > +{
> > +     struct ksz_device *dev = irq_data_get_irq_chip_data(d);
> > +     int ret;
> > +
> > +     ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, dev-
> > >girq.masked);
> > +     if (ret)
> > +             dev_err(dev->dev, "failed to change IRQ mask\n");
> > +
> > +     mutex_unlock(&dev->lock_irq);
> > +}
> > +
> > +static const struct irq_chip ksz_girq_chip = {
> > +     .name                   = "ksz-global",
> > +     .irq_mask               = ksz_girq_mask,
> > +     .irq_unmask             = ksz_girq_unmask,
> > +     .irq_bus_lock           = ksz_girq_bus_lock,
> > +     .irq_bus_sync_unlock    = ksz_girq_bus_sync_unlock,
> > +};
> 
> As the pirq code is almost identical to the girq code, how about
> putting
> a "reg_mask", "reg_status" and a pointer to ksz_device into ksz_irq,
> and
> using the ksz_irq as the chip data?
> 
> These would then become:
> 
> static void ksz_irq_mask(struct irq_data *d)
> {
>         struct ksz_irq *ki = irq_data_get_irq_chip_data(d);
> 
>         ki->masked |= BIT(d->hwirq);
> }
> 
> static void ksz_irq_unmask(struct irq_data *d)
> {
>         struct ksz_irq *ki = irq_data_get_irq_chip_data(d);
> 
>         ki->masked &= ~BIT(d->hwirq);
> }
> 
> static void ksz_irq_bus_lock(struct irq_data *d)
> {
>         struct ksz_irq *ki = irq_data_get_irq_chip_data(d);
> 
>         mutex_lock(&ki->dev->lock_irq);
> }
> 
> static void ksz_irq_bus_sync_unlock(struct irq_data *d)
> {
>         struct ksz_irq *ki = irq_data_get_irq_chip_data(d);
>         struct ksz_device *dev = ki->dev;
>         int ret;
> 
>         ret = ksz_write32(dev, ki->reg_masked, ki->masked);
>         if (ret)
>                 dev_err(dev->dev, "failed to change IRQ mask\n");
> 
>         mutex_unlock(&dev->lock_irq);
> }
> 
> and thus this code could be shared between both pirq and girq.
> I'm pretty sure the thead_fn could be shared as well, and I'm
> sure that the setup and tear down could be improved in a similar
> way.

Thanks for the suggestion. I will update the code and send the next
version of the patch.

> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!