lkml.org 
[lkml]   [2013]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: RFC (v2): Intel QST sensor driver
On 19/03/13 00:27, Guenter Roeck wrote:
> Couple of problems I noticed when browsing through the code.
>
> - Some functions return errors with return code 0.
>
> if (ret <= 0)
> goto out;
> ...
> out:
> return ret;
>
> For values of 0, the calling code will likely miss the error.
Thanks for your helpful comments.

In some of the low-level code I decided to use return 0 to indicate
nothing was transmitted. Probably these situations should be regarded as
an error and -EAGAIN used. I'll check them and fix this.
>
> - In some cases, returned errors are replaced with another error
>
> if (ret < 0)
> return -EIO;
>
> You should return the original error.
>
> - Try using something better than -EIO is possible. For example, you can use
> -EINVAL for invalid parameters.
I'd noticed -EIO was used quite a bit in some existing modules (e.g.
abitguru3.ko) and thought this was a general convention. I'll switch to
using the original return codes.
>
> - Don't use strict_str functions. Use kstr functions instead (checkpatch should
> tell you that, actually).
Ah, I'd run checkpatch on my dev box (which runs 2.6.39), the newer
source trees do indeed flag this up, I'll fix it.
>
> - Try using dev_ messages as much as possible (instead of pr_)
>
> - Try allocating memory with devm_ functions. This way you can drop the matching
> calls to kfree().
The client objects don't contain a struct device. Multiple clients have
a pointer to the underlying supporting device but from what I understand
of devm_kzalloc() that would defer freeing memory until the device is
shut down (which only happens on module unload). That could leave an
increasing amount of memory tied up.
>
> - I notice you use kmalloc() a lot. That is ok if you know that you'll
> initialize all fields, but it is kind of risky. Better use kzalloc().
> (if you start using devm_kzalloc, the issue becomes mostly irrelevant,
> as there is no devm_kmalloc).
>
I'd avoided using kzalloc() when I knew I'd need to initialize members,
but none of the code is on a hot path and it avoids oversights when new
members get added.
>> I've added documents that explain the QST protocol and also the design
>> of the driver.
>>
> For my part I like the architecture of your driver. Wonder how difficult
> it would be to implement the functionality supported by the in-kernel driver
> (eg watchdog) with your infrastructure.
The MEI watchdog? that would be quite straightforward to create a module
for. I had planned to write one but didn't have access to any hardware
with this function.
>
> Overall it would be great if you and Tomas could get together and come up
> with a unified implementation.
>
>
I'd be happy to help getting a driver that fits everybody's needs. The
difficult is there are slight differences in approach. From what I can
see from the QST SDK the kernel driver was written to provide a minimal
implementation with the majority of the logic in a cross-platform
userspace library. My driver was aimed at providing a base to make it
easy to write other kernel modules like the QST one.

There's no reason why an adaptation layer that provides the same
ioctl()/dev interface as the current Intel driver couldn't be created.

Simon


\
 
 \ /
  Last update: 2013-03-19 23:21    [W:0.039 / U:0.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site