Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver

[Nish Aravamudan]

On 9/3/05, Chase Venters <chase.venters@xxxxxxxxxxxx> wrote:
> > Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
> > Smartcard Reader.
> 
> Someone correct me if I'm wrong, but wouldn't these #defines be a problem 
> with the new HZ flexibility:
> 
> #define CCID_DRIVER_BULK_DEFAULT_TIMEOUT        (150*HZ)
> #define CCID_DRIVER_ASYNC_POWERUP_TIMEOUT       (35*HZ)
> #define CCID_DRIVER_MINIMUM_TIMEOUT             (3*HZ)
> #define READ_WRITE_BUFFER_SIZE 512
> #define POLL_LOOP_COUNT                         1000

These are all fine. Although I am a bit suspicious of 150 second
timeouts; but if that is the hardware...

> /* how often to poll for fifo status change */
> #define POLL_PERIOD                             (HZ/100)

This needs to be msecs_to_jiffies(10), please.

> In particular, 2.6.13 allows a HZ of 100, which would define POLL_PERIOD to 0.

Um, 100/100 = 1, not 0?

> Your later calls to mod_timer would be setting cmx_poll_timer to the current
> value of jiffies.

Which is technically ok, because HZ=100, a
     jiffies + 0
or
     jiffies + 1
timeout request will both result in the soft-timer being expired at
the *next* timer interrupt. Regardless, you're right, and
msecs_to_jiffies() will cover it.

> Also, you've got a typo in the comments:
> 
> *       - adhere to linux kenrel coding style and policies
> 
> Forgive me if I'm way off - I'm just now getting my feet wet in kernel
> development. Just making comments based on what I (think) I know at this
> point.

Of bigger concern to me is the use of the sleep_on() family of
functions, all of which are deprecated.

Thanks,
Nish
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/