Hi,
I thought I'd look into converting the MMC sysfs code to use the new
bus_type->dev_attrs pointer. However, it doesn't appear that enough
thought has been put into how this should work.
1. include/linux/device.h, macro for creating device attributes:
#define DEVICE_ATTR(_name,_mode,_show,_store) \
struct device_attribute dev_attr_##_name = __ATTR(_name,_mode,_show,_store)
2. include/linux/device.h, bus_type definition:
struct bus_type {
...
struct device_attribute * dev_attrs;
...
};
3. device_add_attrs(), the code which adds the attributes to a device:
if (bus->dev_attrs) {
for (i = 0; attr_name(bus->dev_attrs[i]); i++) {
error = device_create_file(dev,&bus->dev_attrs[i]);
if (error)
goto Err;
}
}
The crux of the problem:
- As can be seen from (3) and (2), we expect dev_attrs to point to an
array of at least two struct device_attributes. This is incompatible
with (1).
Example of the problem:
- MMC code can do this:
#define MMC_ATTR(name, fmt, args...) \
static ssize_t mmc_dev_show_##name (struct device *dev, char *buf) \
{ \
struct mmc_card *card = dev_to_mmc_card(dev); \
return sprintf(buf, fmt, args); \
} \
static DEVICE_ATTR(name, S_IRUGO, mmc_dev_show_##name, NULL)
MMC_ATTR(cid, "%08x %08x %08x %08x\n",
card->raw_cid[0], card->raw_cid[1],
card->raw_cid[2], card->raw_cid[3]);
MMC_ATTR(csd, "%08x %08x %08x %08x\n",
card->raw_csd[0], card->raw_csd[1],
card->raw_csd[2], card->raw_csd[3]);
MMC_ATTR(date, "%02d/%04d\n", card->cid.month, card->cid.year);
MMC_ATTR(fwrev, "0x%x\n", card->cid.fwrev);
MMC_ATTR(hwrev, "0x%x\n", card->cid.hwrev);
MMC_ATTR(manfid, "0x%06x\n", card->cid.manfid);
MMC_ATTR(name, "%s\n", card->cid.prod_name);
MMC_ATTR(oemid, "0x%02x\n", card->cid.oemid);
MMC_ATTR(serial, "0x%08x\n", card->cid.serial);
static struct device_attribute *mmc_dev_attributes[] = {
&dev_attr_cid,
&dev_attr_csd,
&dev_attr_date,
&dev_attr_fwrev,
&dev_attr_hwrev,
&dev_attr_manfid,
&dev_attr_name,
&dev_attr_oemid,
&dev_attr_serial,
};
and handle the array of mmc_dev_attributes itself. However, converting
this to a suitable form to allow it to be poked into bus_type->dev_attrs
makes this:
#define MMC_ATTR(name, fmt, args...) \
static ssize_t mmc_dev_show_##name (struct device *dev, char *buf) \
{ \
struct mmc_card *card = dev_to_mmc_card(dev); \
return sprintf(buf, fmt, args); \
}
MMC_ATTR(cid, "%08x %08x %08x %08x\n",
card->raw_cid[0], card->raw_cid[1],
card->raw_cid[2], card->raw_cid[3]);
MMC_ATTR(csd, "%08x %08x %08x %08x\n",
card->raw_csd[0], card->raw_csd[1],
card->raw_csd[2], card->raw_csd[3]);
MMC_ATTR(date, "%02d/%04d\n", card->cid.month, card->cid.year);
MMC_ATTR(fwrev, "0x%x\n", card->cid.fwrev);
MMC_ATTR(hwrev, "0x%x\n", card->cid.hwrev);
MMC_ATTR(manfid, "0x%06x\n", card->cid.manfid);
MMC_ATTR(name, "%s\n", card->cid.prod_name);
MMC_ATTR(oemid, "0x%02x\n", card->cid.oemid);
MMC_ATTR(serial, "0x%08x\n", card->cid.serial);
static struct device_attributes mmc_dev_attrs[] = {
{
{
.name = "cid",
.owner = THIS_MODULE,
.mode = S_IRUGO,
},
.show = mmc_dev_show_cid,
}, {
{
.name = "csd",
.owner = THIS_MODULE,
.mode = S_IRUGO,
},
.show = mmc_dev_show_csd,
}, {
{
.name = "date",
.owner = THIS_MODULE,
.mode = S_IRUGO,
},
.show = mmc_dev_show_date,
}, ... etc ...
};
Hardly elegant, hardly clean, and hardly foolproof from silly C'n'P
errors.
Can we please convert the attribute stuff to something a little saner
so the existing macros can still be used?
I don't think anyone uses this yet, so now would be an opportune
moment to fix this.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/
2.6 Serial core
On Saturday 18 September 2004 09:56 am, Russell King wrote:
>
> static struct device_attributes mmc_dev_attrs[] = {
> {
> {
> .name = "cid",
> .owner = THIS_MODULE,
> .mode = S_IRUGO,
> },
> .show = mmc_dev_show_cid,
> }, {
> {
> .name = "csd",
> .owner = THIS_MODULE,
> .mode = S_IRUGO,
> },
> .show = mmc_dev_show_csd,
> }, {
> {
> .name = "date",
> .owner = THIS_MODULE,
> .mode = S_IRUGO,
> },
> .show = mmc_dev_show_date,
> }, ... etc ...
> };
>
> Hardly elegant, hardly clean, and hardly foolproof from silly C'n'P
> errors.
>
What's wrong with the following (drivers/input/serio/serio.c):
static struct device_attribute serio_device_attrs[] = {
__ATTR(description, S_IRUGO, serio_show_description, NULL),
__ATTR(driver, S_IWUSR | S_IRUGO, serio_show_driver, serio_rebind_driver),
__ATTR(bind_mode, S_IWUSR | S_IRUGO, serio_show_bind_mode, serio_set_bind_mode),
__ATTR_NULL
};
Pretty compact and expressive IMHO.
--
Dmitry
On Sat, Sep 18, 2004 at 10:43:41AM -0500, Dmitry Torokhov wrote:
> What's wrong with the following (drivers/input/serio/serio.c):
>
> static struct device_attribute serio_device_attrs[] = {
> __ATTR(description, S_IRUGO, serio_show_description, NULL),
You should use __ATTR_RO() for that one :)
> __ATTR(driver, S_IWUSR | S_IRUGO, serio_show_driver, serio_rebind_driver),
> __ATTR(bind_mode, S_IWUSR | S_IRUGO, serio_show_bind_mode, serio_set_bind_mode),
> __ATTR_NULL
> };
>
> Pretty compact and expressive IMHO.
Yes, that's the way to currently do it. Russell, is that acceptable?
thanks,
greg k-h