Re: [PATCH V2] net: phy: Add sysfs attribute for PHY c45 identifiers.

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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!