On 03/09/2011 06:39 PM, Jamie Iles wrote:

Hi Maxime,

Thanks for doing this, it looks very nice!  A couple of nitpicks, but
I've given it a quick spin on my platform and it provides us with all of
the hooks to export all of the information we need.

Jamie

Hi Jamie,
Thanks for your review. Fine it works for you.

+
+static struct sysdev_class_attribute *soc_default_attrs[];
+
+struct sys_soc {
+	char *mach_name;
Can this be made const?

Yes, you're right.

quoted

+
+	if (si->info)
+		return sprintf(buf, "%s\n", si->info);
+	else if (si->get_info)
+		return sprintf(buf, "%s\n", si->get_info(si));
+	else
+		return sprintf(buf, "No data\n");

Isn't this a platform error if we don't have a way to get the required
info?  If in register_sys_soc_info() we check that one of si->info or
si->get_info is non-NULL then we don't need this check.  If we have
something like:

static bool sys_soc_info_is_valid(struct sys_soc_info *info)
{
	if ((!info->info&&  !info->get_info) ||
	    info->info&&  info->get_info)
	    return false;

	return true;
}

Yes it is a platform error, it should be tested at registration time.

then we can do this at registration.  Is there a valid use case where
someone could set the static info and the dynamic get_info callback?

At registration, no. But in the get_info callback, we can set the info 
field in order to call the callback once if the value will never change.
However, I will remove this possibility as it is not clear.

Make name const?  Also, should num be a size_t?

Fixed

soc.mach_name = kstrdup(name, GFP_KERNEL) instead of the allocate and
sprintf?

Fixed.

quoted

+struct sys_soc_info {
+	char *name;
+	char *info;
+	char *(*get_info)(struct sys_soc_info *);

Could this return a const char* ?

Fixed.

quoted

+	struct sysdev_class_attribute attr;
+};
+
+/**
+ * void register_sys_soc(char *name, struct sys_soc_info *, int num)

I think this should be "register_sys_soc - register the soc information"
for valid kerneldoc notation..

Fixed.

Regards,
Maxime