* [lm-sensors] New abituguru driver using platform_device
2006-02-04 13:28 [lm-sensors] New abituguru driver using platform_device Hans de Goede
@ 2006-02-06 15:41 ` Jean Delvare
2006-02-07 23:05 ` Jean Delvare
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Jean Delvare @ 2006-02-06 15:41 UTC (permalink / raw)
To: lm-sensors
Hi Hans,
On 2006-02-04, Hans de Goede wrote:
> The subject says it all, could someone review this one please? And can
> we then get some movement with regards to upstreaming this?
Thanks a lot for having converted your driver to a platform driver so
quickly, this is very much appreciated. See my comments below, after
you've addressed them I'll consider the driver for inclusion into -mm.
> If you prefer a patch including docs and the necessary makefile mods
> please lett me know against which tree and howto get this tree.
This is a requirement actually. I can't merge anything but proper
patches. You would base such a patch on Andrew's latest -mm. In the
case of the uguru, I also make it a requirement that the patch adds you
to MAINTAINERS as the maintainer of the driver. Also don't forget to
tag the driver EXPERIMENTAL in Kconfig.
The documentation must come in a patch format too, but could be a
separate patch.
Also make sure you include a Signed-off-by line in your mail, as
mentioned in Documentation/SubmittingPatches
Here come my comments on your code:
> /*
> abituguru.c - Part of lm_sensors, Linux kernel modules
> for hardware monitoring
No, it's not part of lm_sensors. Many drivers have this comment as a
legacy from the Linux 2.4 version - but new drivers shouldn't say that.
> Copyright (c) 2005 Hans de Goede <j.w.r.degoede at hhs.nl>
You may want to add 2006 now.
> /* This is needed untill this gets merged upstream */
> #ifndef SENSOR_ATTR_2
> #define SENSOR_ATTR_2(_name, _mode, _show, _store, _nr, _index) \
> { .dev_attr = __ATTR(_name, _mode, _show, _store), \
> .index = _index, \
> .nr = _nr }
> #endif
This has been merged upstream, so don't include it here.
> #define ABIT_UGURU_PRINTK(level, format, arg...) \
> do { \
> if (level <= verbose) { \
> if (level = 1) \
> printk(KERN_WARNING ABIT_UGURU_NAME ": "\
> format , ## arg); \
> else if (level = 2) \
> printk(KERN_INFO ABIT_UGURU_NAME ": "\
> format , ## arg); \
> else \
> printk(KERN_DEBUG ABIT_UGURU_NAME ": "\
> format , ## arg); \
> } \
> } while (0)
I see very little interest in letting the user hide warnings and
informational messages. It's even a dangerous feature for warnings.
Additionally, you must use dev_warn, dev_info and dev_dbg to print
messages whenever possible, not printk directly.
> /* Constants */
> /* in (Volt) sensors go up to 3494 mV, temp to 255000 milli degrees
> celcius */
> const int abituguru_bank1_max_value[2] = { 3494, 255000 };
> /* Register 0 is a bitfield, 1 and 2 are pwm settings (255 = 100%), 3 and
> 4 are temperature trip points. */
> const int abituguru_pwm_settings_multiplier[5] = { 0, 1, 1, 1000, 1000 };
These should be declared static.
> /* Default verbose is 4, since this driver is still in the testing fase */
Typo: phase.
> static int verbose = 4;
> module_param(verbose, int, 0);
> MODULE_PARM_DESC(verbose, "How verbose should the driver be? (0-5):\n"
> " 0 no output at all\n"
> " 1 + warnings\n"
> " 2 + normal output\n"
> " 3 + standard debug output\n"
> " 4 + sensors type probing info\n"
> " 5 + retryable errors");
Maybe you could let the user change it after load with:
module_param(verbose, int, 0644);
Also, as said above, I think that modes 0 and 1 should not exist at all.
> unsigned char update_timeouts; /* number of update timeouts since last
> successfull update. */
Typo: successful.
> static int abituguru_wait(struct abituguru_data *data, u8 state)
> {
> int timeout = ABIT_UGURU_WAIT_TIMEOUT;
>
> while (inb_p(data->addr + ABIT_UGURU_DATA) != state)
> {
Coding style!
> timeout--;
> if (timeout = 0)
> return -EBUSY;
> }
> return 0;
> }
Hm, this is a busy waiting loop. Not recommended. Why don't you msleep()
between retries? Or at the very least yield()? In both cases, the
timeout should be a duration rather than a number of tries.
> while(inb_p(data->addr+ABIT_UGURU_DATA) != ABIT_UGURU_STATUS_INPUT){
Coding style: missing spaces.
> if (!data->uguru_ready && (abituguru_ready(data) != 0))
> return -EIO;
The first check is redundant, abituguru_ready() starts with the same
check.
> ABIT_UGURU_PRINTK(3, "timeout exceeded "
> "waiting for more input state "
> "(bank: %d)\n", (int)bank_addr);
The cast shouldn't be needed (you may need %u instead of %d). Same in
various other places.
> if (abituguru_write(data, ABIT_UGURU_SENSOR_BANK1+2,
> sensor_addr, data->bank1_settings[sensor_addr],
> 3) != 3) return -EIO;
Coding style.
> if (abituguru_write(data, ABIT_UGURU_SENSOR_BANK1+2, sensor_addr,
> buf,3) != 3)
Coding style: missing space.
> if (abituguru_write(data, ABIT_UGURU_SENSOR_BANK1+2, sensor_addr,
> data->bank1_settings[sensor_addr], 3) != 3) return -EIO;
Coding style: missing new line.
> return sprintf(buf,"%d\n",(data->bank1_settings[attr->index][attr->nr]*
> data->bank1_max_value[attr->index] + 128) / 255);
Coding style: missing spaces. Same in many other places.
> return sprintf(buf, "%d\n", (data->bank2_value[attr->index]*
> ABIT_UGURU_FAN_MAX + 128) / 255);
Coding style: doubled space.
> if ( (data->bank2_settings[i][0] != orig_val) &&
> (abituguru_write(data, ABIT_UGURU_SENSOR_BANK2+2,
> i, data->bank2_settings[i], 2) < 1) ) {
Coding style: extra spaces. Same in a few other places.
> unsigned long val = simple_strtoul(buf, NULL, 10) - 1;
Doesn't look safe: if the input value is "0", you end up with val ~0. Is it what you want?
> static struct sensor_device_attribute_2 abituguru_sysfs_attr[] = {
> SENSOR_ATTR_2(fan1_input, 0444, show_bank2_value, NULL, 0, 0),
Do not use spaces for alignment please. Use tabs.
> u8 data_val = inb_p(ABIT_UGURU_BASE + ABIT_UGURU_DATA);
You don't use this value anywhere?
> int i,j,res,err = 0;
Coding style: missing spaces.
> const u8 probe_order[16] = {
> 0x00,0x01,0x03,0x04,0x0A,0x08,0x0E,0x02,
> 0x09,0x06,0x05,0x0B,0x0F,0x0D,0x07,0x0C };
Ditto.
> ERROR1:
> kfree(data);
> ERROR0:
> return err;
Please rename these labels to something more explicit (like exit_free...)
> data->bank1_attr[i*3 ].dev_attr.attr.mode = 0444;
> data->bank1_attr[i*3 ].dev_attr.attr.owner = THIS_MODULE;
> data->bank1_attr[i*3 ].dev_attr.show = show_bank1_value;
> data->bank1_attr[i*3 ].dev_attr.store = NULL;
> data->bank1_attr[i*3 ].index = probe_order[i];
> data->bank1_attr[i*3 +1].dev_attr.attr.mode = 0644;
> data->bank1_attr[i*3 +1].dev_attr.attr.owner = THIS_MODULE;
> data->bank1_attr[i*3 +1].dev_attr.show = show_bank1_setting;
> data->bank1_attr[i*3 +1].dev_attr.store = store_bank1_setting;
> data->bank1_attr[i*3 +1].index = probe_order[i];
> data->bank1_attr[i*3 +1].nr = 1;
> data->bank1_attr[i*3 +2].dev_attr.attr.mode = 0644;
> data->bank1_attr[i*3 +2].dev_attr.attr.owner = THIS_MODULE;
> data->bank1_attr[i*3 +2].dev_attr.show = show_bank1_setting;
> data->bank1_attr[i*3 +2].dev_attr.store = store_bank1_setting;
> data->bank1_attr[i*3 +2].index = probe_order[i];
> data->bank1_attr[i*3 +2].nr = 2;
Naaah, really, you don't make things clearer with all this alignment
masquerade. Rather the contrary.
> for (i = 0; i < (sizeof(abituguru_sysfs_attr)/
> sizeof(struct sensor_device_attribute_2)); i++)
Use the ARRAY_SIZE macro please.
> data->last_updated = jiffies;
> (...)
> /* upon error force the next update */
> if (res != 1)
> data->last_updated = jiffies - (unsigned long)HZ;
Why don't you just wait for the end of the (successful) update before
setting data->last_updated?
> if ( ((data_val = 0x00) || (data_val = 0x08)) &&
> ((cmd_val = 0x00) || (cmd_val = 0xAC)) )
> return ABIT_UGURU_BASE;
Coding style: drop the extra spaces.
> printk(KERN_INFO "Asuming Abit uGuru is present "
> "because of \"force\" parameter\n");
Typo: assuming.
> module_init(sm_abituguru_init);
> module_exit(sm_abituguru_exit);
Please drop the "sm_" prefix, some old drivers have it but I don't
even know what it is supposed to mean.
--
Jean Delvare
^ permalink raw reply [flat|nested] 5+ messages in thread* [lm-sensors] New abituguru driver using platform_device
2006-02-04 13:28 [lm-sensors] New abituguru driver using platform_device Hans de Goede
2006-02-06 15:41 ` Jean Delvare
@ 2006-02-07 23:05 ` Jean Delvare
2006-02-08 9:20 ` Hans de Goede
2006-02-08 11:23 ` Jean Delvare
3 siblings, 0 replies; 5+ messages in thread
From: Jean Delvare @ 2006-02-07 23:05 UTC (permalink / raw)
To: lm-sensors
Hi Hans,
Please answer to the list rather than just me.
> > Additionally, you must use dev_warn, dev_info and dev_dbg to print
> > messages whenever possible, not printk directly.
>
> I did, but with a platform driver they make all the messages start with
> "abituguru abituguru.240 :" which is rather convoluted imho, hence the
> change to printk.
Hm, you're right. I didn't notice because I have very few device
messages in my own driver (f71805f). The repeated device/driver name is
due to the way the platform core builds the "bus_id" identifier when
you register a device. Maybe this can be improved (the hexadecimal
address would be a nice bus_id in our case), but I'm not sure, as the
identifier has to be unique to all platform devices, at least as I
understand it. I'll look into it eventually, or feel free to do so
yourself.
>
> >> static int abituguru_wait(struct abituguru_data *data, u8 state)
> >> {
> >> int timeout = ABIT_UGURU_WAIT_TIMEOUT;
> >>
> >> while (inb_p(data->addr + ABIT_UGURU_DATA) != state) {
> >> timeout--;
> >> if (timeout = 0)
> >> return -EBUSY;
> >> }
> >> return 0;
> >> }
> >
> > Hm, this is a busy waiting loop. Not recommended. Why don't you msleep()
> > between retries? Or at the very least yield()? In both cases, the
> > timeout should be a duration rather than a number of tries.
> >
>
> Usually this succeeds within 100 - 150 reads (max 250) which assuming
> the isa bus is the bottleneck and tacking the _p into account translates
> to 300 isa cycles which is 37.5 usec, since the timing can vary I could
> complicate the code by first during a small usleep which will translate
> to a busy wait
No, by definition, this isn't a busy wait if you are sleeping.
> (...) and then do some isa bus busy waiting, but that
> complicates stuff without winning anything.
What I don't get is why you need to do ISA bus waiting at all. Why
don't you just sleep-wait? 10 us sleeps shoud do. Please keep in mind
that your device isn't the only user of the ISA bus
(check /proc/ioports). Given that all uGuru registers are read at once
by design, if you saturate the ISA bus, other devices may experience
increased latency.
As a side note, I don't think the _p has any effect on modern systems,
does it?
> > > while(inb_p(data->addr+ABIT_UGURU_DATA) != ABIT_UGURU_STATUS_INPUT){
> >
> > Coding style: missing spaces.
> >
>
> 1) arround the '+' I assume. Will add I agree it makes things easier to
> read but:
That, but more importanly after the "while" and before the opening
curly brace.
> 2) I'm getting rather tired of these spaces CodingStyle discussions I've
> had them with Rudolf Marek too. This is not an upstream requirement
> but interpretation by the lm_sensors of the CodingStyle document,
> open CodingStyle in your text editor, search for the word space and
> tell me where the _text_ says that spaces should be added around
> operators?
It's absolutely not limited to the lm_sensors group, although I admit
it isn't mentioned in CodingStyle. The "spacing preferences" are
documented here, under the name "kernel guide to space":
http://www.mellanox.com/mst/boring.txt
It probably should be merged into the kernel documentation so as to
make it look more official, as it seems that most developers agree with
most of the rules written here. I agree with 95% of them myself.
Spacing is just as important as coding style itself. We insist on both
for the same reason. Have you ever been maintaining 55000 lines of code
written by a few hundred different people?
> Quoting from my reply to the review done by Rudolf; "printf and friends
> are vararg, thus no auto casting gets done and %d expects an int not u8.
> It might be that gcc does the right thing when a function is marked as a
> printf alike (there is a special attribute for this) but without the
> cast it is not correct C, I'd rather not depend on a gcc "feature" when
> I can write correct C instead."
>
> Really this way it is correct C, of you have a vararg function the
> compiler does not know what should be put on the stack and thus puts the
> arguments with their uncasted type on the stack, at least that is what
> ANSI-C says, thus the casts are needed otherwise only one byte gets put
> on the stack where printf reads 4 as it expects an int which is 4 bytes.
>
> The gcc team has decided to deviate from the ANSI standard for printf
> alike functions and interprets the format string and casts the varargs
> as needed. Which is unfortunate as this make code written for gcc break
> on compilers which lack this "feature". (I guess I have written to much
> code for uC's that I know this).
Thanks a lot for the clarification, I didn't know that. In fact I have
often wondered why/how it was working (e.g. using %u with an unsigned
char) but as it did work OK I never looked any further. I've almost
never worked with non-gcc compilers.
> > > return sprintf(buf,"%d\n",(data->bank1_settings[attr->index][attr->nr]*
> > > data->bank1_max_value[attr->index] + 128) / 255);
> >
> > Coding style: missing spaces. Same in many other places.
>
> This way it fits in 80 chars which is a hard CodingStyle requirement
> unlike the spaces. I can spread it out over an extra line if you want.
I definitely want. If the 80 chars limit rule makes you compact your
code horizontally, it totally misses its objective. If you think it's
too long, just introduce an additional temporary variable. Once
optimized by the compiler, I guess the code will be pretty much the
same anyway.
> > > if ( (data->bank2_settings[i][0] != orig_val) &&
> > > (abituguru_write(data, ABIT_UGURU_SENSOR_BANK2+2,
> > > i, data->bank2_settings[i], 2) < 1) ) {
> >
> > Coding style: extra spaces. Same in a few other places.
>
> Explain please you mean the spaces between ( ( and ) ) thats just to
> make the grouping clearer, otherwise you drown in all the (())).
If you need extra spaces there, it means that your code is too complex
and you need to simplify it (e.g. by introducing additional local
variables) and/or you have unneeded parentheses (which is the case
here). This type of extra-spacing really never helped me read any code.
Good indentation (which is the case here) does.
> (...) I must
> say you guys are a bit pedantic when it comes to this I understand a
> unified codingstyle for tabs and indenting is important and I've tried
> to follow this, you rightfully have pointed out a misplaced { and a case
> where my editor decided to use 8 spaces instead of a tab, but we are all
> humans so code will always have small differences.
Whenever I mention a coding style error in a review, it's only meant
for the author to consider fixing it. No more, no less. If your coding
style was really not correct, my review would have been "go read
Documentation/CodingStyle" and nothing more. Fortunately you're way
past this point, your coding style is, in average, pretty acceptable.
> Talking about this I have taken the "simple_strtoul(buf, NULL, 10)" code
> fragment from other lm_sensors drivers, but shouldn't we pass in &(char
> *) instead if null and checks it points to a '\0' char after the
> conversion to make sure the user send us a valid decimal number,
> currently we see garbage input as just 0.
We could do that, but this would make the code much more complex (and
large, binaey-wise) for very little benefit IMHO.
> > > data->bank1_attr[i*3 ].dev_attr.attr.mode = 0444;
> > > data->bank1_attr[i*3 ].dev_attr.attr.owner = THIS_MODULE;
> > > data->bank1_attr[i*3 ].dev_attr.show = show_bank1_value;
> > > data->bank1_attr[i*3 ].dev_attr.store = NULL;
> > > data->bank1_attr[i*3 ].index = probe_order[i];
> > > data->bank1_attr[i*3 +1].dev_attr.attr.mode = 0644;
> > > data->bank1_attr[i*3 +1].dev_attr.attr.owner = THIS_MODULE;
> > > data->bank1_attr[i*3 +1].dev_attr.show = show_bank1_setting;
> > > data->bank1_attr[i*3 +1].dev_attr.store = store_bank1_setting;
> > > data->bank1_attr[i*3 +1].index = probe_order[i];
> > > data->bank1_attr[i*3 +1].nr = 1;
> > > data->bank1_attr[i*3 +2].dev_attr.attr.mode = 0644;
> > > data->bank1_attr[i*3 +2].dev_attr.attr.owner = THIS_MODULE;
> > > data->bank1_attr[i*3 +2].dev_attr.show = show_bank1_setting;
> > > data->bank1_attr[i*3 +2].dev_attr.store = store_bank1_setting;
> > > data->bank1_attr[i*3 +2].index = probe_order[i];
> > > data->bank1_attr[i*3 +2].nr = 2;
> >
> > Naaah, really, you don't make things clearer with all this alignment
> > masquerade. Rather the contrary.
>
> And you suggest to fix this how (iow how should I make things clearer)?
Drop any additional space inside the square brackets and doubled spaces
before equal signs as well. In other words, write every line as you
would have if it were isolated. If it then looks a bit too compact,
feel free to insert a couple blank lines.
Thanks,
--
Jean Delvare
^ permalink raw reply [flat|nested] 5+ messages in thread* [lm-sensors] New abituguru driver using platform_device
2006-02-04 13:28 [lm-sensors] New abituguru driver using platform_device Hans de Goede
2006-02-06 15:41 ` Jean Delvare
2006-02-07 23:05 ` Jean Delvare
@ 2006-02-08 9:20 ` Hans de Goede
2006-02-08 11:23 ` Jean Delvare
3 siblings, 0 replies; 5+ messages in thread
From: Hans de Goede @ 2006-02-08 9:20 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
Many thanks for clarifying your review, I only have one question left,
see below.
Jean Delvare wrote:
> Hi Hans,
>
> Please answer to the list rather than just me.
>
Oops, sorry. I'm not used to the write to list and CC the author kinda
use of mailinglist. Right now I'm only writing this to the list, is this
ok, or do you always want a direct CC?
>>>> static int abituguru_wait(struct abituguru_data *data, u8 state)
>>>> {
>>>> int timeout = ABIT_UGURU_WAIT_TIMEOUT;
>>>>
>>>> while (inb_p(data->addr + ABIT_UGURU_DATA) != state) {
>>>> timeout--;
>>>> if (timeout = 0)
>>>> return -EBUSY;
>>>> }
>>>> return 0;
>>>> }
>>> Hm, this is a busy waiting loop. Not recommended. Why don't you msleep()
>>> between retries? Or at the very least yield()? In both cases, the
>>> timeout should be a duration rather than a number of tries.
>>>
>> Usually this succeeds within 100 - 150 reads (max 250) which assuming
>> the isa bus is the bottleneck and tacking the _p into account translates
>> to 300 isa cycles which is 37.5 usec, since the timing can vary I could
>> complicate the code by first during a small usleep which will translate
>> to a busy wait
>
> No, by definition, this isn't a busy wait if you are sleeping.
>
My bad, the kernel doesn't have a usleep function, because sleeping for
such short amounts is not supported, its called udelay, which translates
to a busy wait, see arch/xxx/lib/delay.c
>> (...) and then do some isa bus busy waiting, but that
>> complicates stuff without winning anything.
>
> What I don't get is why you need to do ISA bus waiting at all. Why
> don't you just sleep-wait? 10 us sleeps shoud do. Please keep in mind
> that your device isn't the only user of the ISA bus
> (check /proc/ioports).
> Given that all uGuru registers are read at once
> by design, if you saturate the ISA bus, other devices may experience
> increased latency.
>
I only do an update once every second, thus reducing the ISA bus load.
During an update the wait function gets called 147 times, resulting in
circa 5 ms ISA-bus usage.
But the real point is that using delay functions doesn't win all that
much since we still keep the CPU occupied, thus although it will keep
the ISA bus free there will be no CPU to use it and if we get preempted
we'll also stop using the ISA bus. The only exception to this is SMP
systems, but SMP systems really shouldn't rely on the ISA-bus.
I could change the code to use delay functions if you insist, but I'd
rather not as the current version has already been tested thoroughly by
a small team of beta testers I've build up and thus I'd rather not touch
this piece of code.
An other option to be nicer to other processes would be to yield between
reads in the update function, this way the code-path of a single read
doesn't get any changes, so I'm pretty sure this won't cause any
problems. Then again if something else wants the CPU, we would get
preempted anyways, so I don't know if this really helps.
> As a side note, I don't think the _p has any effect on modern systems,
> does it?
>
It still does, it causes a read of isa address 0x80, see
include/asm/io.h, thus causing a one isa cycle delay.
Regards,
Hans
^ permalink raw reply [flat|nested] 5+ messages in thread* [lm-sensors] New abituguru driver using platform_device
2006-02-04 13:28 [lm-sensors] New abituguru driver using platform_device Hans de Goede
` (2 preceding siblings ...)
2006-02-08 9:20 ` Hans de Goede
@ 2006-02-08 11:23 ` Jean Delvare
3 siblings, 0 replies; 5+ messages in thread
From: Jean Delvare @ 2006-02-08 11:23 UTC (permalink / raw)
To: lm-sensors
Hi Hans,
On 2006-02-08, Hans de Goede wrote:
> Oops, sorry. I'm not used to the write to list and CC the author kinda
> use of mailinglist. Right now I'm only writing this to the list, is this
> ok, or do you always want a direct CC?
The easiest for you, both are equally fine as far as I am concerned.
> > No, by definition, this isn't a busy wait if you are sleeping.
>
> My bad, the kernel doesn't have a usleep function, because sleeping for
> such short amounts is not supported, its called udelay, which translates
> to a busy wait, see arch/xxx/lib/delay.c
Ah, right, I had missed that point. It's indeed msleep() we use in i2c
bus drivers. Obviously not an option here.
> I could change the code to use delay functions if you insist, but I'd
> rather not as the current version has already been tested thoroughly by
> a small team of beta testers I've build up and thus I'd rather not touch
> this piece of code.
No, I am not insisting. The only conclusion this leads me to is: what a
crappy hardware design. No surprise Abit didn't want to document it
publicly.
> An other option to be nicer to other processes would be to yield between
> reads in the update function, this way the code-path of a single read
> doesn't get any changes, so I'm pretty sure this won't cause any
> problems. Then again if something else wants the CPU, we would get
> preempted anyways, so I don't know if this really helps.
I'm not a specialist of preemption and yield, but given that preemption
may be disabled, using yield() at times would indeed probably be fair.
> > As a side note, I don't think the _p has any effect on modern systems,
> > does it?
>
> It still does, it causes a read of isa address 0x80, see
> include/asm/io.h, thus causing a one isa cycle delay.
Thanks for the pointer once again. What I was in fact told some times ago
is that modern systems did not need the additional cycle, not that the
cycle wasn't done; I remember now.
So, inb uses one single ISA bus cycle and inb_p uses two, right? If so,
and if we agree that the ISA bus is clocked at 8 MHz, then the 37.5 us
max you announced for a read are in fact more like 75 us max, aren't
they? Not that it matters much for your driver implementation - just
trying to make sure I understand how things work.
Thanks,
--
Jean Delvare
^ permalink raw reply [flat|nested] 5+ messages in thread