lkml.org 
[lkml]   [2011]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/6] drivers/base: add bus for System-on-Chip devices
    Hi Lee,

    I'll have a proper look and test at this later, but it looks pretty good
    to me, so just a couple of minor nits.

    Jamie

    On Mon, Oct 17, 2011 at 12:52:54PM +0100, Lee Jones wrote:
    [...]
    > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
    > new file mode 100644
    > index 0000000..1578e4e
    > --- /dev/null
    > +++ b/drivers/base/soc.c
    > @@ -0,0 +1,115 @@
    > +/*
    > + * Copyright (C) ST-Ericsson SA 2011
    > + *
    > + * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
    > + * License terms: GNU General Public License (GPL), version 2
    > + */
    > +
    > +#include <linux/sysfs.h>
    > +#include <linux/module.h>
    > +#include <linux/init.h>
    > +#include <linux/stat.h>
    > +#include <linux/slab.h>
    > +#include <linux/sys_soc.h>
    > +#include <linux/err.h>
    > +
    > +static ssize_t soc_info_get(struct device *dev,
    > + struct device_attribute *attr,
    > + char *buf);
    > +
    > +static DEVICE_ATTR(machine, S_IRUGO, soc_info_get, NULL);
    > +static DEVICE_ATTR(family, S_IRUGO, soc_info_get, NULL);
    > +static DEVICE_ATTR(soc_id, S_IRUGO, soc_info_get, NULL);
    > +static DEVICE_ATTR(revision, S_IRUGO, soc_info_get, NULL);
    > +
    > +static ssize_t soc_info_get(struct device *dev,
    > + struct device_attribute *attr,
    > + char *buf)
    > +{
    > + struct soc_device *soc_dev =
    > + container_of(dev, struct soc_device, dev);
    > +
    > + if (attr == &dev_attr_machine)
    > + return sprintf(buf, "%s\n", soc_dev->attr->machine);
    > + if (attr == &dev_attr_family)
    > + return sprintf(buf, "%s\n", soc_dev->attr->family);
    > + if (attr == &dev_attr_revision)
    > + return sprintf(buf, "%s\n", soc_dev->attr->revision);
    > + if (attr == &dev_attr_soc_id)
    > + return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
    > +
    > + return -EINVAL;
    > +
    > +}
    > +
    > +struct bus_type soc_bus_type = {
    > + .name = "soc",
    > +};

    static?

    > +
    > +static int __init soc_bus_register(void)
    > +{
    > + return bus_register(&soc_bus_type);
    > +}
    > +core_initcall(soc_bus_register);
    > +
    > +struct attribute *soc_attr[] = {
    > + &dev_attr_machine.attr,
    > + &dev_attr_family.attr,
    > + &dev_attr_soc_id.attr,
    > + &dev_attr_revision.attr,
    > + NULL,
    > +};

    static?

    > +
    > +struct attribute_group soc_attr_group = {
    > + .attrs = soc_attr,
    > +};

    static const?

    > +
    > +struct device *soc_device_register(struct soc_device_attribute *soc_dev_attr)
    > +{
    > + struct soc_device *soc_dev;
    > + static atomic_t soc_device_num = ATOMIC_INIT(0);
    > + int ret;
    > +
    > + soc_dev = kzalloc(sizeof(*soc_dev), GFP_KERNEL);
    > + if (!soc_dev)
    > + return ERR_PTR(-ENOMEM);
    > +
    > + soc_dev->attr = soc_dev_attr;
    > + soc_dev->dev.bus = &soc_bus_type;
    > +
    > + dev_set_name(&soc_dev->dev, "soc%d",
    > + atomic_inc_return(&soc_device_num) - 1);
    > +
    > + ret = device_register(&soc_dev->dev);
    > + if (ret)
    > + goto out1;
    > +
    > + ret = sysfs_create_group(&soc_dev->dev.kobj, &soc_attr_group);
    > + if (ret)
    > + goto out2;

    If you create an array of struct attribute_group's and set
    soc_dev->dev.groups to that then the driver core will handle creating
    the attributes for you and removing them at cleanup. It also means the
    attributes will get created at device creation time so there isn't a
    potential race with udev events looking for attributes that haven't been
    created yet.

    > +
    > + return &soc_dev->dev;
    > +
    > +out2:
    > + device_unregister(&soc_dev->dev);
    > +out1:
    > + kfree(soc_dev);
    > + kfree(soc_dev->attr);
    > + return ERR_PTR(ret);
    > +}
    > +
    > +void soc_device_unregister(struct device *dev)
    > +{
    > + struct soc_device *soc_dev =
    > + container_of(dev, struct soc_device, dev);
    > +
    > + sysfs_remove_group(&dev->kobj, &soc_attr_group);
    > +
    > + if (device_is_registered(dev))
    > + device_unregister(dev);

    I don't think you need this - I think it's fine to say that a
    precondition of calling this is that the device is registered (that's
    what the platform_device layer does).

    > +
    > + bus_unregister(&soc_bus_type);

    I don't think this is quite right - we're freeing the bus on the first
    unregister and we won't be able to register any more soc devices. As
    this can't sensibly be a loadable module I think it's fine to leave it
    always registered.

    > +
    > + kfree(soc_dev->attr);
    > + kfree(soc_dev);

    device_unregister() should do this for you. The device could still be
    hanging around after device_unregister() if there are active references
    to it so we don't want to free the memory too early. We should have a
    release function in the struct device set in soc_device_register that
    frees the struct soc_device.

    > +}
    > diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
    > new file mode 100644
    > index 0000000..2485a0f2
    > --- /dev/null
    > +++ b/include/linux/sys_soc.h
    > @@ -0,0 +1,38 @@
    > +/*
    > + * Copyright (C) ST-Ericsson SA 2011
    > + * Author: Lee Jones <lee.jones@linaro.org> for ST-Ericsson.
    > + * License terms: GNU General Public License (GPL), version 2
    > + */
    > +#ifndef __SOC_BUS_H
    > +#define __SOC_BUS_H
    > +
    > +#include <linux/kobject.h>
    > +#include <linux/device.h>
    > +#include <linux/platform_device.h>

    I'm not sure you need kobject.h and platform_device.h here.

    > +
    > +struct soc_device_attribute {
    > + const char *machine;
    > + const char *family;
    > + const char *revision;
    > + const char *soc_id;
    > +};
    > +
    > +struct soc_device {
    > + struct device dev;
    > + struct soc_device_attribute *attr;
    > +};

    This can be private to the implementation file.

    > +
    > +/**
    > + * soc_device_register - register SoC as a device
    > + * @soc_plat_dev_attr: Attributes passed from platform to be attributed to a SoC
    > + */
    > +struct device *soc_device_register(
    > + struct soc_device_attribute *soc_plat_dev_attr);
    > +
    > +/**
    > + * soc_device_unregister - unregister SoC device
    > + * @dev: SoC device to be unregistered
    > + */
    > +void soc_device_unregister(struct device *dev);
    > +
    > +#endif /* __SOC_BUS_H */
    > --
    > 1.7.5.4
    >


    \
     
     \ /
      Last update: 2011-10-17 14:15    [from the cache]
    ©2003-2014 Jasper Spaans. Advertise on this site