Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 14 Sep 2022 12:27:39 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Arun Ramadoss <arun.ramadoss@microchip.com>
Cc:     linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        woojung.huh@microchip.com, UNGLinuxDriver@microchip.com,
        andrew@lunn.ch, vivien.didelot@gmail.com, f.fainelli@gmail.com,
        olteanv@gmail.com, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, Tristram.Ha@microchip.com,
        prasanna.vengateshan@microchip.com, hkallweit1@gmail.com
Subject: Re: [Patch net-next v2 4/5] net: dsa: microchip: move interrupt
 handling logic from lan937x to ksz_common
Message-ID: <YyG6q6SPURSAtz7C@shell.armlinux.org.uk>
References: <20220914035223.31702-1-arun.ramadoss@microchip.com>
 <20220914035223.31702-5-arun.ramadoss@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220914035223.31702-5-arun.ramadoss@microchip.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

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]);

?

> +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);

?

> +}
> +
> +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.

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