On Mon, Jul 19, 2010 at 02:25:55AM -0400, Chen Gong wrote:
> 于 7/19/2010 11:20 AM, Guenter Roeck 写道:
> > On Sun, Jul 18, 2010 at 10:50:33PM -0400, Chen Gong wrote:
> >> cleanup some redundant codes in coretemp.c.
> >>
> >> Signed-off-by: Chen Gong<gong.chen@xxxxxxxxxxxxxxx>
> >> ---
> >> drivers/hwmon/coretemp.c | 16 +++-------------
> >> 1 files changed, 3 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> >> index 9232c4e..423ece5 100644
> >> --- a/drivers/hwmon/coretemp.c
> >> +++ b/drivers/hwmon/coretemp.c
> >> @@ -54,12 +54,12 @@ struct coretemp_data {
> >> const char *name;
> >> u32 id;
> >> u16 core_id;
> >> + u8 alarm;
> >> char valid; /* zero until following fields are valid */
> >> unsigned long last_updated; /* in jiffies */
> >> int temp;
> >> int tjmax;
> >> int ttarget;
> >> - u8 alarm;
> >> };
> >>
> >> /*
> >> @@ -308,7 +308,7 @@ static int __devinit coretemp_probe(struct platform_device *pdev)
> >> #ifdef CONFIG_SMP
> >> data->core_id = c->cpu_core_id;
> >> #endif
> >> - data->name = "coretemp";
> >> + data->name = DRVNAME;
> >> mutex_init(&data->update_lock);
> >>
> >> /* test if we can access the THERM_STATUS MSR */
> >> @@ -543,8 +543,7 @@ static int __init coretemp_init(void)
> >> if (c->cpuid_level>= 6&& (cpuid_eax(0x06)& 0x01)) {
Just noticed .. the formatting above is a bit odd. Maybe you can fix
it while you are at it. Should be
if (c->cpuid_level >= 6 && (cpuid_eax(0x06) & 0x01)) {
> >> err = coretemp_device_add(i);
> >> if (err)
> >> - goto exit_devices_unreg;
> >> -
> >> + goto exit;
> >
> > This error exit including cleanup should be retained.
> > Failure reasons are memory errors and similar severe problems,
> > so it makes sense to clean up and bail out.
>
> I don't think so. There are 2 reasons:
> 1. function coretemp_device_add has dealed with memory failure and
> similar errors. Afterwards the logic is a little bit unnecessary.
>
> 2. why when one device fails, all devices should be removed ? Does this
> make sense ? If my consumption is OK, the reason 1 is meaningful.
>
Hmm ... maybe you are right, especially in the context that each call to
coretemp_device_add() really adds a new sysfs group and not just an additional sensor.
But then it doesn't really make sense to abort after the first error either.
Or, in other words, maybe coretemp_device_add() should not return an error
in the first place.
> >
> > Also, since this patch changes driver operation, calling it "cleanup"
> > isn't really appropriate.
>
> If above explanation is reasonable, the title should be changed, or
> it is still a cleanup patch. ( Only one line changed :-) )
>
> >
> >> } else {
> >> printk(KERN_INFO DRVNAME ": CPU (model=0x%x)"
> >> " has no thermal sensor.\n", c->x86_model);
> >> @@ -555,15 +554,6 @@ static int __init coretemp_init(void)
> >> register_hotcpu_notifier(&coretemp_cpu_notifier);
> >> #endif
> >> return 0;
> >> -
> >> -exit_devices_unreg:
> >> - mutex_lock(&pdev_list_mutex);
> >> - list_for_each_entry_safe(p, n,&pdev_list, list) {
> >> - platform_device_unregister(p->pdev);
> >> - list_del(&p->list);
> >> - kfree(p);
> >> - }
> >> - mutex_unlock(&pdev_list_mutex);
> >> exit:
> >> return err;
> >> }
> >> --
> >> 1.7.2.rc3
> >>
>
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
