RE: [PATCH 1/2 v2] spi: overhaul davinci spi driver to correct multiple errors
Hi Brian, On Mon, Mar 15, 2010 at 21:20:13, Kevin Hilman wrote: > Brian Niebuhr writes: > > > > > This patch is a significant overhaul of the davinci spi > >> controller driver > >> > that corrects multiple errors: > >> > > >> > - Eliminate a race condition that exists for slow SPI devices > >> > - Fix DMA transfer length error > >> > - Fix limitations preventing multiple SPI devices on the > >> same controller > >> > > >> > Signed-off-by: Brian Niebuhr > >> > >> The verbose description of the issues addressed from PATCH 0/2 should > >> go here is well so it makes it into the permanent git history. > > > > I can certainly do that. > > > >> That being said, I think for the sake of reviewing, you're going to > >> need to break this up into reviewable pieces, each having a verbose > >> description of the issues being solved. > >> > >> There is also a mixture of fixes, enhancements, renames etc. These > >> should be done as separate patches. > >> > >> I know that it's more work to break it up like this, but that's the > >> only way to make a large change like this reviewable by others. > > > > I guess I was hoping that this could be reviewed as if it were a new > > driver submission. I ended up more or less rewriting all of the > > functional parts of the driver (txrx_bufs(), chipselect(), IRQs and DMA > > callbacks), so it's very difficult to show this as a series of changes. > > I do understand the problem from your perspective, though. My thought > > was that if the TI folks were willing to look the driver over and they > > gave their blessing, that you would look at it as if it were a > > replacement driver and accept or deny it on that basis. > > I'm OK with the approach of considering it as a brand new driver. The > changelog made me think it was a bunch of fixes/enhancements and not a > re-write. > > It should then be made more clear in the changelog that this is > essentially a re-write, and why it is not done in a series of small > changes. > > Whichever approach, this will need to worked out between you and the > origial TI authors (Sandeep, Sudahkar) who will need to review/signoff > this replacement. > I tried testing this patch on some DA8xx platforms I have but could not do it as the patch did not apply cleanly to Kevin's tree. You might have to rebase this patch so that it applies on Kevin's tree. Also, I would still prefer if you can breakdown this patch as per the various fixes you have done because the current SPI driver is already accepted in mainline and is working, of course with some limitations. But if you want to treat it as complete overhaul, then it is better to split the patch such that, one patch removes the existing driver and other patch adds the new driver. In this way it is easy to review the patch. Regards, Sudhakar ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH 1/2 v2] spi: overhaul davinci spi driver to correct multiple errors
Brian Niebuhr writes: > This patch is a significant overhaul of the davinci spi controller driver > that corrects multiple errors: > > - Eliminate a race condition that exists for slow SPI devices > - Fix DMA transfer length error > - Fix limitations preventing multiple SPI devices on the same controller > > Signed-off-by: Brian Niebuhr The verbose description of the issues addressed from PATCH 0/2 should go here is well so it makes it into the permanent git history. That being said, I think for the sake of reviewing, you're going to need to break this up into reviewable pieces, each having a verbose description of the issues being solved. There is also a mixture of fixes, enhancements, renames etc. These should be done as separate patches. I know that it's more work to break it up like this, but that's the only way to make a large change like this reviewable by others. Thanks, Kevin ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH 1/2 v2] spi: overhaul davinci spi driver to correct multiple errors
> > This patch is a significant overhaul of the davinci spi > controller driver > > that corrects multiple errors: > > > > - Eliminate a race condition that exists for slow SPI devices > > - Fix DMA transfer length error > > - Fix limitations preventing multiple SPI devices on the > same controller > > > > Signed-off-by: Brian Niebuhr > > The verbose description of the issues addressed from PATCH 0/2 should > go here is well so it makes it into the permanent git history. I can certainly do that. > That being said, I think for the sake of reviewing, you're going to > need to break this up into reviewable pieces, each having a verbose > description of the issues being solved. > > There is also a mixture of fixes, enhancements, renames etc. These > should be done as separate patches. > > I know that it's more work to break it up like this, but that's the > only way to make a large change like this reviewable by others. I guess I was hoping that this could be reviewed as if it were a new driver submission. I ended up more or less rewriting all of the functional parts of the driver (txrx_bufs(), chipselect(), IRQs and DMA callbacks), so it's very difficult to show this as a series of changes. I do understand the problem from your perspective, though. My thought was that if the TI folks were willing to look the driver over and they gave their blessing, that you would look at it as if it were a replacement driver and accept or deny it on that basis. Thanks for your consideration, Brian ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH 1/2 v2] spi: overhaul davinci spi driver to correct multiple errors
Brian Niebuhr writes: > > > This patch is a significant overhaul of the davinci spi >> controller driver >> > that corrects multiple errors: >> > >> > - Eliminate a race condition that exists for slow SPI devices >> > - Fix DMA transfer length error >> > - Fix limitations preventing multiple SPI devices on the >> same controller >> > >> > Signed-off-by: Brian Niebuhr >> >> The verbose description of the issues addressed from PATCH 0/2 should >> go here is well so it makes it into the permanent git history. > > I can certainly do that. > >> That being said, I think for the sake of reviewing, you're going to >> need to break this up into reviewable pieces, each having a verbose >> description of the issues being solved. >> >> There is also a mixture of fixes, enhancements, renames etc. These >> should be done as separate patches. >> >> I know that it's more work to break it up like this, but that's the >> only way to make a large change like this reviewable by others. > > I guess I was hoping that this could be reviewed as if it were a new > driver submission. I ended up more or less rewriting all of the > functional parts of the driver (txrx_bufs(), chipselect(), IRQs and DMA > callbacks), so it's very difficult to show this as a series of changes. > I do understand the problem from your perspective, though. My thought > was that if the TI folks were willing to look the driver over and they > gave their blessing, that you would look at it as if it were a > replacement driver and accept or deny it on that basis. I'm OK with the approach of considering it as a brand new driver. The changelog made me think it was a bunch of fixes/enhancements and not a re-write. It should then be made more clear in the changelog that this is essentially a re-write, and why it is not done in a series of small changes. Whichever approach, this will need to worked out between you and the origial TI authors (Sandeep, Sudahkar) who will need to review/signoff this replacement. Kevin ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH 1/2 v2] spi: overhaul davinci spi driver to correct multiple errors
I've applied the patch to the latest git kernel (I believe it was against arago), and tested it on the DM355EVM, in the meantime in polling mode only. I intend to make more tests in the near future. Several issues I've encountered were indeed solved (such as the extra writes to the bus after the chip-select turns idle). ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
