On Mon, 19 Jul 2010 00:09:52 -0700, Guenter Roeck wrote:
> 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
> > >> (...)
> > >> @@ -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)) {
This is an artifact from some e-mail client on the way. The original
code in the driver is already well formatted.
>
> > >> 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.
This would make some sense, yes, even though I don't expect it to make
much different in practice.
But as I said previously, such a change should be done consistently,
and with a meaningful description. And other cleanup changes shouldn't
belong to the same patch.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
