cyapa.h is not generated, it lives in sources. Use the #include <path/to/driver/cyapa.h> construct.

I am not sure what deadlock do you mean. Is it because cyapa_sc->mutex is the lock for knotes ? Why not use KNOTE_LOCKED() and require the lock on the _notify() entry ?

Is it fine to have lost wakeup there ?

No initialization in the declaration.

if (bus != NULL)

Why this check is needed at all ?

what is this ? why ?

not to mention that we have pause(9)

if (error != 0)

How is it possible ?

This is not needed, sc is initialized by newbus.

Is it possible ?
I think this could be changed to KASSERT if you are so cautious.

Also, shouldn't the attach do cleanup, e.g. call mtx_fini, if it returns error ?

Do you need full kernel process, or could the kernel thread be enough ?

Kernel process creates the whole process structure, in particular, a separate address space. Threads eat less memory.

Why atomic ?

This is ugly and not robust. Look at other examples in kernel how to do this race-free.

How could devnode be NULL ?

How this could be ?

This is N, for N > 1000, attempt to make an exclusive open on the cdev. It does not work (e.g., file descriptor is duped on fork, or it could be passed over the unix domain socket).

If you need to track per-open data, use cdevpriv(9).

But from the quick look over the code, I do not see why do you need this as well. If user opens the device several times, he is stubbing himself into the eye, but kernel is completely fine with the arrangement. So why care ?

If driver did read any data from the device, and later an error occured, the driver must return the read data to userspace, and supress the error.

I won't fix this one, as it's part of the "standard" phrase used in those drivers. I'll change the other occurrences though.

Right now those ranges are not really limited and I can't tell for sure which values will cause which results, therefore I won't add those explanations right now, ok?

It's important to point out that tap to click is only enabled for left and right mouse button events (I'll leave out naming the middle button as well). The background is, that tap to click can be executed by accident quire easily and unintended pasting with the middle button in X can have quite disastrous consequences.

Please verify the new version is ok (it feels awkward style wise)

KNOTE is lockless and is therefore using the global knlist_lock right now. I have to agree that I don't see a reason not to reverse the logic here.

I changed it like you suggested and added sc->mutex as the lock for knotes.

Yes

this was a leftover, removed it

With the stable version of the driver this shouldn't happen, so I removed it. Cleanup is a good point, I moved initialization of the mutex below the last early return to get the same effect.

a thread should work as well, will change this.

I changed the code to use a thread and locking.

I removed all of these now from various functions, assuming that they can only be called will the device is still alive (a lot of defensive coding in this driver tbh).

good call

Entering the while loop, only mtx_sleep interrupted could cause an error, otherwise error will be 0 and data is still in the fifo. So the data is still in there and ready in case the user tries another blocking read (as it only applies to blocking). So this part shouldn't be an issue.

The error returned my uiomove might be a bigger issue (as data might have been read from the fifo) therefore I'll add a condition at the end of the code (basically: if didread error = 0)

Are you sure this applies here? Imho there's nothing read here that could be returned to the user in a useful way.

Use pause(9), for all such tsleeps.

Note that by creating the cdev so early, you expose the devfs node to the user requests before the rest of the initialization is done. Currently there is no way to handle si_drv1 assignment race-free, but the rest could be improved.

No initialization at declaration, check all places.

Still, there is an attempt to enforce single-open. It does not work.

Is it safe to drop the lock this way, you are accessing the driver data unlocked afterward.

And, fifo_read() asserts the lock, from what I see.

Headers should be ordered alphabetically, except param.h being first header, and some exceptions caused by dependencies.

struct cdev *devnode;

Empty line after '{' if no locals. Look for other places with the same style(9) bug.

Assert that the lock is owned.

I think that the comment means that you should check dmiinfo to ensure that you are on the right platform. Otherwise other machine might use 0x67 as the address of the incompatible device.

Put { on the same line as if and as else.

spaces around '|', as should be around any binary op.

This lock could be argued to have some uses, e.g. it ensures that the assignment is seen by other threads taking the sc lock. Also, I suggest to enter the loop locked. Then, you should not unlock after testing sc->detaching. This would also fix the bug of not locking the sc before poll_thread_running = 0 assignment (the unlock is unpaired).

But this lock is useless. The result of the read could be invalidated immediately after the lock is dropped. This is not a serious thing, because all it would do is cause wrong poll frequency for one cycle. So you could just read unlocked and have the same race.

No, I propose to unlock as the first action in the loop body. This + remove unlock in the last line of the loop body.

That makes a lot of sense. I also changed it to a while loop now, so that the condition is at the top.

I might look into that in a separate commit, it sounds interesting.

I don't understand this. So level 1 is not supported, but still the default? Maybe "and" should be "through".

Needs a comma (after a space):
.Xr moused 8 ,

They're all supported, so does this look better?

It supports
.Xr moused 8
levels 0 through 2, level 1 is used by default.

added

This doesn't appear to be used (it's initialized, but nothing ever reads its value).

This looks to be unused?

I think smbus_get_addr() here would be cleaner (cyapa_probe already uses it).