Adding Pimidi kernel module. #6482
Conversation
|
Let the auto-builds complete to look for any obvious problems before submitting any changes, but we would appreciate this PR being split into 3 commits: driver (including Kconfig and Makefile), defconfig files, and the overlay files. |
|
Note: We're not bothered about long lines in overlays, required changes to the list of MAINTAINERS, or new compatible strings requiring dt-bindings changes, but the other checkpatch errors and warnings should be fixed. |
gtrainavicius
force-pushed
the
pimidi_module
branch
from
f811423
to
ca3a536
Compare
|
I have split out the changes into the 3 commits as requested, as well as fixed the patch checks. Some warnings about pr_cont and a logging macro remain, but they are harmless for the module code. |
sound/drivers/pimidi.c
Outdated
| instance->stopping = true; | ||
| cancel_work_sync(&instance->drdy_handler); | ||
| if (irq_initialized) | ||
| devm_free_irq(&client->dev, client->irq, instance); |
There was a problem hiding this comment.
It should not be necessary to call devm_free_irq unless you are intended to release the IRQ before the device is released. Returning an error from the probe method is not one of those situations, so you can delete this line (along with all mentions of irq_initialized).
There was a problem hiding this comment.
You are right, thank you, I've removed the lines in the latest push.
| return; | ||
| } | ||
|
|
||
| if (gpiod_get_value(instance->data_ready_gpio)) { |
There was a problem hiding this comment.
Deciding whether to queue work or enable an interrupt based on the current state of the GPIO feels racy. Are you satisfied that it is safe? I guess it depends on whether the data ready line can spontaneously go high.
I also notice that you are taking care to release the comm_lock mutex before calling this function - why?
There was a problem hiding this comment.
The DRDY line remains high as set by the Pimidi device until all of its output is exhausted, this is a safe check.
The comm mutex is released to let the kernel module communicate with other Pimidi instances.
| goto finalize; | ||
| } | ||
|
|
||
| pimidi_perform_reset(); |
There was a problem hiding this comment.
Can you explain to me why you go to the trouble of resetting the other devices when each instance is probed? I understand that it is a shared reset line, but why can't you just reset when the first instance is probed? Without this, the driver would be much simpler.
There was a problem hiding this comment.
This supports the sudo dtoverlay pimidi sel=x usage when the devices must be loaded dynamically. As the reset is shared, each device must be re-initialized.
There was a problem hiding this comment.
One of us isn't understanding. If the reset is shared, isn't all the hardware reset by the first probe?
There was a problem hiding this comment.
Each device must be initialized by a special 'handshake' via the DRDY GPIO. When a new device is added dynamically to the device tree, the kernel module can't know what is the current state of the new device, therefore, it must reset and do the 'handshake' procedure to all of them all over again, to ensure the state is consistent.
There was a problem hiding this comment.
Keep in mind that when initial device is added via dtoverlay, the kernel module only knows about 1 of the DRDY pins, and a probe gets executed immediately. When a 2nd device gets added, it now knows about 2 DRDY pins, so it must repeat the procedure, as the new device must be reset, and as the reset is a shared line, it must re-initialize all the devices it knew of up till now.
Signed-off-by: Giedrius Trainavičius <[email protected]>
Signed-off-by: Giedrius Trainavičius <[email protected]>
Signed-off-by: Giedrius Trainavičius <[email protected]>
gtrainavicius
force-pushed
the
pimidi_module
branch
from
ca3a536
to
069131b
Compare
|
Thank you! :) |
kernel: media: i2c: imx477: Fix link frequency menu See: raspberrypi/linux#6483 kernel: Adding Pimidi kernel module See: raspberrypi/linux#6482 kernel: Enable Raspberry Touch 2 rotation with overlay See: raspberrypi/linux#6480
kernel: media: i2c: imx477: Fix link frequency menu See: raspberrypi/linux#6483 kernel: Adding Pimidi kernel module See: raspberrypi/linux#6482 kernel: Enable Raspberry Touch 2 rotation with overlay See: raspberrypi/linux#6480
Submitting a new kernel module and its DT overlay for an upcoming Blokas product.