RE: [PATCH RFC v1 07/12] staging: kpc2000: Prepare transfer_complete_cb() for PG_reserved changes
>Right now, ZONE_DEVICE memory is always set PG_reserved. We want to change
>that.
>
>The pages are obtained via get_user_pages_fast(). I assume, these could be
>ZONE_DEVICE pages. Let's just exclude them as well explicitly.
I'm not sure what ZONE_DEVICE pages are, but these pages are normal system RAM,
typically HugePages (but not always).
>
>Cc: Greg Kroah-Hartman
>Cc: Vandana BN
>Cc: "Simon Sandström"
>Cc: Dan Carpenter
>Cc: Nishka Dasgupta
>Cc: Madhumitha Prabakaran
>Cc: Fabio Estevam
>Cc: Matt Sickler
>Cc: Jeremy Sowden
>Signed-off-by: David Hildenbrand
>---
> drivers/staging/kpc2000/kpc_dma/fileops.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c
>b/drivers/staging/kpc2000/kpc_dma/fileops.c
>index cb52bd9a6d2f..457adcc81fe6 100644
>--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
>+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
>@@ -212,7 +212,8 @@ void transfer_complete_cb(struct aio_cb_data *acd, size_t
>xfr_count, u32 flags)
>BUG_ON(acd->ldev->pldev == NULL);
>
>for (i = 0 ; i < acd->page_count ; i++) {
>- if (!PageReserved(acd->user_pages[i])) {
>+ if (!PageReserved(acd->user_pages[i]) &&
>+ !is_zone_device_page(acd->user_pages[i])) {
>set_page_dirty(acd->user_pages[i]);
>}
>}
>--
>2.21.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] KPC2000: kpc2000_spi.c: Fix style issues (line length)
>-Original Message-
>From: devel On Behalf Of
>Chandra Annamaneni
>Sent: Wednesday, October 09, 2019 10:09 PM
>To: gre...@linuxfoundation.org
>Cc: de...@driverdev.osuosl.org; gneuk...@gmail.com; chandra...@gmail.com;
>fabian.krue...@fau.de; linux-
>ker...@vger.kernel.org; si...@nikanor.nu; dan.carpen...@oracle.com
>Subject: [PATCH] KPC2000: kpc2000_spi.c: Fix style issues (line length)
>
>Resoved: "WARNING: line over 80 characters" from checkpatch.pl
>
>Signed-off-by: Chandra Annamaneni
>---
> drivers/staging/kpc2000/kpc2000_spi.c | 20 ++--
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/staging/kpc2000/kpc2000_spi.c
>b/drivers/staging/kpc2000/kpc2000_spi.c
>index 3be33c4..ef78b6d 100644
>--- a/drivers/staging/kpc2000/kpc2000_spi.c
>+++ b/drivers/staging/kpc2000/kpc2000_spi.c
>@@ -30,19 +30,19 @@
> #include "kpc.h"
>
> static struct mtd_partition p2kr0_spi0_parts[] = {
>- { .name = "SLOT_0", .size = 7798784,.offset = 0,
> },
>- { .name = "SLOT_1", .size = 7798784,.offset =
>MTDPART_OFS_NXTBLK},
>- { .name = "SLOT_2", .size = 7798784,.offset =
>MTDPART_OFS_NXTBLK},
>- { .name = "SLOT_3", .size = 7798784,.offset =
>MTDPART_OFS_NXTBLK},
>- { .name = "CS0_EXTRA", .size = MTDPART_SIZ_FULL, .offset =
>MTDPART_OFS_NXTBLK},
>+ { .name = "SLOT_0", .size = 7798784, .offset = 0,},
>+ { .name = "SLOT_1", .size = 7798784, .offset = MTDPART_OFS_NXTBLK},
>+ { .name = "SLOT_2", .size = 7798784, .offset = MTDPART_OFS_NXTBLK},
>+ { .name = "SLOT_3", .size = 7798784, .offset = MTDPART_OFS_NXTBLK},
>+ { .name = "CS0_EXTRA", .size = MTDPART_SIZ_FULL, .offset =
>MTDPART_OFS_NXTBLK},
> };
>
> static struct mtd_partition p2kr0_spi1_parts[] = {
>- { .name = "SLOT_4", .size = 7798784,.offset = 0,
> },
>- { .name = "SLOT_5", .size = 7798784,.offset =
>MTDPART_OFS_NXTBLK},
>- { .name = "SLOT_6", .size = 7798784,.offset =
>MTDPART_OFS_NXTBLK},
>- { .name = "SLOT_7", .size = 7798784,.offset =
>MTDPART_OFS_NXTBLK},
>- { .name = "CS1_EXTRA", .size = MTDPART_SIZ_FULL, .offset =
>MTDPART_OFS_NXTBLK},
>+ { .name = "SLOT_4", .size = 7798784, .offset = 0,},
>+ { .name = "SLOT_5", .size = 7798784, .offset = MTDPART_OFS_NXTBLK},
>+ { .name = "SLOT_6", .size = 7798784, .offset = MTDPART_OFS_NXTBLK},
>+ { .name = "SLOT_7", .size = 7798784, .offset = MTDPART_OFS_NXTBLK},
>+ { .name = "CS1_EXTRA", .size = MTDPART_SIZ_FULL, .offset =
>MTDPART_OFS_NXTBLK},
> };
>
> static struct flash_platform_data p2kr0_spi0_pdata = {
Is the line length limit a hard rule or can exceptions be made? I really feel
that these data tables are more easily read when they're formatted like
tables...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] staging:kpc2000:Fix dubious x | !y sparse warning
>-Original Message- >From: devel On Behalf Of Greg >KH >Sent: Thursday, August 01, 2019 11:35 AM >To: Harsh Jain >Cc: de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org >Subject: Re: [PATCH] staging:kpc2000:Fix dubious x | !y sparse warning > >On Thu, Aug 01, 2019 at 12:06:06AM +0530, Harsh Jain wrote: >> Bitwise OR(|) operation with 0 always yield same result. >> It fixes dubious x | !y sparse warning. >> >> Signed-off-by: Harsh Jain >> --- >> drivers/staging/kpc2000/kpc2000_i2c.c | 16 +--- >> 1 file changed, 1 insertion(+), 15 deletions(-) >> >> diff --git a/drivers/staging/kpc2000/kpc2000_i2c.c >> b/drivers/staging/kpc2000/kpc2000_i2c.c >> index b108da4..5f027d7c 100644 >> --- a/drivers/staging/kpc2000/kpc2000_i2c.c >> +++ b/drivers/staging/kpc2000/kpc2000_i2c.c >> @@ -536,29 +536,15 @@ static u32 i801_func(struct i2c_adapter *adapter) >> >> u32 f = >> I2C_FUNC_I2C | /* 0x0001 (I enabled >> this one) */ >> - !I2C_FUNC_10BIT_ADDR | /* 0x0002 */ >> - !I2C_FUNC_PROTOCOL_MANGLING | /* 0x0004 */ >> ((priv->features & FEATURE_SMBUS_PEC) ? I2C_FUNC_SMBUS_PEC : >> 0) | /* 0x0008 */ >> - !I2C_FUNC_SMBUS_BLOCK_PROC_CALL | /* 0x8000 */ >> I2C_FUNC_SMBUS_QUICK | /* 0x0001 */ >> - !I2C_FUNC_SMBUS_READ_BYTE| /* 0x0002 */ >> - !I2C_FUNC_SMBUS_WRITE_BYTE | /* 0x0004 */ >> - !I2C_FUNC_SMBUS_READ_BYTE_DATA | /* 0x0008 */ >> - !I2C_FUNC_SMBUS_WRITE_BYTE_DATA | /* 0x0010 */ >> - !I2C_FUNC_SMBUS_READ_WORD_DATA | /* 0x0020 */ >> - !I2C_FUNC_SMBUS_WRITE_WORD_DATA | /* 0x0040 */ >> - !I2C_FUNC_SMBUS_PROC_CALL| /* 0x0080 */ >> - !I2C_FUNC_SMBUS_READ_BLOCK_DATA | /* 0x0100 */ >> - !I2C_FUNC_SMBUS_WRITE_BLOCK_DATA | /* 0x0200 */ > >This is ok, it is showing you that these bits are explicitly being not >set. Which is good, now you can go through the list and see that all >are accounted for. > >So I think this should stay as-is, thanks. I was going to say the same thing, but I didn't know what the kernel style guideline was. Would Linus prefer this style or would commenting them out be preferred? Seems like the sparse warnings means the current style is not acceptable? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v3] staging: kpc2000: Convert put_page to put_user_page*()
>From: Bharath Vedartham >Changes since v2 >- Added back PageResevered check as suggested by John Hubbard. > >The PageReserved check needs a closer look and is not worth messing >around with for now. > >Matt, Could you give any suggestions for testing this patch? Myself or someone else from Daktronics would have to do the testing since the hardware isn't really commercially available. I've been toying with the idea of asking for a volunteer from the mailing list to help me out with this - I'd send them some hardware and they'd do all the development and testing. :) I still have to run that idea by Management though. >If in-case, you are willing to pick this up to test. Could you >apply this patch to this tree and test it with your devices? I've been meaning to get to testing the changes to the drivers since upstreaming them, but I've been swamped with other development. I'm keeping an eye on the mailing lists, so I'm at least aware of what is coming down the pipe. I'm not too worried about this specific change, even though I don't really know if the reserved check and the dirtying are even necessary. It sounded like John's suggestion was to not do the PageReserved() check and just use put_user_pges_dirty() all the time. John, is that incorrect? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] staging: kpc2000: Convert put_page() to put_user_page*()
It looks like Outlook is going to absolutely trash this email. Hopefully it
comes through okay.
>> There have been issues with get_user_pages and filesystem writeback.
>> The issues are better described in [1].
>>
>> The solution being proposed wants to keep track of gup_pinned pages
>which will allow to take furthur steps to coordinate between subsystems
>using gup.
>>
>> put_user_page() simply calls put_page inside for now. But the
>implementation will change once all call sites of put_page() are converted.
>>
>> I currently do not have the driver to test. Could I have some suggestions to
>test this code? The solution is currently implemented in [2] and
>> it would be great if we could apply the patch on top of [2] and run some
>tests to check if any regressions occur.
>
>Because this is a common pattern, and because the code here doesn't likely
>need to set page dirty before the dma_unmap_sg call, I think the following
>would be better (it's untested), instead of the above diff hunk:
>
>diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c
>b/drivers/staging/kpc2000/kpc_dma/fileops.c
>index 48ca88bc6b0b..d486f9866449 100644
>--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
>+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
>@@ -211,16 +211,13 @@ void transfer_complete_cb(struct aio_cb_data
>*acd, size_t xfr_count, u32 flags)
>BUG_ON(acd->ldev == NULL);
>BUG_ON(acd->ldev->pldev == NULL);
>
>- for (i = 0 ; i < acd->page_count ; i++) {
>- if (!PageReserved(acd->user_pages[i])) {
>- set_page_dirty(acd->user_pages[i]);
>- }
>- }
>-
>dma_unmap_sg(>ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents,
> acd->ldev->dir);
>
>for (i = 0 ; i < acd->page_count ; i++) {
>- put_page(acd->user_pages[i]);
>+ if (!PageReserved(acd->user_pages[i])) {
>+ put_user_pages_dirty(>user_pages[i], 1);
>+ else
>+ put_user_page(acd->user_pages[i]);
>}
>
>sg_free_table(>sgt);
I don't think I ever really knew the right way to do this.
The changes Bharath suggested look okay to me. I'm not sure about the check
for PageReserved(), though. At first glance it appears to be equivalent to
what was there before, but maybe I should learn what that Reserved page flag
really means.
>From [1], the only comment that seems applicable is
* - MMIO/DMA pages. Some architectures don't allow to ioremap pages that are
* not marked PG_reserved (as they might be in use by somebody else who does
* not respect the caching strategy).
These pages should be coming from anonymous (RAM, not file backed) memory in
userspace. Sometimes it comes from hugepage backed memory, though I don't
think that makes a difference. I should note that transfer_complete_cb handles
both RAM to device and device to RAM DMAs, if that matters.
[1] https://elixir.bootlin.com/linux/v5.2/source/include/linux/page-flags.h#L17
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 0/6] staging: kpc2000_dma: fixes for AIO file-ops.
>-Original Message- >From: Jeremy Sowden >I've had a go at getting the DMA AIO working. It compiles, but I don't >have access to the hardware, so I have done no more testing than that. Honestly, it'd probably be better to just remove the AIO support entirely. The one use case we had that could possibly have benefitted from AIO has been switched away from DMA entirely. We switched because the DMA buffer was a couple hundred bytes and the overhead of setting up the DMA was killing throughput. AIO *might* have been able to help there, but the userspace side of AIO is a PITA to work with. IMO, if "AIO" for the kpc_dma driver were to make a come back, I think it would be better to use something like io_uring The other things that use DMA wouldn't benefit from AIO as they have to setup other parts of the hardware that can't coordinate with the DMA controllers (or at least not without a lot of work). TL;DR: it's probably better to just kill the AIO parts of the driver than to try to make them work. >The fifth patch removes the cancel call-back because it is empty and so >doesn't serve any purpose (AFAICS). However, it doesn't appear to be >too tricky to implement something that would abort the transfer in the >same manner that kpc_dma_close() if this would be useful. It's empty because I didn't have time to figure out how to cancel the DMA operation on the hardware side. Doing the same "reset the whole engine" type of cancel could work, but I'm not sure how well that would mesh with aio_cancel (the latter would kill *all* in-flight operations, the former is only killing the one). As I said above, it's probably better to just remove all the AIO pieces. >Jeremy Sowden (6): > staging: kpc2000_dma: added Kconfig to enable asynchronous I/O. > staging: kpc2000_dma: removed casts of void pointers. > staging: kpc2000_dma: formatting fixes for AIO functions. > staging: kpc2000_dma: replaced aio_(read|write) file-ops with >(read|write)_iter ones. > staging: kpc2000_dma: removed aio cancel call-back. > staging: kpc2000: updated TODO in light of DMA AIO fixes. > > drivers/staging/kpc2000/Kconfig | 8 +++ > drivers/staging/kpc2000/TODO | 4 +- > drivers/staging/kpc2000/kpc_dma/fileops.c | 69 --- > 3 files changed, 44 insertions(+), 37 deletions(-) > >-- >2.20.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 4/6] staging: kpc2000_dma: replaced aio_(read|write) file-ops with (read|write)_iter ones.
>-Original Message-
>From: Jeremy Sowden
>
>The AIO API was implemented in terms of obsolete file-ops. Replaced the
>->aio_read and ->aio_write call-backs with ->read_iter and ->write_iter
>ones. Replaced the call to aio_complete with a call to the ki_complete
>call-back of the kiocb object.
>
>Cc: Matt Sickler
>Signed-off-by: Jeremy Sowden
>---
> drivers/staging/kpc2000/kpc_dma/fileops.c | 40 +++
> 1 file changed, 26 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c
>b/drivers/staging/kpc2000/kpc_dma/fileops.c
>index d74300f14dff..1e8f8c41f82a 100644
>--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
>+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
>@@ -10,6 +10,7 @@
> #include
> #include /* copy_*_user */
> #include /* aio stuff */
>+#include
> #include
> #include
> #include "kpc_dma_driver.h"
>@@ -243,7 +244,7 @@ void transfer_complete_cb(struct aio_cb_data *acd,
>size_t xfr_count, u32 flags)
>}
>} else {
> #ifdef CONFIG_KPC2000_DMA_AIO
>- aio_complete(acd->kcb, acd->len, acd->flags);
>+ acd->kcb->ki_complete(acd->kcb, acd->len, acd->flags);
> #endif
>kfree(acd);
>}
>@@ -319,42 +320,54 @@ static int kpc_dma_aio_cancel(struct kiocb *kcb)
>return 0;
> }
This part was wrapped in the ifdef because aio_complete was removed some time
after 3.16 and I didn't bother with figuring out the replacement for it since
I figured the AIO functionality would be removed entirely.
As for the read_iter/write_iter, my understanding is that's for allowing
scatter-gather type buffers from userspace. If so, that functionality could
be removed entirely. Our use cases have zero need for it, which is why it's
not implemented right now.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v2] staging: kpc2000: kpc_i2c: remove the macros inb_p and outb_p
>-Original Message- >From: devel On Behalf Of >Geordan Neukum > >This inb() call looks like a bug. We perform a 64-bit operation when >talking to this hardware register everywhere else in this driver. Anyone >have more insight into the hardware with which this driver interacts >such that they could shed some light on the subject? That would be me. I looked at the VHDL for the hardware. The registers seem to be aligned to 8 bytes but only use the LS byte of each. So it probably doesn't matter whether the memory transactions are 64-bit or 8-bit. I know the hardware doesn't support byte-enables either, which is probably why the registers were padded this way. Probably also why the inb_p and outb_p macros were redefined. >Probably a separate issue, but I did notice it as a result of this patch. > >Thanks, >Geordan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 1/2] staging: kpc2000: Use '%llx' for printing 'long long int' type
>-Original Message- >From: Fabio Estevam >Hi Greg, > >Please discard this. It fixes arm32 build warning, but introduces >warnings with arm64. > >I will think about a better fix. The hardware/driver will only ever be used on amd64. If it can be totally disabled for any other architecture as an easy fix, that's acceptable to me. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] staging: kpc2000: kpc_dma: fix symbol 'kpc_dma_add_device' was not declared.
>This was reported by sparse:
>drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c:39:7: warning: symbol
>'kpc_dma_add_device
>' was not declared. Should it be static?
>
>Signed-off-by: Valerio Genovese
>---
> drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h
>b/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h
>index ee47f43e71cf..19e88c3bc13f 100644
>--- a/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h
>+++ b/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h
>@@ -56,6 +56,7 @@ struct dev_private_data {
> };
>
> struct kpc_dma_device *kpc_dma_lookup_device(int minor);
>+void kpc_dma_add_device(struct kpc_dma_device *ldev);
>
> extern const struct file_operations kpc_dma_fops;
>
Wouldn't it be better to mark the function static?
It's only used in kpc_dma_driver.c which is where it's defined.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 1/4] staging: kpc2000: add spaces around operators in core.c
>From: devel On Behalf Of >Greg KH >On Fri, May 24, 2019 at 01:07:59PM +0200, Simon Sandström wrote: >> --- a/drivers/staging/kpc2000/kpc2000/core.c >> +++ b/drivers/staging/kpc2000/kpc2000/core.c >> @@ -276,18 +276,18 @@ static ssize_t kp2000_cdev_read(struct file *filp, > >This whole function just needs to be deleted, it's a horrible hack. >From the outside, I would definitely agree with you. On the inside though, we rely on this function to quickly identify what kind and version is running on a given piece of hardware. Since that same information is provided by an ioctl, I could be convinced to remove this API and write a userspace application that uses the ioctl to get the information and pretty prints it. I'd be more inclined to agree with the deletion if it means the whole char dev interface can be removed from the kpc2000 driver. That won't be straightforward as the ioctl is exposed through this interface. We could remove the ioctl, but we'd need to ensure that all the same information is exposed via sysfs. Our userspace side is all funneled through a single class, so changing it to use sysfs wouldn't be too difficult. I'd support that change if someone wants to make it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] staging: kpc2000: simplify nested conditionals that just return a boolean.
>-Original Message-
>From: devel On Behalf Of Greg
>KH
>On Fri, May 24, 2019 at 01:19:26PM +0100, Jeremy Sowden wrote:
>> kp2000_check_uio_irq contained a pair of nested ifs which each evaluated
>> a bitwise operation. If both operations yielded true, the function
>> returned 1; otherwise it returned 0.
>>
>> Replaced the whole thing with one return statement that evaluates the
>> combination of both bitwise operations.
>
>Does this really do the same thing?
>
>>
>> Signed-off-by: Jeremy Sowden
>> ---
>> This applies to staging-testing.
>>
>> I was in two minds whether to send this patch or something less terse:
>>
>> + return (interrupt_active & irq_check_mask) &&
>> +(interrupt_mask_inv & irq_check_mask);
>>
>> drivers/staging/kpc2000/kpc2000/cell_probe.c | 10 --
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c
>b/drivers/staging/kpc2000/kpc2000/cell_probe.c
>> index f731a97c6cac..d10f695b6673 100644
>> --- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
>> +++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
>> @@ -240,12 +240,10 @@ int kp2000_check_uio_irq(struct kp2000_device
>*pcard, u32 irq_num)
>> u64 interrupt_mask_inv = ~readq(pcard->sysinfo_regs_base +
>REG_INTERRUPT_MASK);
>> u64 irq_check_mask = (1 << irq_num);
>>
>> - if (interrupt_active & irq_check_mask) { // if it's active (interrupt
>pending)
>> - if (interrupt_mask_inv & irq_check_mask) {// and if it's
>> not masked
>off
>> - return 1;
>> - }
>> - }
>> - return 0;
>> + /*
>> + * Is the IRQ active (interrupt pending) and not masked off?
>> + */
>> + return (interrupt_active & interrupt_mask_inv & irq_check_mask) != 0;
>
>I have no idea what these bits really are, but try the above with the
>following values:
interrupt_active indicates which IRQs are active/pending
0 = not pending
1 = pending
irq_check_mask has only a single bit set which is which IRQ we're testing for
Each one is "associated" with one of the UIO "cores" (see core_probe.c for how
that association is discovered).
interrupt_mask_inv is a bitwise inversion of the HW register. The HW register
tells the HW which interrupts to ignore.
In the HW register:
0 = pass this IRQ up to the host.
1 = mask the IRQ
In interrupt_mask_inv:
0 = ignore this IRQ
1 = care about this IRQ
This code is checking 3 things:
- Is there an interrupt for this UIO core
- Is that interrupt signaled
- Is the interrupt not masked
Just in case it's not obvious yet: the mask/pending bits allow the HW to catch
an interrupt but not signal the host until the interrupt is unmasked. This
allows interrupts that happen while the IRQ is masked to still be handled once
the IRQ is un-masked.
>interrupt_active = BIT(0);
>interrupt_mask_inv = BIT(1);
>irq_check_mask = (BIT(1) | BIT(0));
>
>So in your new code you have:
>BIT(0) & BIT(1) & (BIT(1) | BIT(0))
>which reduces to:
>0 & (BIT(1) | BIT(0))
>which then reduces to:
>0
>
>The original if statements will return 1.
>Your new one will return 0.
>
>You can't AND interrupt_active with interrupt_mask_inv like you did
>here, right?
>
>Or am I reading this all wrong?
>
>And what's wrong with the original code here? What is complaining about
>it (other than the crazy comment style...)?
I would agree with Greg, if there's nothing complaining about the way the
original code was written it should probably be left that way. This new way
seems overly tricky, even if it was proven to do the same thing.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 02/11] staging: kpc2000: add separate show functions for kpc_uio_class device attributes.
>-Original Message-
>From: devel On Behalf Of
>Define separate simple show functions for each attribute instead of having a
>one big one containing a chain of conditionals.
>
>+static ssize_t s2c_dma_ch_show(struct device *dev,
>+ struct device_attribute *attr, char *buf)
>+{
>+ return 0;
>+}
>+
>+static ssize_t c2s_dma_ch_show(struct device *dev,
>+ struct device_attribute *attr, char *buf)
>+{
>+ return 0;
>+}
These two can be removed. Technically, that would be a userspace-breaking
change, but I can guarantee that all existing userspace consumers don't actually
read that sysfs node.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v2 5/9] staging: kpc2000: use atomic_t to assign card numbers.
>-Original Message- >From: devel On Behalf Of >Previously the next card number was assigned from a static int local variable, >which was read and later incremented. This was not thread- safe, so now we >use an atomic_t and atomic_fetch_add instead. Switching to atomic_fetch_add is definitely an improvement over what that code was doing prior, but is that the proper solution? How do other parts of the kernel handle giving devices unique ID numbers? Honestly, the atomic_t solution might be "good enough". Our PCIe devices get removed and reprobed at least once per boot. We do this so they boot into a "bootloader" program so we can verify that the "production" image stored in the on-board flash is the correct type/version. We then tell the card to reconfigure itself while we remove the PCIe device and then rescan the PCIe bus for the "new" device. That ends up increasing this card count more. This would never be a problem in production (given that we only do this maybe a half dozen times per boot, worst case). Even in dev, we've never reconfigured enough times for this counter to overflow. That was maybe rambling a bit, just wanted to point it out in case there's a "proper" way we should be doing this. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v2] kpc_i2c: Remove unused file
>-Original Message- >From: Dan Carpenter > >Add Staging: to the subject. Added to my notes so I don't forget it next time. >[PATCH v2] Staging: kpc_i2c: Remove unused file fileops.c > >On Thu, May 09, 2019 at 01:38:27PM +, Matt Sickler wrote: >> The whole file was wrapped in an #if 0. I'm guessing it was a >> leftover file from when we were first developing the driver and we just >forgot about it. >> >> V2: Forgot the signed-off-by line on the first patch. > >Put this after the --- cut off line > >> >> Signed-off-by: Matt Sickler >> --- > ^^^ > >Here. Noted. I just looked up a "v2" patch in the mailing list archive to see what that looks like. I'll try to do that next time. > >There is something else wrong with the patch and it's corrupted a bit or >something. Please read the first paragraph of Documentation/process/email- >clients.rst Ugh. I'm about to throw Outlook in the trash and just use my personal email account. I know most subsystem maintainers don't accept pull requests but Daktronics does have a github account that I could push my changes to and use git-request-pull to ask Greg to pull from. Greg, would that be an acceptable solution? If not, I can continue struggle-bussing with Outlook. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v2] kpc_i2c: Remove unused file
>-Original Message- >From: Dan Carpenter > >On Thu, May 09, 2019 at 02:47:50PM +, Matt Sickler wrote: > >A few people/subsystems (DRM) put the change log in the commit message >but that's pretty weird and I don't know if they do it on purpose or >they're just not aware how to do it properly... :P > >When it's under the --- then it isn't stored in the permanent git log. Good to know the "why" behind that process! >> Ugh. I'm about to throw Outlook in the trash and just use >> my personal email account. >> I know most subsystem maintainers don't accept pull requests >> but Daktronics does have a github account that I could push >> my changes to and use git-request-pull to ask Greg to pull >> from. Greg, would that be an acceptable solution? If not, >> I can continue struggle-bussing with Outlook. > >You can't just use git send-email? Outlook is going to give you >headaches forever. I use mutt, but one subsystem only accept patches >from git send-email so I have to add a fake "X-Mailer: git-send-email" >header to my patches... I guess I have not tried. Daktronics uses Outlook and Office 365 and lots of "security" junk like Okta 2FA. I had assumed that mutt wouldn't work because of those settings. Our IT is very much windows-only, so they probably wouldn't make any exceptions for me to allow mutt to work. I'll give it a try though. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] kpc_i2c: Remove unused file
The whole file was wrapped in an #if 0. I'm guessing it was a leftover file
from when we were first developing the driver and we just forgot about it.
V2: Forgot the signed-off-by line on the first patch.
Signed-off-by: Matt Sickler
---
drivers/staging/kpc2000/kpc_i2c/Makefile | 2 +-
drivers/staging/kpc2000/kpc_i2c/fileops.c | 181 --
2 files changed, 1 insertion(+), 182 deletions(-) delete mode 100644
drivers/staging/kpc2000/kpc_i2c/fileops.c
diff --git a/drivers/staging/kpc2000/kpc_i2c/Makefile
b/drivers/staging/kpc2000/kpc_i2c/Makefile
index 73ec07ac7d39..63a6ce4b8e03 100644
--- a/drivers/staging/kpc2000/kpc_i2c/Makefile
+++ b/drivers/staging/kpc2000/kpc_i2c/Makefile
@@ -1,4 +1,4 @@
# SPDX-License-Identifier: GPL-2.0
obj-m := kpc2000_i2c.o
-kpc2000_i2c-objs := i2c_driver.o fileops.o
+kpc2000_i2c-objs := i2c_driver.o
diff --git a/drivers/staging/kpc2000/kpc_i2c/fileops.c
b/drivers/staging/kpc2000/kpc_i2c/fileops.c
deleted file mode 100644
index e749c0994491..
--- a/drivers/staging/kpc2000/kpc_i2c/fileops.c
+++ /dev/null
@@ -1,181 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-#if 0
-#include
-#include
-#include /* printk() */
-#include /* kmalloc() */
-#include /* everything... */
-#include/* error codes */
-#include/* size_t */
-#include
-#include/* copy_*_user */
-
-#include "i2c_driver.h"
-
-int i2c_cdev_open(struct inode *inode, struct file *filp) -{
- struct i2c_device *lddev;
-
- if(NULL == inode) {
-//printk(KERN_WARNING " i2c_cdev_open: inode is a NULL pointer\n");
-DBG_PRINT(KERN_WARNING, "i2c_cdev_open: inode is a NULL pointer\n");
-return -EINVAL;
- }
- if(NULL == filp) {
-//printk(KERN_WARNING " i2c_cdev_open: filp is a NULL pointer\n");
-DBG_PRINT(KERN_WARNING, "i2c_cdev_open: filp is a NULL pointer\n");
-return -EINVAL;
- }
-
- lddev = container_of(inode->i_cdev, struct i2c_device, cdev);
- //printk(KERN_DEBUG " i2c_cdev_open(filp = [%p], lddev = [%p])\n",
filp, lddev);
- DBG_PRINT(KERN_DEBUG, "i2c_cdev_open(filp = [%p], lddev = [%p])\n", filp,
lddev);
-
- filp->private_data = lddev; /* so other methods can access it */
-
- return 0;/* success */
-}
-
-int i2c_cdev_close(struct inode *inode, struct file *filp) -{
- struct i2c_device *lddev;
-
- if(NULL == inode) {
-//printk(KERN_WARNING " i2c_cdev_close: inode is a NULL
pointer\n");
-DBG_PRINT(KERN_WARNING, "i2c_cdev_close: inode is a NULL pointer\n");
-return -EINVAL;
- }
- if(NULL == filp) {
-//printk(KERN_WARNING " i2c_cdev_close: filp is a NULL pointer\n");
-DBG_PRINT(KERN_WARNING, "i2c_cdev_close: filp is a NULL pointer\n");
-return -EINVAL;
- }
-
- lddev = filp->private_data;
- //printk(KERN_DEBUG " i2c_cdev_close(filp = [%p], lddev = [%p])\n",
filp, lddev);
- DBG_PRINT(KERN_DEBUG, "i2c_cdev_close(filp = [%p], lddev = [%p])\n", filp,
lddev);
-
- return 0;
-}
-
-ssize_t i2c_cdev_read(struct file *filp, char __user *buf, size_t count,
loff_t *f_pos) -{
- size_t copy;
- ssize_t ret = 0;
- int err = 0;
- u64 read_val;
- char tmp_buf[48] = { 0 };
- struct i2c_device *lddev = filp->private_data;
-
- if(NULL == filp) {
-//printk(KERN_WARNING " i2c_cdev_read: filp is a NULL pointer\n");
-DBG_PRINT(KERN_WARNING, "i2c_cdev_read: filp is a NULL pointer\n");
-return -EINVAL;
- }
- if(NULL == buf) {
-//printk(KERN_WARNING " i2c_cdev_read: buf is a NULL pointer\n");
-DBG_PRINT(KERN_WARNING, "i2c_cdev_read: buf is a NULL pointer\n");
-return -EINVAL;
- }
- if(NULL == f_pos) {
-//printk(KERN_WARNING " i2c_cdev_read: f_pos is a NULL pointer\n");
-DBG_PRINT(KERN_WARNING, "i2c_cdev_read: f_pos is a NULL pointer\n");
-return -EINVAL;
- }
-
- if(count < sizeof(tmp_buf)) {
-//printk(KERN_INFO " i2c_cdev_read: buffer is too small (count =
%d, should be at least %d bytes)\n", (int)count, (int)sizeof(tmp_buf));
-DBG_PRINT(KERN_INFO, "i2c_cdev_read: buffer is too small (count = %d,
should be at least %d bytes)\n", (int)count, (int)sizeof(tmp_buf));
-return -EINVAL;
- }
- if(((*f_pos * 8) + lddev->pldev->resource[0].start) >
lddev->pldev->resource[0].end) {
-//printk(KERN_INFO " i2c_cdev_read: bad read addr %016llx\n",
(*f_pos * 8) + lddev->pldev->resource[0].start);
-DBG_PRINT(KERN_INFO, "i2c_cdev_read: bad read addr %016llx\n", (*f_pos *
8) + lddev->pldev->resource[0].start);
-//printk(KERN_INFO " i2c_cdev_read: addr end %016llx\n",
lddev->pldev->resource[0].end);
-DBG_PRINT(KERN_INFO, "i2c_cdev_read: addr end %016llx\n",
lddev->pldev->resource[0].end);
-//printk(KERN_INFO "
[PATCH] kpc_i2c: Remove unused file
The whole file was wrapped in an #if 0. I'm guessing it was a leftover file
from when we were first developing the driver and we just forgot about it.
---
drivers/staging/kpc2000/kpc_i2c/Makefile | 2 +-
drivers/staging/kpc2000/kpc_i2c/fileops.c | 181 --
2 files changed, 1 insertion(+), 182 deletions(-)
delete mode 100644 drivers/staging/kpc2000/kpc_i2c/fileops.c
diff --git a/drivers/staging/kpc2000/kpc_i2c/Makefile
b/drivers/staging/kpc2000/kpc_i2c/Makefile
index 73ec07ac7d39..63a6ce4b8e03 100644
--- a/drivers/staging/kpc2000/kpc_i2c/Makefile
+++ b/drivers/staging/kpc2000/kpc_i2c/Makefile
@@ -1,4 +1,4 @@
# SPDX-License-Identifier: GPL-2.0
obj-m := kpc2000_i2c.o
-kpc2000_i2c-objs := i2c_driver.o fileops.o
+kpc2000_i2c-objs := i2c_driver.o
diff --git a/drivers/staging/kpc2000/kpc_i2c/fileops.c
b/drivers/staging/kpc2000/kpc_i2c/fileops.c
deleted file mode 100644
index e749c0994491..
--- a/drivers/staging/kpc2000/kpc_i2c/fileops.c
+++ /dev/null
@@ -1,181 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-#if 0
-#include
-#include
-#include /* printk() */
-#include /* kmalloc() */
-#include /* everything... */
-#include/* error codes */
-#include/* size_t */
-#include
-#include/* copy_*_user */
-
-#include "i2c_driver.h"
-
-int i2c_cdev_open(struct inode *inode, struct file *filp)
-{
- struct i2c_device *lddev;
-
- if(NULL == inode) {
-//printk(KERN_WARNING " i2c_cdev_open: inode is a NULL pointer\n");
-DBG_PRINT(KERN_WARNING, "i2c_cdev_open: inode is a NULL pointer\n");
-return -EINVAL;
- }
- if(NULL == filp) {
-//printk(KERN_WARNING " i2c_cdev_open: filp is a NULL pointer\n");
-DBG_PRINT(KERN_WARNING, "i2c_cdev_open: filp is a NULL pointer\n");
-return -EINVAL;
- }
-
- lddev = container_of(inode->i_cdev, struct i2c_device, cdev);
- //printk(KERN_DEBUG " i2c_cdev_open(filp = [%p], lddev = [%p])\n",
filp, lddev);
- DBG_PRINT(KERN_DEBUG, "i2c_cdev_open(filp = [%p], lddev = [%p])\n", filp,
lddev);
-
- filp->private_data = lddev; /* so other methods can access it */
-
- return 0;/* success */
-}
-
-int i2c_cdev_close(struct inode *inode, struct file *filp)
-{
- struct i2c_device *lddev;
-
- if(NULL == inode) {
-//printk(KERN_WARNING " i2c_cdev_close: inode is a NULL
pointer\n");
-DBG_PRINT(KERN_WARNING, "i2c_cdev_close: inode is a NULL pointer\n");
-return -EINVAL;
- }
- if(NULL == filp) {
-//printk(KERN_WARNING " i2c_cdev_close: filp is a NULL pointer\n");
-DBG_PRINT(KERN_WARNING, "i2c_cdev_close: filp is a NULL pointer\n");
-return -EINVAL;
- }
-
- lddev = filp->private_data;
- //printk(KERN_DEBUG " i2c_cdev_close(filp = [%p], lddev = [%p])\n",
filp, lddev);
- DBG_PRINT(KERN_DEBUG, "i2c_cdev_close(filp = [%p], lddev = [%p])\n", filp,
lddev);
-
- return 0;
-}
-
-ssize_t i2c_cdev_read(struct file *filp, char __user *buf, size_t count,
loff_t *f_pos)
-{
- size_t copy;
- ssize_t ret = 0;
- int err = 0;
- u64 read_val;
- char tmp_buf[48] = { 0 };
- struct i2c_device *lddev = filp->private_data;
-
- if(NULL == filp) {
-//printk(KERN_WARNING " i2c_cdev_read: filp is a NULL pointer\n");
-DBG_PRINT(KERN_WARNING, "i2c_cdev_read: filp is a NULL pointer\n");
-return -EINVAL;
- }
- if(NULL == buf) {
-//printk(KERN_WARNING " i2c_cdev_read: buf is a NULL pointer\n");
-DBG_PRINT(KERN_WARNING, "i2c_cdev_read: buf is a NULL pointer\n");
-return -EINVAL;
- }
- if(NULL == f_pos) {
-//printk(KERN_WARNING " i2c_cdev_read: f_pos is a NULL pointer\n");
-DBG_PRINT(KERN_WARNING, "i2c_cdev_read: f_pos is a NULL pointer\n");
-return -EINVAL;
- }
-
- if(count < sizeof(tmp_buf)) {
-//printk(KERN_INFO " i2c_cdev_read: buffer is too small (count =
%d, should be at least %d bytes)\n", (int)count, (int)sizeof(tmp_buf));
-DBG_PRINT(KERN_INFO, "i2c_cdev_read: buffer is too small (count = %d,
should be at least %d bytes)\n", (int)count, (int)sizeof(tmp_buf));
-return -EINVAL;
- }
- if(((*f_pos * 8) + lddev->pldev->resource[0].start) >
lddev->pldev->resource[0].end) {
-//printk(KERN_INFO " i2c_cdev_read: bad read addr %016llx\n",
(*f_pos * 8) + lddev->pldev->resource[0].start);
-DBG_PRINT(KERN_INFO, "i2c_cdev_read: bad read addr %016llx\n", (*f_pos *
8) + lddev->pldev->resource[0].start);
-//printk(KERN_INFO " i2c_cdev_read: addr end %016llx\n",
lddev->pldev->resource[0].end);
-DBG_PRINT(KERN_INFO, "i2c_cdev_read: addr end %016llx\n",
lddev->pldev->resource[0].end);
-//printk(KERN_INFO " i2c_cdev_read: EOF reached\n");
-DBG_PRINT(KERN_INFO, "i2c_cdev_read: EOF reached\n");
-return 0;
- }
-
- down_read(>rw_sem);
-
- read_val = *(lddev->regs + *f_pos);
- copy = clamp_t(size_t, count, 1, sizeof(tmp_buf));
- copy = scnprintf(tmp_buf, copy, "reg: 0x%x val: 0x%llx\n", (unsigned
int)*f_pos, read_val);
- err = copy_to_user(buf, tmp_buf, copy);
- if(err)
RE: RFC: kpc2000 driver naming
>-Original Message- >From: 'gre...@linuxfoundation.org' >On Sun, May 05, 2019 at 10:14:17PM +0000, Matt Sickler wrote: >> The I2C and SPI drivers don't depend on anything other than the I2C >> and SPI subsystems. Actually, they might be depending on the kp2000 >> driver having the PCIe registers mapped into kernel space instead of doing >> that themselves. >> I'm not sure if that's the correct thing to do or not, so that might >> be something to look closely at with all these drivers. > >Are all of these drivers needed for this hardware to work? Should they even >be separate drivers or should they all be mushed into one? Can anyone do >anything useful with just one of them? > >> Yes, all 4 drivers are required for proper functioning of the card. >> SPI is used to reprogram the flash chips that store the FPGA >> bitstream. I2C is used for monitoring and programming clock >> generators. DMA is required for some parts of other cores. > >So should we just merge this into one driver at link time? That would make >more sense, right? Yes. All the drivers are required for the hardware to work. In some sense, they "could" be used independently, but most likely only within Daktronics hardware. I guess if someone else had an FPGA design that needed a SPI controller, they could reuse our driver as long as their FPGA implemented a compatible SPI controller. One thing I would be concerned with would be future FPGA designs that need to mix-and-match. For example (using new names), today we have the P2K card which uses the dak-p2k main driver, and dak-i2c, dak-spi, and dak-dma "sub-drivers". Perhaps the next generation hardware would need to use a new dak-p3k main driver but can reuse the dak-i2c and dak-dma sub-drivers. And maybe it needs a new dak-spi-v2 driver (because something in the hardware changed in an incompatible way). This is all hypothetical though - it could range from complete driver reuse to needing all new drivers for everything - we won't know for sure until the new hardware designs ramp up in the next 6-12 months. If there's a way to do link-time trickery to get all 4 drivers compiled into one .ko, I'd be fine with that. I do think it's a good idea to keep them at least slightly separated to facilitate that mix-and-match scenario as well as just ease of maintaining the code. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: RFC: kpc2000 driver naming
>-Original Message- >From: 'gre...@linuxfoundation.org' >On Fri, May 03, 2019 at 10:24:00PM +0000, Matt Sickler wrote: >> Hello, >> >> Recently Greg KH posted the first set of drivers for our PCIe device >(kpc2000) and shortly after that I posted the kpc2000_dma driver. I was >wondering about naming / structure standards in the Linux kernel. >> First, a real quick background on these devices: Daktronics makes a PCIe >card with an FPGA on it to drive our LED displays (and other processing tasks). >Inside the FPGA, we use something similar to AXI-4 to divide the PCIe BAR >register space [1] into separate "IP cores". The kpc2000 driver is responsible >for probing the PCIe device, doing some basic setup (mapping the BAR, >setting up an IRQ, PCIe configuration, etc) and then enumerating these >"cores". Enumeration of the cores is facilitated by the "board info" core >that is >always at the beginning of the BAR and has a defined format. Most of the >cores are controlled entirely by userspace - the driver will add a UIO sub >device for each one which userspace uses to control FPGA registers. Only 3 >core types are handled by drivers: DMA, I2C, SPI. These are IP cores inside >the FPGA that (in the case of i2c and spi) interact with other physical devices >on the PCIe card. >> Currently, we only have the one PCIe device (the "P2K" card) but we have >more on our roadmap (one we've been calling "p3k" internally). I'm 99% >confident that the I2C and SPI cores will be exactly the same on the new FPGA >design. I'm 80% confident that the DMA engines themselves will be exactly >the same on the new FPGA design. The next card PCIe driver will quite likely >be separate from the kpc2000 driver because how bitstreams are stored / >loaded / configured is changing due to using a newer FPGA. There will likely >be common code between the two. > >Please wrap your emails at a sane column, otherwise this is just a huge wall of >text that is hard to read/understand. We use Outlook and Office 365, so I figured the emails were going to be formatted badly. Just for clarity, are you saying I should hard wrap (insert newlines myself) at an 80-column boundary? >> Now on to my actual questions: Once the drivers are "good enough" to be >moved outside of staging, I'm wondering where the drivers will end up and >what their names will/should be. >> Since the I2C and SPI drivers are single-file, I'm guessing they're going to >move to drivers/i2c/busses/i2c-dak/ and drivers/spi/spi-dak/, respectively. I >tweaked the names, since "i2c-dak" and "spi-dak" make more sense to me >than "kpc_i2c" and "kpc_spi". > >Feel free to rename them to whatever you want, I just randomly picked a >name when I did the import of the drivers. > >> So that leaves the DMA and main PCIe drivers. Where do those end up in >the tree? Would "dak-dma" and "dak-p2k" (and eventually "dak-p3k") make >more sense as names for those drivers? > >Maybe, as long as it is a "unique" name, that's all that should matter. >The subsystem maintainers of those areas might care more, but you can deal >with that when you get closer to moving the code out of staging. > >> The final question relates to how Kconfig entries are setup. The >> I2C, SPI, and DMA drivers could be "selected" on their own (even if >> the "dak-p2k" and "dak-p3k" drivers aren't selected), but that doesn't >> make much sense because they'd never get used in that configuration. >> Conversely, if you select the "dak-p2k" driver, the I2C, SPI, and DMA >> drivers better get selected too, otherwise the device won't function >> correctly. From what I can tell with Kconfig, if A depends on B, you >> can't even see (let alone select) A without already selecting B. >> Right now, the Kconfig entries are setup like this (using the current names, >not the new ones presented above): >> KPC2000_DMA depends on KPC2000 (this compiles the kpc2000_dma >driver) >> KPC2000_I2C depends on KPC2000 && I2C (this compiles the kpc2000_i2c >driver) >> KPC2000_SPI depends on KPC2000 && SPI (this compiles the kpc2000_spi >driver) >> KPC2000_CORE depends on KPC2000 >> KPC2000 depends on PCI (this compiles the kpc2000 driver) Greg, >> what is the purpose of the KPC2000_CORE config option? Nothing (that I >see) depends on it, and it doesn't cause any code to get compiled. > >I don't remember, I guess I thought that was a chunk of code the others all >depended on being present? If that's not the cas
RFC: kpc2000 driver naming
Hello, Recently Greg KH posted the first set of drivers for our PCIe device (kpc2000) and shortly after that I posted the kpc2000_dma driver. I was wondering about naming / structure standards in the Linux kernel. First, a real quick background on these devices: Daktronics makes a PCIe card with an FPGA on it to drive our LED displays (and other processing tasks). Inside the FPGA, we use something similar to AXI-4 to divide the PCIe BAR register space [1] into separate "IP cores". The kpc2000 driver is responsible for probing the PCIe device, doing some basic setup (mapping the BAR, setting up an IRQ, PCIe configuration, etc) and then enumerating these "cores". Enumeration of the cores is facilitated by the "board info" core that is always at the beginning of the BAR and has a defined format. Most of the cores are controlled entirely by userspace - the driver will add a UIO sub device for each one which userspace uses to control FPGA registers. Only 3 core types are handled by drivers: DMA, I2C, SPI. These are IP cores inside the FPGA that (in the case of i2c and spi) interact with other physical devices on the PCIe card. Currently, we only have the one PCIe device (the "P2K" card) but we have more on our roadmap (one we've been calling "p3k" internally). I'm 99% confident that the I2C and SPI cores will be exactly the same on the new FPGA design. I'm 80% confident that the DMA engines themselves will be exactly the same on the new FPGA design. The next card PCIe driver will quite likely be separate from the kpc2000 driver because how bitstreams are stored / loaded / configured is changing due to using a newer FPGA. There will likely be common code between the two. Now on to my actual questions: Once the drivers are "good enough" to be moved outside of staging, I'm wondering where the drivers will end up and what their names will/should be. Since the I2C and SPI drivers are single-file, I'm guessing they're going to move to drivers/i2c/busses/i2c-dak/ and drivers/spi/spi-dak/, respectively. I tweaked the names, since "i2c-dak" and "spi-dak" make more sense to me than "kpc_i2c" and "kpc_spi". So that leaves the DMA and main PCIe drivers. Where do those end up in the tree? Would "dak-dma" and "dak-p2k" (and eventually "dak-p3k") make more sense as names for those drivers? The final question relates to how Kconfig entries are setup. The I2C, SPI, and DMA drivers could be "selected" on their own (even if the "dak-p2k" and "dak-p3k" drivers aren't selected), but that doesn't make much sense because they'd never get used in that configuration. Conversely, if you select the "dak-p2k" driver, the I2C, SPI, and DMA drivers better get selected too, otherwise the device won't function correctly. From what I can tell with Kconfig, if A depends on B, you can't even see (let alone select) A without already selecting B. Right now, the Kconfig entries are setup like this (using the current names, not the new ones presented above): KPC2000_DMA depends on KPC2000 (this compiles the kpc2000_dma driver) KPC2000_I2C depends on KPC2000 && I2C (this compiles the kpc2000_i2c driver) KPC2000_SPI depends on KPC2000 && SPI (this compiles the kpc2000_spi driver) KPC2000_CORE depends on KPC2000 KPC2000 depends on PCI (this compiles the kpc2000 driver) Greg, what is the purpose of the KPC2000_CORE config option? Nothing (that I see) depends on it, and it doesn't cause any code to get compiled. I would have thought something like this makes more sense [2]: KPC2000_DMA depends nothing KPC2000_I2C depends on I2C KPC2000_SPI depends on SPI KPC2000 depends on PCI && KPC2000_DMA && KPC2000_I2C && KPC2000_SPI Which way is "better"? Does it even matter which way it's setup? [1] Technically, there are two BARs. The first one is primarily registers for the DMA engines. The second one is dedicated to our IP cores. [2] Using the new naming, and taking the p3k future into account: DAK_DMA depends on nothing DAK_I2C depends on I2C DAK_SPI depends on SPI DAK_P2K depends on PCI && DAK_DMA && DAK_I2C && DAK_SPI DAK_P3K depends on PCI && DAK_DMA && DAK_I2C && DAK_SPI Thanks, Matt Sickler ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH RFC V2] staging: kpc2000: use int for wait_for_completion_interruptible
ACK. However, that part isn't the only part of that function that uses
"return rv" though.
There's another part that does "rv = get_user_pages(...)" and get_user_pages()
returns a long.
Does this same kind of change need to happen for that case?
>-Original Message-
>From: Nicholas Mc Guire
>Sent: Saturday, April 27, 2019 4:15 AM
>To: Greg Kroah-Hartman
>Cc: Matt Sickler ; de...@driverdev.osuosl.org;
>linux-ker...@vger.kernel.org; Nicholas Mc Guire
>Subject: [PATCH RFC V2] staging: kpc2000: use int for
>wait_for_completion_interruptible
>
>
>weit_for_completion_interruptible returns in (0 on completion and -
>ERESTARTSYS on interruption) - so use an int not long for API conformance
>and simplify the logic here a bit: need not check explicitly for == 0 as this
>is
>either -ERESTARTSYS or 0.
>
>Signed-off-by: Nicholas Mc Guire
>---
>
>Problem located with experimental API conformance checking cocci script
>
>V2: kbuild reported a missing closing comment - seems that I managed
>send out the the initial version before compile testing/checkpatching
>sorry for the noise.
>
>Not sure if making such point-wise fixes makes much sense - this driver has a
>number of issues both style-wise and API compliance wise.
>
>Note that kpc_dma_transfer() returns int not long - currently rv (long) is
>being
>returned in most places (some places do return int) - so the return handling
>here is a bit inconsistent.
>
>Patch was compile-tested with: x86_64_defconfig + KPC2000=y,
>KPC2000_DMA=y (with a number of unrelated sparse warnings about non-
>declared symbols, and smatch warnings about overflowing constants as well
>as coccicheck warning about usless casting)
>
>Patch is against 5.1-rc6 (localversion-next is next-20190426)
>
> drivers/staging/kpc2000/kpc_dma/fileops.c | 16 +---
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c
>b/drivers/staging/kpc2000/kpc_dma/fileops.c
>index 5741d2b..66f0d5a 100644
>--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
>+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
>@@ -38,6 +38,7 @@ int kpc_dma_transfer(struct dev_private_data *priv,
>struct kiocb *kcb, unsigned {
>unsigned int i = 0;
>long rv = 0;
>+ int ret = 0;
>struct kpc_dma_device *ldev;
>struct aio_cb_data *acd;
>DECLARE_COMPLETION_ONSTACK(done); @@ -180,16 +181,17 @@ int
>kpc_dma_transfer(struct dev_private_data *priv, struct kiocb *kcb, unsigned
>
>// If this is a synchronous kiocb, we need to put the calling process
> to
>sleep until the transfer is complete
>if (kcb == NULL || is_sync_kiocb(kcb)){
>- rv = wait_for_completion_interruptible();
>- // If the user aborted (rv == -ERESTARTSYS), we're no longer
>responsible for cleaning up the acd
>- if (rv == -ERESTARTSYS){
>+ ret = wait_for_completion_interruptible();
>+ /* If the user aborted (ret == -ERESTARTSYS), we're
>+* no longer responsible for cleaning up the acd
>+*/
>+ if (ret) {
>acd->cpl = NULL;
>- }
>- if (rv == 0){
>- rv = acd->len;
>+ } else {
>+ ret = acd->len;
>kfree(acd);
>}
>- return rv;
>+ return ret;
>}
>
>return -EIOCBQUEUED;
>--
>2.1.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: kpc2000: Add DMA driver
Add Daktronics DMA driver. I've added the SPDX license identifiers, Kconfig
entry, and cleaned up as many of the warnings as I could.
The AIO support code will be removed in a future patch.
Signed-off-by: Matt Sickler
---
drivers/staging/kpc2000/Kconfig | 11 +
drivers/staging/kpc2000/Makefile | 1 +
drivers/staging/kpc2000/kpc_dma/Makefile | 6 +
drivers/staging/kpc2000/kpc_dma/dma.c| 264 ++
drivers/staging/kpc2000/kpc_dma/fileops.c| 420 +++
drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c | 248 +
drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h | 220
drivers/staging/kpc2000/kpc_dma/uapi.h | 11 +
8 files changed, 1181 insertions(+)
create mode 100644 drivers/staging/kpc2000/kpc_dma/Makefile
create mode 100644 drivers/staging/kpc2000/kpc_dma/dma.c
create mode 100644 drivers/staging/kpc2000/kpc_dma/fileops.c
create mode 100644 drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c
create mode 100644 drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h
create mode 100644 drivers/staging/kpc2000/kpc_dma/uapi.h
diff --git a/drivers/staging/kpc2000/Kconfig b/drivers/staging/kpc2000/Kconfig
index 926e770d6e0e..fb5922928f47 100644
--- a/drivers/staging/kpc2000/Kconfig
+++ b/drivers/staging/kpc2000/Kconfig
@@ -44,3 +44,14 @@ config KPC2000_I2C
If unsure, say N.
+config KPC2000_DMA
+ tristate "Daktronics KPC DMA controller"
+ depends on KPC2000
+ help
+ Say Y here if you wish to support the Daktronics DMA controller.
+
+ To compile this driver as a module, choose M here: the module
+ will be called kpc2000_dma
+
+ If unsure, say N.
+
diff --git a/drivers/staging/kpc2000/Makefile b/drivers/staging/kpc2000/Makefile
index 6fcb2ee7b27d..1e48e9df1329 100644
--- a/drivers/staging/kpc2000/Makefile
+++ b/drivers/staging/kpc2000/Makefile
@@ -3,3 +3,4 @@
obj-$(CONFIG_KPC2000) += kpc2000/
obj-$(CONFIG_KPC2000_I2C) += kpc_i2c/
obj-$(CONFIG_KPC2000_SPI) += kpc_spi/
+obj-$(CONFIG_KPC2000_DMA) += kpc_dma/
diff --git a/drivers/staging/kpc2000/kpc_dma/Makefile
b/drivers/staging/kpc2000/kpc_dma/Makefile
new file mode 100644
index ..fe5db532c8c8
--- /dev/null
+++ b/drivers/staging/kpc2000/kpc_dma/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-m := kpc_dma.o
+kpc_dma-objs += dma.o
+kpc_dma-objs += fileops.o
+kpc_dma-objs += kpc_dma_driver.o
diff --git a/drivers/staging/kpc2000/kpc_dma/dma.c
b/drivers/staging/kpc2000/kpc_dma/dma.c
new file mode 100644
index ..6959bac11388
--- /dev/null
+++ b/drivers/staging/kpc2000/kpc_dma/dma.c
@@ -0,0 +1,264 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "kpc_dma_driver.h"
+
+/** IRQ Handlers **/
+static
+irqreturn_t ndd_irq_handler(int irq, void *dev_id)
+{
+ struct kpc_dma_device *ldev = (struct kpc_dma_device*)dev_id;
+
+ if ((GetEngineControl(ldev) & ENG_CTL_IRQ_ACTIVE) ||
(ldev->desc_completed->MyDMAAddr != GetEngineCompletePtr(ldev)))
+ schedule_work(>irq_work);
+
+ return IRQ_HANDLED;
+}
+
+static
+void ndd_irq_worker(struct work_struct *ws)
+{
+ struct kpc_dma_descriptor *cur;
+ struct kpc_dma_device *eng = container_of(ws, struct kpc_dma_device,
irq_work);
+ lock_engine(eng);
+
+ if (GetEngineCompletePtr(eng) == 0)
+ goto out;
+
+ if (eng->desc_completed->MyDMAAddr == GetEngineCompletePtr(eng))
+ goto out;
+
+ cur = eng->desc_completed;
+ do {
+ cur = cur->Next;
+ dev_dbg(>pldev->dev, "Handling completed descriptor %p
(acd = %p)\n", cur, cur->acd);
+ BUG_ON(cur == eng->desc_next); // Ordering failure.
+
+ if (cur->DescControlFlags & DMA_DESC_CTL_SOP){
+ eng->accumulated_bytes = 0;
+ eng->accumulated_flags = 0;
+ }
+
+ eng->accumulated_bytes += cur->DescByteCount;
+ if (cur->DescStatusFlags & DMA_DESC_STS_ERROR)
+ eng->accumulated_flags |= ACD_FLAG_ENG_ACCUM_ERROR;
+
+ if (cur->DescStatusFlags & DMA_DESC_STS_SHORT)
+ eng->accumulated_flags |= ACD_FLAG_ENG_ACCUM_SHORT;
+
+ if (cur->DescControlFlags & DMA_DESC_CTL_EOP){
+ if (cur->acd)
+ transfer_complete_cb(cur->acd,
eng->accumulated_bytes, eng->accumulated_flags | ACD_FLAG_DONE);
+ }
+
+ eng->desc_completed = cur;
+
[PATCH] staging: kpc2000: Add DMA driver
>From 6e70cb81c75ebb0ac7897d07c0f12d80505bb22b Mon Sep 17 00:00:00 2001
From: Matt Sickler
Date: Mon, 22 Apr 2019 09:24:03 -0500
Subject: [PATCH] staging: kpc2000: Add DMA driver
Add Daktronics DMA driver. I've added the SPDX license identifiers, Kconfig
entry, and cleaned up as many of the warnings as I could.
I'm not sure what should be done about the AIO "support" code in this driver.
It's currently guarded by an #ifdef CONFIG_KPC_DMA_AIO. Even if that option
was turned on, the code doesn't compile (aio_complete() was removed since
the original version of this driver) and it probably doesn't work right even
if it did compile. Maybe it's best to just remove it completely?
---
drivers/staging/kpc2000/Kconfig | 11 +
drivers/staging/kpc2000/Makefile | 1 +
drivers/staging/kpc2000/kpc_dma/Makefile | 6 +
drivers/staging/kpc2000/kpc_dma/dma.c| 264 ++
drivers/staging/kpc2000/kpc_dma/fileops.c| 420 +++
drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c | 248 +
drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h | 220
drivers/staging/kpc2000/kpc_dma/uapi.h | 11 +
8 files changed, 1181 insertions(+)
create mode 100644 drivers/staging/kpc2000/kpc_dma/Makefile
create mode 100644 drivers/staging/kpc2000/kpc_dma/dma.c
create mode 100644 drivers/staging/kpc2000/kpc_dma/fileops.c
create mode 100644 drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c
create mode 100644 drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h
create mode 100644 drivers/staging/kpc2000/kpc_dma/uapi.h
diff --git a/drivers/staging/kpc2000/Kconfig b/drivers/staging/kpc2000/Kconfig
index 926e770d6e0e..fb5922928f47 100644
--- a/drivers/staging/kpc2000/Kconfig
+++ b/drivers/staging/kpc2000/Kconfig
@@ -44,3 +44,14 @@ config KPC2000_I2C
If unsure, say N.
+config KPC2000_DMA
+ tristate "Daktronics KPC DMA controller"
+ depends on KPC2000
+ help
+ Say Y here if you wish to support the Daktronics DMA controller.
+
+ To compile this driver as a module, choose M here: the module
+ will be called kpc2000_dma
+
+ If unsure, say N.
+
diff --git a/drivers/staging/kpc2000/Makefile b/drivers/staging/kpc2000/Makefile
index 6fcb2ee7b27d..1e48e9df1329 100644
--- a/drivers/staging/kpc2000/Makefile
+++ b/drivers/staging/kpc2000/Makefile
@@ -3,3 +3,4 @@
obj-$(CONFIG_KPC2000) += kpc2000/
obj-$(CONFIG_KPC2000_I2C) += kpc_i2c/
obj-$(CONFIG_KPC2000_SPI) += kpc_spi/
+obj-$(CONFIG_KPC2000_DMA) += kpc_dma/
diff --git a/drivers/staging/kpc2000/kpc_dma/Makefile
b/drivers/staging/kpc2000/kpc_dma/Makefile
new file mode 100644
index ..fe5db532c8c8
--- /dev/null
+++ b/drivers/staging/kpc2000/kpc_dma/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-m := kpc_dma.o
+kpc_dma-objs += dma.o
+kpc_dma-objs += fileops.o
+kpc_dma-objs += kpc_dma_driver.o
diff --git a/drivers/staging/kpc2000/kpc_dma/dma.c
b/drivers/staging/kpc2000/kpc_dma/dma.c
new file mode 100644
index ..6959bac11388
--- /dev/null
+++ b/drivers/staging/kpc2000/kpc_dma/dma.c
@@ -0,0 +1,264 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "kpc_dma_driver.h"
+
+/** IRQ Handlers **/
+static
+irqreturn_t ndd_irq_handler(int irq, void *dev_id)
+{
+ struct kpc_dma_device *ldev = (struct kpc_dma_device*)dev_id;
+
+ if ((GetEngineControl(ldev) & ENG_CTL_IRQ_ACTIVE) ||
(ldev->desc_completed->MyDMAAddr != GetEngineCompletePtr(ldev)))
+ schedule_work(>irq_work);
+
+ return IRQ_HANDLED;
+}
+
+static
+void ndd_irq_worker(struct work_struct *ws)
+{
+ struct kpc_dma_descriptor *cur;
+ struct kpc_dma_device *eng = container_of(ws, struct kpc_dma_device,
irq_work);
+ lock_engine(eng);
+
+ if (GetEngineCompletePtr(eng) == 0)
+ goto out;
+
+ if (eng->desc_completed->MyDMAAddr == GetEngineCompletePtr(eng))
+ goto out;
+
+ cur = eng->desc_completed;
+ do {
+ cur = cur->Next;
+ dev_dbg(>pldev->dev, "Handling completed descriptor %p
(acd = %p)\n", cur, cur->acd);
+ BUG_ON(cur == eng->desc_next); // Ordering failure.
+
+ if (cur->DescControlFlags & DMA_DESC_CTL_SOP){
+ eng->accumulated_bytes = 0;
+ eng->accumulated_flags = 0;
+ }
+
+ eng->accumulated_bytes += cur->DescByteCount;
+ if (cur->DescStatusFlags & DMA_DESC_STS_ERROR)
+ eng->accumulated_flags |= ACD_FLAG_ENG_ACCUM_ERROR;
+
+ if (cur->
