[RFC PATCHv1 1/2] Export SoC info through sysfs
Maxime Coquelin
maxime.coquelin-nonst at stericsson.com
Thu Mar 10 04:45:12 EST 2011
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.
>> +
>> + 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.
>> +struct sys_soc_info {
>> + char *name;
>> + char *info;
>> + char *(*get_info)(struct sys_soc_info *);
> Could this return a const char* ?
>
Fixed.
>> + 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
More information about the linux-arm-kernel
mailing list