hi,

On Thu, Mar 18, 2010 at 07:44:50PM +0100, ext Nishanth Menon wrote:
+int opp_store_data(struct omap_opp *opp, char *name, void *data)
+{
+       return -EINVAL;

Would -ENODATA fit better ??

diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
index bb8120e..15f6f7c 100644
--- a/arch/arm/plat-omap/opp.c
+++ b/arch/arm/plat-omap/opp.c
@@ -14,12 +14,19 @@
#include <linux/errno.h>
#include <linux/err.h>
#include <linux/init.h>
+#include <linux/list.h>
#include <linux/slab.h>
#include <linux/cpufreq.h>

#include <plat/opp_twl_tps.h>
#include <plat/opp.h>

+struct omap_opp_data {
+       char *name;
+       void *data;
+       struct list_head list;
+};
+
/**
 * struct omap_opp - OMAP OPP description structure
 * @enabled:    true/false - marking this OPP as enabled/disabled
@@ -37,6 +44,7 @@ struct omap_opp {
        unsigned long rate;
        unsigned long u_volt;
        u8 opp_id;
+       struct list_head data_list;
};

/*
@@ -218,6 +226,7 @@ static void omap_opp_populate(struct omap_opp *opp,
        opp->rate = opp_def->freq;
        opp->enabled = opp_def->enabled;
        opp->u_volt = opp_def->u_volt;
+       INIT_LIST_HEAD(&opp->data_list);
}

int opp_add(enum opp_t opp_type, const struct omap_opp_def *opp_def)
@@ -352,6 +361,63 @@ int opp_disable(struct omap_opp *opp)
        return 0;
}

+void *opp_get_data(struct omap_opp *opp, char *name)
+{
+       void *data = ERR_PTR(-EINVAL);
+       struct omap_opp_data *tmp;
+
+       if (unlikely(!opp || !name))
+               return ERR_PTR(-EINVAL);
+
+       list_for_each_entry(tmp, &opp->data_list, list)
+               if (!strcmp(name, tmp->name)) {
+                       data = tmp->data;
+                       break;
+               }
+       return data;
+}
+
+int opp_store_data(struct omap_opp *opp, char *name, void *data)
+{
+       struct omap_opp_data *new;
+       if (unlikely(!opp || !name))
+               return -EINVAL;
+       /* NAK to double registration */
+       if (unlikely(!IS_ERR(opp_get_data(opp, name))))
+               return -EINVAL;
+
+       new = kmalloc(sizeof(struct omap_opp), GFP_KERNEL);
+       if (!new)
+               return -ENOMEM;
+       new->name = kmalloc(strlen(name) + 1, GFP_KERNEL);
+       if (!new->name) {
+               kfree(new);
+               return -ENOMEM;
+       }
+       new->data = data;
+       strcpy(new->name, name);
+       INIT_LIST_HEAD(&new->list);
+       list_add(&new->list, &opp->data_list);
+       return 0;
+}

will you really want to add several data entries there ? I don't see the point. Maybe I'm missing something.

I like the idea of having a void * to allow you to store anything there. But generally that's a 1 - 1 relation:

1 device/bus/(in this case) opp - 1 data.

by doing that, the API will look like dev_set_drvdata()/dev_get_drvdata() and could be implemented simply as:

void opp_set_data(struct omap_opp *opp, void *data)
{
        opp->data = data;

        return 0;
}

void *opp_get_data(struct omap_opp *opp)
{
        return opp->data;
}

--
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to