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:   Tue, 13 Jun 2023 17:47:42 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Jianhui Zhao <zhaojh329@gmail.com>, hkallweit1@gmail.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2] net: phy: Add sysfs attribute for PHY c45 identifiers.
Message-ID: <ZIidrqudxBaOva90@shell.armlinux.org.uk>
References: <20230613143025.111844-1-zhaojh329@gmail.com>
 <612d5964-6034-4188-8da5-53f3f38a25e4@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <612d5964-6034-4188-8da5-53f3f38a25e4@lunn.ch>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Tue, Jun 13, 2023 at 06:18:58PM +0200, Andrew Lunn wrote:
> > +#define DEVICE_ATTR_C45_ID(i) \
> > +static ssize_t \
> > +phy_c45_id##i##_show(struct device *dev, \
> > +	struct device_attribute *attr, char *buf) \
> > +{ \
> > +	struct phy_device *phydev = to_phy_device(dev); \
> > +\
> > +	if (!phydev->is_c45) \
> > +		return 0; \
> > +\
> > +	return sprintf(buf, "0x%.8lx\n", \
> > +		(unsigned long)phydev->c45_ids.device_ids[i]); \
> > +} \
> 
> That is not the most efficient implementation.
> 
> You can have one generic
> 
> static ssize_t phy_c45_id_show(struct device *dev, char *buf, int i)
> {
> 	struct phy_device *phydev = to_phy_device(dev);
> 
> 	if (!phydev->is_c45)
> 		return 0;
> 
> 	return sprintf(buf, "0x%.8lx\n",
> 		      (unsigned long)phydev->c45_ids.device_ids[i]);
> }
> 
> And then your macros becomes
> 
> #define DEVICE_ATTR_C45_ID(i)			  \
> static ssize_t					  \
> phy_c45_id##i##_show(struct device *dev,	  \
> 	struct device_attribute *attr, char *buf) \
> {						  \
> 	return phy_c45_id_show(dev, buf, i);	  \
> }
> 

I have a further suggestion, which I think will result in yet more
efficiencies:

struct phy_c45_devid_attribute {
	struct device_attribute attr;
	int index;
};

#define to_phy_c45_devid_attr(attr) \
	container_of(attr, struct phy_c45_devid_attribute, attr)

static ssize_t phy_c45_id_show(struct device *dev,
			       struct device_attribute *attr, char *buf)
{
	struct phy_c45_devid_attribute *devattr = to_phy_c45_devid_attr(attr);
	struct phy_device *phydev = to_phy_device(dev);
	unsigned long id;

	if (!phydev->is_c45)
		return 0;

	id = phydev->c45_ids.device_ids[devattr->index];

	return sprintf(buf, "0x%.8lx\n", id);
}

#define DEVICE_ATTR_C45_ID(i) \
static struct phy_c45_devid_attribute dev_attr_phy_c45_id##i = { \
	.attr = { \
		.attr   = { .name = __stringify(mmd##i), .mode = 0444 }, \
		.show   = phy_c45_id_show \
	}, \
	.index = i, \
}

which will probably result in less code size for a little larger data
size. Note that the references to this would need to be:

	&dev_attr_phy_c45_id1.attr.attr

in the array (adding an extra .attr).

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