Hi Yadwinder, Sachin, On Friday 15 of November 2013 17:11:28 Sachin Kamat wrote: > From: Yadwinder Singh Brar <yadi.brar@xxxxxxxxxxx> > > This patch introduces a common ASV (Adaptive Supply Voltage) basic framework > for samsung SoCs. It provides common APIs (to be called by users to get ASV > values or init opp_table) and an interface for SoC specific drivers to > register ASV members (instances). [snip] > diff --git a/drivers/power/asv/asv.c b/drivers/power/asv/asv.c > new file mode 100644 > index 000000000000..3f2c31a0d3a9 > --- /dev/null > +++ b/drivers/power/asv/asv.c > @@ -0,0 +1,176 @@ > +/* > + * ASV(Adaptive Supply Voltage) common core > + * > + * Copyright (c) 2013 Samsung Electronics Co., Ltd. > + * http://www.samsung.com/ > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > +*/ > + > +#include <linux/io.h> > +#include <linux/pm_opp.h> > +#include <linux/slab.h> > +#include <linux/device.h> > +#include <linux/power/asv-driver.h> > + > +static LIST_HEAD(asv_list); > +static DEFINE_MUTEX(asv_mutex); > + > +struct asv_member { > + struct list_head node; > + struct asv_info *asv_info; nit: Inconsistent indentation of member names. In general I would recommend dropping the tabs between types and names and using a single space instead, since this is more future proof - you will never have to change other lines to add new ones. > +}; > + > +static void add_asv_member(struct asv_member *asv_mem) > +{ > + mutex_lock(&asv_mutex); > + list_add_tail(&asv_mem->node, &asv_list); > + mutex_unlock(&asv_mutex); > +} > + > +static struct asv_member *asv_get_mem(enum asv_type_id asv_type) I don't really like this enum based look-up. It's hard to define an enum that covers any possible existing and future platforms that would not be bloated with single platform specific entries. IMHO something string based could be more scalable. > +{ > + struct asv_member *asv_mem; > + struct asv_info *asv_info; > + > + list_for_each_entry(asv_mem, &asv_list, node) { > + asv_info = asv_mem->asv_info; > + if (asv_type == asv_info->type) > + return asv_mem; > + } Don't you need any kind of locking here? A mutex in add_asv_member() suggests that read access to the list should be protected as well. > + > + return NULL; > +} > + > +unsigned int asv_get_volt(enum asv_type_id target_type, > + unsigned int target_freq) Do you need this function at all? I believe this is all about populating device's OPP array with frequencies and voltages according to its ASV level. Users will be able to query for required voltage using standard OPP calls then, without a need for ASV specific functions like this one. > +{ > + struct asv_member *asv_mem = asv_get_mem(target_type); > + struct asv_freq_table *dvfs_table; > + struct asv_info *asv_info; > + unsigned int i; > + > + if (!asv_mem) > + return 0; > + > + asv_info = asv_mem->asv_info; > + dvfs_table = asv_info->dvfs_table; > + > + for (i = 0; i < asv_info->nr_dvfs_level; i++) { > + if (dvfs_table[i].freq == target_freq) > + return dvfs_table[i].volt; > + } > + > + return 0; > +} > + > +int asv_init_opp_table(struct device *dev, enum asv_type_id target_type) > +{ > + struct asv_member *asv_mem = asv_get_mem(target_type); > + struct asv_info *asv_info; > + struct asv_freq_table *dvfs_table; > + unsigned int i; > + > + if (!asv_mem) > + return -EINVAL; > + > + asv_info = asv_mem->asv_info; > + dvfs_table = asv_info->dvfs_table; > + > + for (i = 0; i < asv_info->nr_dvfs_level; i++) { > + if (dev_pm_opp_add(dev, dvfs_table[i].freq * 1000, > + dvfs_table[i].volt)) { > + dev_warn(dev, "Failed to add OPP %d\n", > + dvfs_table[i].freq); Hmm, shouldn't it be considered a failure instead? > + continue; > + } > + } > + > + return 0; > +} > + > +static struct asv_member *asv_init_member(struct asv_info *asv_info) > +{ > + struct asv_member *asv_mem; > + int ret = 0; > + > + if (!asv_info) { > + pr_err("No ASV info provided\n"); > + return NULL; I'd suggest adopting the ERR_PTR() convention, which allows returning more information about the error. > + } > + > + asv_mem = kzalloc(sizeof(*asv_mem), GFP_KERNEL); > + if (!asv_mem) { > + pr_err("Allocation failed for member: %s\n", asv_info->name); > + return NULL; > + } > + > + asv_mem->asv_info = kmemdup(asv_info, sizeof(*asv_info), GFP_KERNEL); > + if (!asv_mem->asv_info) { > + pr_err("Copying asv_info failed for member: %s\n", > + asv_info->name); > + kfree(asv_mem); > + return NULL; For consistency, the error here should be handled as below, by using the error path. > + } > + asv_info = asv_mem->asv_info; > + > + if (asv_info->ops->get_asv_group) { > + ret = asv_info->ops->get_asv_group(asv_info); > + if (ret) { > + pr_err("get_asv_group failed for %s : %d\n", > + asv_info->name, ret); > + goto err; > + } > + } > + > + if (asv_info->ops->init_asv) > + ret = asv_info->ops->init_asv(asv_info); > + if (ret) { > + pr_err("asv_init failed for %s : %d\n", > + asv_info->name, ret); > + goto err; > + } > + > + /* In case of parsing table from DT, we may need to add flag to identify > + DT supporting members and call init_asv_table from asv_init_opp_table( > + after getting dev_node from dev,if required), instead of calling here. > + */ coding style: Wrong multi-line comment style. /* * This is a valid multi-line comment. */ > + > + if (asv_info->ops->init_asv_table) { > + ret = asv_info->ops->init_asv_table(asv_info); > + if (ret) { > + pr_err("init_asv_table failed for %s : %d\n", > + asv_info->name, ret); > + goto err; > + } > + } Hmm, I don't see a point of these three separate callbacks above. In general, I'd suggest a different architecture. I'd see this more as: 1) Platform code registers static platform device to instantiate SoC ASV driver. 2) SoC specific ASV driver probes, reads group ID from hardware register, calls register_asv_member() with appropriate DVFS table for detected group. 3) Driver using ASV calls asv_init_opp_table() with its struct device and ASV member name, which causes the ASV code to fill device's operating point using OPP calls. Now client driver has all the information it needs and the work of ASV subsystem is done. The control flow between drivers would be much simpler and no callbacks would have to be called. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |