All of lore.kernel.org
 help / color / mirror / Atom feed
From: r.marek@assembler.cz (Rudolf Marek)
To: lm-sensors@vger.kernel.org
Subject: [lm-sensors] new chip support kernel driver
Date: Sat, 25 Nov 2006 16:36:33 +0000	[thread overview]
Message-ID: <45687111.2050500@assembler.cz> (raw)
In-Reply-To: <F7F9B0BE3E9BD449B110D0B1CEF6CAEF03FA5811@ABGEX01E.abg.fsc.net>

Hello Thilo,

Sorry for delay, it always takes long time to do the review. Jean was moving to 
other place and I was very busy too. However I had some time and took a look to 
your driver. Please check the attachment.

Please can you send in future the patches as txt attachments? (and diff them 
with diff -uprN please).

> I will ask for public documentation and of course you can ask me. I know 
> this is not the best.

Anything new on this? It is quite a pity that I dont know what you are doing 
sometimes ;)

I have few general remarks.

1) you can get rid of most register #define using some simple computation macro, 
or static lookup array.

2) You need to use new mutex stuff (please check the patch I put the note how to 
do that)

3) SENSOR_ATTR is your friend, you dont need all those sysfs access macros, I 
put the note there how to fix it

4) you will need to use group add/ grop remove and check the return status

5) there is a lot of non-standard files
    Do you need them? Or your customer? Maybe we can tidy them up. Please can 
you post the list (cat/ls output) so we can check if the content/names are 
according to our standard?

6) cool would be to have per channel alarm and not just big alarm value.

7) watchdog interface
    I know that that those days it is not easy to use the watchdog subsystem in 
kernel and that it sucks. And it is not even easy to connect it to i2c device 
driver. If you have chosen your non-standard watchdog interface you are on your 
own and the users too.

so you can a) remove the watchdog interface
            b) document it Documentation/hwmon/yourchip
            c) use the kernel watchdog interface (yes it really sucks)

8) use indent -i8 -kr this will format your file according the coding style ;)

9) GPL - they moved their office, and maybe you want GPL v2 only.

I think I will create some hwmon driver SDK so everyone can you the right source 
as the base ;)

The userspace patch will be next once we are done with the driver.

Maybe Jean will have some comments too. If you have any questions comments on my 
review please let us know.

Regards
Rudolf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fschmd_kernel_module_2.6.16.21.patch
Type: text/x-patch
Size: 28689 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20061125/2b9950c5/attachment-0001.bin 

      parent reply	other threads:[~2006-11-25 16:36 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-10-16  7:37 [lm-sensors] new chip support kernel driver Cestonaro, Thilo
2006-10-16 11:04 ` Jean Delvare
2006-10-17  7:11 ` Cestonaro, Thilo
2006-11-25 16:36 ` Rudolf Marek [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=45687111.2050500@assembler.cz \
    --to=r.marek@assembler.cz \
    --cc=lm-sensors@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.