Fwd: Telecom Clock Driver for MPCBL0010 ATCA computer blade
Mark Gross
Andrew,
Attached is a simple charactor driver for possible inclusion in your MM tree.
This driver is specific to the MPCBL0010 that will start shipping this fall.
The telcom clock is a special circuit, line card PLL, that provids a mechanism
for synchronization of specialized hardware across the backplane of a chassis
of multiple computers with similar specail curcits. In this case the
synchronization signals get routed to multiple places, typically to pins on
expansion slots for hardware that knows what to do with this signal. (SONET,
G.813, stratum 3...) and similar signaling applications found in telcom sites
can use this type of thing.
The actual device is hidden behind the FPGA on the motherboar, and is
connected to the FPGA via I2C. This driver only talks to the FPGA registers.
Thanks,
--
--mgross
BTW: This may or may not be the opinion of my employer, more likely not.
-------------------------------------------------------
--
--mgross
BTW: This may or may not be the opinion of my employer, more likely not.
Jesper Juhl
>
> Andrew,
>
> Attached is a simple charactor driver for possible inclusion in your MM tree.
>
> This driver is specific to the MPCBL0010 that will start shipping this fall.
>
> The telcom clock is a special circuit, line card PLL, that provids a mechanism
> for synchronization of specialized hardware across the backplane of a chassis
> of multiple computers with similar specail curcits. In this case the
> synchronization signals get routed to multiple places, typically to pins on
> expansion slots for hardware that knows what to do with this signal. (SONET,
> G.813, stratum 3...) and similar signaling applications found in telcom sites
> can use this type of thing.
>
> The actual device is hidden behind the FPGA on the motherboar, and is
> connected to the FPGA via I2C. This driver only talks to the FPGA registers.
>
A few minor style and spelling comments :
[snip]
> + *
> + * Send feedback to <sebastien...@ca.kontron.com>
> + *
> + * 2.6 driver version being maintained by <mark....@intel.com>
shouldn't this info go into CREDITS/MAINTAINERS and the above then simply be
* Send feedback to Sebastien Bouchard and the current maintainer.
???
Same comment for the other files.
[snip]
> +/* sysFS interface definition:
Isn't it just called "sysfs" without the caps?
> +Uppon loading the driver will create a sysfs directory under class/misc/tlclk.
s/Uppon/Upon/
> +
> +This directory exports the following interfaces. There opperation is documented
Line exceeds 80 characters (in this case due to trailing whitespace).
Let me quote Documentation/CodingStyle Chapter 2 :
"
The limit on the length of lines is 80 columns and this is a hard limit.
Statements longer than 80 columns will be broken into sensible chunks.
Descendants are always substantially shorter than the parent and are placed
substantially to the right. The same applies to function headers with a long
argument list. Long strings are as well broken into shorter strings.
"
You have both comment and code lines elsewhere in the file that exceed this.
Please fix.
And while you are at it, please get rid of all the trailing whitespace. A
simple sed script will do it like this :
sed -r s/"[ \t]+$"/""/ file_with_trailing_whitespace.c > fixed_file.c
[snip]
> +All sysfs interaces are integers in hex format, i.e echo 99 > refalign
I trust you mean "interfaces" ...
[snip]
> +#define debug_printk( args... ) printk( args)
> +#else
> +#define debug_printk( args... )
Why these spaces after start paren and before closing paren?
[snip]
> +tlclk_read(struct file * filp, char __user * buf, size_t count, loff_t * f_pos)
Most of your functions nicely place the '*' next to the variable name, but this
one, the next one and a few other cases do not. Please be consistent.
[snip]
> +static CLASS_DEVICE_ATTR(enable_clk3b_output, S_IWUGO, NULL,
> + store_enable_clk3b_output);
> +
> +
> +
> +static ssize_t store_enable_clk3a_output(struct class_device *d,
Why all those blank lines? One should do just fine.
[snip]
> +static CLASS_DEVICE_ATTR(enable_clk3a_output, S_IWUGO, NULL,
> + store_enable_clk3a_output);
> +
> +
> +
> +static ssize_t store_enable_clkb1_output(struct class_device *d,
Again a lot of blank lines, and this is not the last case (just the last one
I'm going to point out).
[snip]
> +#ifdef CONFIG_SYSFS
> + if( 0 > (ret = misc_register(&tlclk_miscdev )) ) {
First a style thing :
if (0 > (ret = misc_register(&tlclk_miscdev))) {
Secondly, wouldn't this look nicer as
if ((ret = misc_register(&tlclk_miscdev)) < 0) {
or is that just me?
Personally I think I would prefer this :
ret = misc_register(&tlclk_miscdev);
if (ret < 0) {
Looks more readable to me.
> + printk(KERN_ERR" misc_register retruns %d \n", ret);
Ehh, why a space just before the newline in this printk?
> + ret = -EBUSY;
ret = -EBUSY;
--
Jesper Juhl <jespe...@gmail.com>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Alexey Dobriyan
> +++ linux-2.6.14-rc2-mm2-tlclk/drivers/char/tlclk.c
Can you drop ioctl part of interface and leave only sysfs one?
> +#ifdef TLCLK_IOCTL
> +static int
> +tlclk_ioctl(struct inode *inode,
> + struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> + unsigned long flags;
> + unsigned char val;
> + int ret_val = 0;
> +
> + val = (unsigned char)arg;
> + if (_IOC_TYPE(cmd) != TLCLK_IOC_MAGIC)
> + return -ENOTTY;
> +
> + if (_IOC_NR(cmd) > TLCLK_IOC_MAXNR)
> + return -ENOTTY;
> +
> + spin_lock_irqsave(&event_lock, flags);
> + switch (cmd) {
> + case IOCTL_RESET:
> + SET_PORT_BITS(TLCLK_REG4, 0xfd, val);
> + break;
> + case IOCTL_MODE_SELECT:
> + SET_PORT_BITS(TLCLK_REG0, 0xcf, val);
> + break;
> + case IOCTL_REFALIGN:
> + /* GENERATING 0 to 1 transistion */
> + SET_PORT_BITS(TLCLK_REG0, 0xf7, 0);
> + udelay(2);
> + SET_PORT_BITS(TLCLK_REG0, 0xf7, 0x08);
> + udelay(2);
> + SET_PORT_BITS(TLCLK_REG0, 0xf7, 0);
> + break;
> + case IOCTL_HARDWARE_SWITCHING:
> + SET_PORT_BITS(TLCLK_REG0, 0x7f, val);
> + break;
> + case IOCTL_HARDWARE_SWITCHING_MODE:
> + SET_PORT_BITS(TLCLK_REG0, 0xbf, val);
> + break;
> + case IOCTL_FILTER_SELECT:
> + SET_PORT_BITS(TLCLK_REG0, 0xfb, val);
> + break;
> + case IOCTL_SELECT_REF_FREQUENCY:
> + SET_PORT_BITS(TLCLK_REG1, 0xfd, val);
> + break;
> + case IOCTL_SELECT_REDUNDANT_CLOCK:
> + SET_PORT_BITS(TLCLK_REG1, 0xfe, val);
> + break;
> + case IOCTL_SELECT_AMCB1_TRANSMIT_CLOCK:
> + if ((val == CLK_8kHz) || (val == CLK_16_384MHz)) {
> + SET_PORT_BITS(TLCLK_REG3, 0xf8, 0x5);
> + SET_PORT_BITS(TLCLK_REG1, 0xfb, ~val);
> + } else if (val >= CLK_8_592MHz) {
> + SET_PORT_BITS(TLCLK_REG3, 0xf8, 0x7);
> + switch (val) {
> + case CLK_8_592MHz:
> + SET_PORT_BITS(TLCLK_REG0, 0xfc, 1);
> + break;
> + case CLK_11_184MHz:
> + SET_PORT_BITS(TLCLK_REG0, 0xfc, 0);
> + break;
> + case CLK_34_368MHz:
> + SET_PORT_BITS(TLCLK_REG0, 0xfc, 3);
> + break;
> + case CLK_44_736MHz:
> + SET_PORT_BITS(TLCLK_REG0, 0xfc, 2);
> + break;
> + }
> + } else
> + SET_PORT_BITS(TLCLK_REG3, 0xf8, val);
> + break;
> + case IOCTL_SELECT_AMCB2_TRANSMIT_CLOCK:
> + if ((val == CLK_8kHz) || (val == CLK_16_384MHz)) {
> + SET_PORT_BITS(TLCLK_REG3, 0xc7, 0x28);
> + SET_PORT_BITS(TLCLK_REG1, 0xfb, ~val);
> + } else if (val >= CLK_8_592MHz) {
> + SET_PORT_BITS(TLCLK_REG3, 0xc7, 0x38);
> + switch (val) {
> + case CLK_8_592MHz:
> + SET_PORT_BITS(TLCLK_REG0, 0xfc, 1);
> + break;
> + case CLK_11_184MHz:
> + SET_PORT_BITS(TLCLK_REG0, 0xfc, 0);
> + break;
> + case CLK_34_368MHz:
> + SET_PORT_BITS(TLCLK_REG0, 0xfc, 3);
> + break;
> + case CLK_44_736MHz:
> + SET_PORT_BITS(TLCLK_REG0, 0xfc, 2);
> + break;
> + }
> + } else
> + SET_PORT_BITS(TLCLK_REG3, 0xc7, val << 3);
> + break;
> + case IOCTL_TEST_MODE:
> + SET_PORT_BITS(TLCLK_REG4, 0xfd, 2);
> + break;
> + case IOCTL_ENABLE_CLKA0_OUTPUT:
> + SET_PORT_BITS(TLCLK_REG2, 0xfe, val);
> + break;
> + case IOCTL_ENABLE_CLKB0_OUTPUT:
> + SET_PORT_BITS(TLCLK_REG2, 0xfd, val << 1);
> + break;
> + case IOCTL_ENABLE_CLKA1_OUTPUT:
> + SET_PORT_BITS(TLCLK_REG2, 0xfb, val << 2);
> + break;
> + case IOCTL_ENABLE_CLKB1_OUTPUT:
> + SET_PORT_BITS(TLCLK_REG2, 0xf7, val << 3);
> + break;
> + case IOCTL_ENABLE_CLK3A_OUTPUT:
> + SET_PORT_BITS(TLCLK_REG3, 0xbf, val << 6);
> + break;
> + case IOCTL_ENABLE_CLK3B_OUTPUT:
> + SET_PORT_BITS(TLCLK_REG3, 0x7f, val << 7);
> + break;
> + case IOCTL_READ_ALARMS:
> + ret_val = (inb(TLCLK_REG2) & 0xf0);
> + break;
> + case IOCTL_READ_INTERRUPT_SWITCH:
> + ret_val = inb(TLCLK_REG6);
> + break;
> + case IOCTL_READ_CURRENT_REF:
> + ret_val = ((inb(TLCLK_REG1) & 0x08) >> 3);
> + break;
> + }
> + spin_unlock_irqrestore(&event_lock, flags);
> +
> + return ret_val;
> +}
> +#endif
Mark Gross
Thanks for the review, I thought I had the 80 line thing taken care of last month. :(
See attached, I hope it catches all the issues you found.
Mark Gross
> > --- linux-2.6.14-rc2-mm2/drivers/char/tlclk.c
> > +++ linux-2.6.14-rc2-mm2-tlclk/drivers/char/tlclk.c
>
> Can you drop ioctl part of interface and leave only sysfs one?
>
I would like to keep if for a little while because the hardware validation
guys are still using test harnesses written for the 2.4 kernel driver
version. However; I am willing to pull this block if that would help in
getting this driver into the kernel.
--mgross
--
--mgross
BTW: This may or may not be the opinion of my employer, more likely not.
Greg KH
> +#if CONFIG_DEBUG_KERNEL
> +#else
> +#define debug_printk( args... )
Please just use the existing dev_dbg() and friend functions instead of
creating your own.
> +DEFINE_SPINLOCK(event_lock);
This should be static, right?
> +irqreturn_t tlclk_interrupt(int irq, void *dev_id, struct pt_regs *regs);
static?
> +DECLARE_WAIT_QUEUE_HEAD(wq);
static?
> +#ifdef TLCLK_IOCTL
Please just delete this whole section, no new ioctls for 2.6 please.
> +ssize_t
> +tlclk_read(struct file * filp, char __user * buf, size_t count, loff_t * f_pos)
Return type on the same line as the function name please.
> +{
> + int count0 = sizeof(struct tlclk_alarms);
> +
> + wait_event_interruptible(wq, got_event);
> + if (copy_to_user(buf, alarm_events, sizeof(struct tlclk_alarms)))
> + return -EFAULT;
> +
> + memset(alarm_events, 0, sizeof(struct tlclk_alarms));
> + got_event = 0;
> +
> + return count0;
count0 doesn't really need to be here, does it?
What if you get passed less than that size of data? You will be reading
in off of the end of the buffer (which is not a nice thing to do...)
> +#ifdef CONFIG_SYSFS
Not needed, just drop this #ifdef please.
> +static ssize_t show_current_ref(struct class_device *d, char * buf)
> +{
> + unsigned long ret_val;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&event_lock, flags);
> + ret_val = ((inb(TLCLK_REG1) & 0x08) >> 3);
> + spin_unlock_irqrestore(&event_lock, flags);
Odd indentation here. You do this a lot, please fix them all.
> +static int __init tlclk_init(void)
> +{
> + int ret;
> +#ifdef CONFIG_SYSFS
> + struct class_device *class;
> +#endif
Again, please drop all of the #ifdefs from this file, they are not
needed.
> + alarm_events = kcalloc(1, sizeof(struct tlclk_alarms), GFP_KERNEL);
We have kzalloc() now.
> +
> + if (!alarm_events)
> + goto out1;
> +
> +/* Read telecom clock IRQ number (Set by BIOS) */
Indentation is wrong.
> + if( 0 > (ret = misc_register(&tlclk_miscdev )) ) {
Try this instead:
ret = misc_register(&tlclk_miscdev);
if (ret) {
> + printk(KERN_ERR" misc_register retruns %d \n", ret);
> + ret = -EBUSY;
> + goto out3;
> + }
> + class = tlclk_miscdev.class;
> + class_device_create_file(class, &class_device_attr_current_ref);
Try registering a whole attribute group instead. It's much nicer than
the 20 lines you have to register and unregister your devices (and you
don't handle the error condition properly if something goes wrong half
way through.)
> diff -urN -X dontdiff linux-2.6.14-rc2-mm2/drivers/char/tlclk.h linux-2.6.14-rc2-mm2-tlclk/drivers/char/tlclk.h
Why not just put this stuff into the tlclk.c file itself, as it isn't
needed anywhere else?
thanks,
greg k-h
Mark Gross
> > + printk(KERN_ERR" misc_register retruns %d \n", ret);
> > + ret = -EBUSY;
> > + goto out3;
> > + }
> > + class = tlclk_miscdev.class;
> > + class_device_create_file(class, &class_device_attr_current_ref);
>
> Try registering a whole attribute group instead. It's much nicer than
> the 20 lines you have to register and unregister your devices (and you
> don't handle the error condition properly if something goes wrong half
> way through.)
>
I couldn't find such an API that wasn't static to class.c, or described in class.txt. Any pointers on this would be helpful.
> > diff -urN -X dontdiff linux-2.6.14-rc2-mm2/drivers/char/tlclk.h linux-2.6.14-rc2-mm2-tlclk/drivers/char/tlclk.h
>
> Why not just put this stuff into the tlclk.c file itself, as it isn't
> needed anywhere else?
>
done.
Attached is an update that I think addresses your other comments.
Thanks for looking at this.
Greg KH
> On Thursday 06 October 2005 11:20, Greg KH wrote:
> > > +?????????????ret = ?-EBUSY;
> > > +?????????????goto out3;
> > > +?????}
> > > +?????class = tlclk_miscdev.class;
> > > +?????class_device_create_file(class, &class_device_attr_current_ref);
> >
> > Try registering a whole attribute group instead. ?It's much nicer than
> > the 20 lines you have to register and unregister your devices (and you
> > don't handle the error condition properly if something goes wrong half
> > way through.)
> >
>
> I couldn't find such an API that wasn't static to class.c, or
> described in class.txt. Any pointers on this would be helpful.
sysfs_create_group() is what you want.
> +ssize_t tlclk_read(struct file *filp, char __user *buf, size_t count,
> + loff_t *f_pos)
> +{
> + if (count < sizeof(struct tlclk_alarms))
> + return -EIO;
> +
> + wait_event_interruptible(wq, got_event);
> + if (copy_to_user(buf, alarm_events, sizeof(struct tlclk_alarms)))
> + return -EFAULT;
> +
> + memset(alarm_events, 0, sizeof(struct tlclk_alarms));
> + got_event = 0;
> +
> + return sizeof(struct tlclk_alarms);
> +}
Two spaces after the return :(
> +
> +/* Read telecom clock IRQ number (Set by BIOS) */
Indent the comment please.
> + init_timer(&switchover_timer);
> +/* switchover_timer.function = switchover_timeout; */
> +/* switchover_timer.data = 0; */
Remove these commented out lines?
Other than these minor things, and the attribute group stuff, this looks
good.
Alexey Dobriyan
> +config TELCLOCK
> + tristate "Telecom clock driver for ATCA"
> + depends on EXPERIMENTAL
> + default n
> + help
> + The telecom clock device allows direct userspace access to the
> + configuration of the telecom clock configuration settings.
> + This device is used for hardware synchronization across the ATCA
> + backplane fabric.
People usually tell here how the module will be called. See plenty
examples around.
> --- linux-2.6.14-rc2-mm2/drivers/char/tlclk.c
> +++ linux-2.6.14-rc2-mm2-tlclk/drivers/char/tlclk.c
> +Uppon loading the driver will create a sysfs directory under class/misc/tlclk.
Upon.
> +static int __init tlclk_init(void)
> +{
Missing unregister_chrdev() on error unrolling.
> + printk(KERN_ERR" misc_register retruns %d\n", ret);
returns.
> + return 0;
> +out3:
> + release_region(TLCLK_BASE, 8);
> +out2:
> + kfree(alarm_events);
> +out1:
> + return ret;
> +}
Jesper Juhl
> On Thursday 06 October 2005 09:52, Jesper Juhl wrote:
> > > +This directory exports the following interfaces. There opperation is documented
Btw, please spell "operation" correctly :)
[snip]
> > > + printk(KERN_ERR" misc_register retruns %d \n", ret);
Space between 'KERN_ERR" and '"' please.
--
Jesper Juhl <jespe...@gmail.com>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
Gross, Mark
>From: Jesper Juhl [mailto:jespe...@gmail.com]
>Sent: Friday, October 07, 2005 5:21 AM
>To: Mark Gross
>Cc: ak...@osdl.org; linux-...@vger.kernel.org;
Sebastien...@ca.kontron.com; Gross, Mark
>Subject: Re: Telecom Clock Driver for MPCBL0010 ATCA computer blade
>
>On 10/6/05, Mark Gross <mgr...@linux.intel.com> wrote:
>> On Thursday 06 October 2005 09:52, Jesper Juhl wrote:
>> > > +This directory exports the following interfaces. There
opperation is documented
>
>Btw, please spell "operation" correctly :)
>
>[snip]
>> > > + printk(KERN_ERR" misc_register retruns %d \n", ret);
>
>Space between 'KERN_ERR" and '"' please.
>
Ok, I got them both.
I'm now fighting with GKH's request to use sysfs_create_group. I'm not
sure it will work with a misc class device, as its written for the more
common bus based device objects in mind.
Once I get the Greg issue figured out I'll have a new post with all the
updates to make folks happy.
--mgross
Mark Gross
I think I've gotten all the comments addressed with this version.
Most significantly I moved the driver from a misc_device to a platform_device.
Thanks,
Greg KH
>
> Most significantly I moved the driver from a misc_device to a
> platform_device.
You should still use the misc_device to register your file ops, just
stick with the platform device for the sysfs stuff. You need that
misc_device in order to work properly with udev. Have you tested this
code on a udev-only system?
Other than that, it looks very good.
Oh, one minor thing:
> +#include <linux/sysfs.h>
> +#define DEBUG
> +#include <linux/device.h>
Do you always want DEBUG to be enabled? :)
thanks,
greg k-h
Mark Gross
> On Wed, Oct 12, 2005 at 03:30:00PM -0700, Mark Gross wrote:
> >
> > Most significantly I moved the driver from a misc_device to a
> > platform_device.
>
> You should still use the misc_device to register your file ops, just
> stick with the platform device for the sysfs stuff. You need that
> misc_device in order to work properly with udev. Have you tested this
> code on a udev-only system?
I shall tomorrow. Most of the test cases are exercised over the sysfs
interface, the use of the fops are not used very often.
>
> Other than that, it looks very good.
>
> Oh, one minor thing:
>
> > +#include <linux/sysfs.h>
> > +#define DEBUG
> > +#include <linux/device.h>
>
> Do you always want DEBUG to be enabled? :)
No, but I'm glad I tested that otherwise the my problem with using dev_dbg
with the kobj->dev devices I got from the misc_device class could have gotten
by me.
>
> thanks,
>
Thank you.
Greg KH
> No, but I'm glad I tested that otherwise the my problem with using dev_dbg
> with the kobj->dev devices I got from the misc_device class could have gotten
> by me.
Yeah, it's always good to test the code to make sure it compiles :)
This patch looks good, I have no objections to it.
Mark Gross
> On Wed, Oct 12, 2005 at 04:36:29PM -0700, Mark Gross wrote:
> > No, but I'm glad I tested that otherwise the my problem with using dev_dbg
> > with the kobj->dev devices I got from the misc_device class could have
gotten
> > by me.
>
> Yeah, it's always good to test the code to make sure it compiles :)
>
> This patch looks good, I have no objections to it.
>
> thanks,
>
One minor update, after testing with misc_class device for udev I found that
it would be nice to have the sysfs file inodes all use the same base name
"teleco_clock".
Please consider including this driver in the MM tree.
See attached.
Jesper Juhl
> On Wednesday 12 October 2005 18:14, Greg KH wrote:
> > On Wed, Oct 12, 2005 at 04:36:29PM -0700, Mark Gross wrote:
> > > No, but I'm glad I tested that otherwise the my problem with using dev_dbg
> > > with the kobj->dev devices I got from the misc_device class could have
> gotten
> > > by me.
> >
> > Yeah, it's always good to test the code to make sure it compiles :)
> >
> > This patch looks good, I have no objections to it.
> >
> > thanks,
> >
>
> One minor update, after testing with misc_class device for udev I found that
> it would be nice to have the sysfs file inodes all use the same base name
> "teleco_clock".
>
> Please consider including this driver in the MM tree.
>
> See attached.
>
Hi Mark,
I just took a new look at your patch and I have (again) a few small comments...
+static int tlclk_open(struct inode *inode, struct file *filp)
+{
+ int result;
+
+ /* Make sure there is no interrupt pending while
+ * initialising interrupt handler */
+ inb(TLCLK_REG6);
+
+ /* This device is wired through the FPGA IO space of the ATCA blade
+ * we can't share this IRQ */
+ result = request_irq(telclk_interrupt, &tlclk_interrupt,
+ SA_INTERRUPT, "telco_clock", tlclk_interrupt);
+ if (result == -EBUSY) {
+ printk(KERN_ERR "telco_clock: Interrupt can't be reserved!\n");
+ return -EBUSY;
+ }
+ inb(TLCLK_REG6); /* Clear interrupt events */
+
+ return 0;
+}
It seems to me that you can get rid of the "result" variable here by
rewriting the funcion like this :
static int tlclk_open(struct inode *inode, struct file *filp)
{
/* Make sure there is no interrupt pending while
* initialising interrupt handler */
inb(TLCLK_REG6);
/* This device is wired through the FPGA IO space of the ATCA blade
* we can't share this IRQ */
if (-EBUSY == request_irq(telclk_interrupt, &tlclk_interrupt,
SA_INTERRUPT, "telco_clock", tlclk_interrupt)) {
printk(KERN_ERR "telco_clock: Interrupt can't be reserved!\n");
return -EBUSY;
}
inb(TLCLK_REG6); /* Clear interrupt events */
return 0;
}
And btw, what about the other error return values that request_irq can return?
You might get back -ENOMEM or -EINVAL... So shouldn't you rather be
doing something like
result = request_irq(...);
if (result < 0)
/* handle error */
?????
(which then of course would bring the "result" variable back into play ;)
+ssize_t tlclk_read(struct file *filp, char __user *buf, size_t count,
..
+ return sizeof(struct tlclk_alarms);
Why do you have 2 spaces here between "return" and "sizeof..." ?
+static DEVICE_ATTR(current_ref, S_IRUGO, show_current_ref, NULL);
+
+
+static ssize_t show_interrupt_switch(struct device *d,
Surely a single space between these two lines should be enough ;) (ok,
I'm nitpicking, I admit it).
+ unsigned long tmp;
+ unsigned char val;
+ unsigned long flags;
+
+ sscanf(buf, "%lX", &tmp);
+ dev_dbg(d, "tmp = 0x%lX\n", tmp);
+
+ val = (unsigned char)tmp;
You do this a lot, I'm wondering why you don't read directly into
"val" and then get rid of the "tmp" variable?
Maybe I'm missing something, but in tlclk_init() you are calling
request_region() and in case of failure you can end up exiting via the
out3: label which will result in release_region() being called... What
now prevents the release region() in tlclk_cleanup() from being called
on an already released region?
--
Jesper Juhl <jespe...@gmail.com>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
Greg KH
Ick, no, that's a mess. Stick with the original version.
Don't call functions within a if() statement, it's harder to read.
> + unsigned long tmp;
> + unsigned char val;
> + unsigned long flags;
> +
> + sscanf(buf, "%lX", &tmp);
> + dev_dbg(d, "tmp = 0x%lX\n", tmp);
> +
> + val = (unsigned char)tmp;
>
> You do this a lot, I'm wondering why you don't read directly into
> "val" and then get rid of the "tmp" variable?
Because you want to cast it.
thanks,
greg k-h
Jesper Juhl
> On Fri, Oct 14, 2005 at 12:47:21AM +0200, Jesper Juhl wrote:
> > Ick, no, that's a mess. Stick with the original version.
> >
> >Don't call functions within a if() statement, it's harder to read.
>
Whoops, wrong button - sorry. Let's copy LKML.
> > I guess you are right - it /would/ save a variable though ;)
>
> Not worth it. Ease of maintainability, which means being able to read
> the code better, trumps a single variable on a slow path.
>
Hmm, yeah, I can see the sense in that - if it's not performance
critical, better make it readable.
> > > > + unsigned long tmp;
> > > > + unsigned char val;
> > > > + unsigned long flags;
> > > > +
> > > > + sscanf(buf, "%lX", &tmp);
> > > > + dev_dbg(d, "tmp = 0x%lX\n", tmp);
> > > > +
> > > > + val = (unsigned char)tmp;
> > > >
> > > > You do this a lot, I'm wondering why you don't read directly into
> > > > "val" and then get rid of the "tmp" variable?
> > >
> > > Because you want to cast it.
> > >
> > Ok, I'm feeling a little dense tonight, so bear with me please, but
> > wouldn't the effect of reading into the unsigned char (and potentially
> > getting the value truncated) result in the same thing as casting it
> > later?
>
> I don't think it would be the same if you put a large value in the
> string. Try it out and see.
>
Ok, I must admit I haven't actually tested it, perhaps I will tomorrow
after I get some sleep.
--
Jesper Juhl <jespe...@gmail.com>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html