| Message ID | 1364905195-24286-8-git-send-email-tpiepho@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Dear Trent Piepho, > Despite many warnings in the SPI documentation and code, the spi-mxs > driver sets shared chip registers in the ->setup method. This method can > be called when transfers are in progress on other slaves controlled by the > master. Setting registers or any other shared state will corrupt those > transfers. > > So fix mxs_spi_setup() to not call mxs_spi_setup_transfer(). > > Signed-off-by: Trent Piepho <tpiepho@gmail.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Fabio Estevam <fabio.estevam@freescale.com> > Cc: Shawn Guo <shawn.guo@linaro.org> > --- > drivers/spi/spi-mxs.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c > index 103c478..2a2147a 100644 > --- a/drivers/spi/spi-mxs.c > +++ b/drivers/spi/spi-mxs.c > @@ -115,21 +115,16 @@ static int mxs_spi_setup_transfer(struct spi_device > *dev, > > static int mxs_spi_setup(struct spi_device *dev) > { > - int err = 0; > - > if (!dev->bits_per_word) > dev->bits_per_word = 8; > > if (dev->mode & ~(SPI_CPOL | SPI_CPHA)) > return -EINVAL; > > - err = mxs_spi_setup_transfer(dev, NULL); > - if (err) { > - dev_err(&dev->dev, > - "Failed to setup transfer, error = %d\n", err); > - } > + if (dev->bits_per_word != 8) > + return -EINVAL; What's this new addition doing here? btw. I was under the impression the MXS SSP block can handle other word-widths than 8 bit, no ? Best regards, Marek Vasut ------------------------------------------------------------------------------ Minimize network downtime and maximize team effectiveness. Reduce network management and security costs.Learn how to hire the most talented Cisco Certified professionals. Visit the Employer Resources Portal http://www.cisco.com/web/learning/employer_resources/index.html
On Tue, Apr 2, 2013 at 4:31 PM, Marek Vasut <marex@denx.de> wrote: >> static int mxs_spi_setup(struct spi_device *dev) >> { >> - int err = 0; >> - >> if (!dev->bits_per_word) >> dev->bits_per_word = 8; >> >> if (dev->mode & ~(SPI_CPOL | SPI_CPHA)) >> return -EINVAL; >> >> - err = mxs_spi_setup_transfer(dev, NULL); >> - if (err) { >> - dev_err(&dev->dev, >> - "Failed to setup transfer, error = %d\n", err); >> - } >> + if (dev->bits_per_word != 8) >> + return -EINVAL; > > What's this new addition doing here? It's not new. The only useful thing mxs_spi_setup_transfer() (which is no longer called) did in this instance was make that check. > btw. I was under the impression the MXS SSP block can handle other word-widths > than 8 bit, no ? In theory it can, however the driver only supports 8-bits and will reject anything else. ------------------------------------------------------------------------------ Minimize network downtime and maximize team effectiveness. Reduce network management and security costs.Learn how to hire the most talented Cisco Certified professionals. Visit the Employer Resources Portal http://www.cisco.com/web/learning/employer_resources/index.html
Dear Trent Piepho, > On Tue, Apr 2, 2013 at 4:31 PM, Marek Vasut <marex@denx.de> wrote: > >> static int mxs_spi_setup(struct spi_device *dev) > >> { > >> > >> - int err = 0; > >> - > >> > >> if (!dev->bits_per_word) > >> > >> dev->bits_per_word = 8; > >> > >> if (dev->mode & ~(SPI_CPOL | SPI_CPHA)) > >> > >> return -EINVAL; > >> > >> - err = mxs_spi_setup_transfer(dev, NULL); > >> - if (err) { > >> - dev_err(&dev->dev, > >> - "Failed to setup transfer, error = %d\n", err); > >> - } > >> + if (dev->bits_per_word != 8) > >> + return -EINVAL; > > > > What's this new addition doing here? > > It's not new. It is new in the context of this patch and it's not described in the commit message. > The only useful thing mxs_spi_setup_transfer() (which > is no longer called) did in this instance was make that check. > > > btw. I was under the impression the MXS SSP block can handle other > > word-widths than 8 bit, no ? > > In theory it can, In practice too ;-) > however the driver only supports 8-bits and will > reject anything else. Then please at least add a comment about this. Best regards, Marek Vasut ------------------------------------------------------------------------------ Minimize network downtime and maximize team effectiveness. Reduce network management and security costs.Learn how to hire the most talented Cisco Certified professionals. Visit the Employer Resources Portal http://www.cisco.com/web/learning/employer_resources/index.html
On Apr 2, 2013 7:27 PM, "Marek Vasut" <marex@denx.de> wrote: > > > The only useful thing mxs_spi_setup_transfer() (which > > is no longer called) did in this instance was make that check. > > > > > btw. I was under the impression the MXS SSP block can handle other > > > word-widths than 8 bit, no ? > > > > In theory it can, > > In practice too ;-) > > > however the driver only supports 8-bits and will > > reject anything else. > > Then please at least add a comment about this. What does that have to do with this patch? There was no comment about it before. You're insisting that changes be broken up to the extent that changing one line of code takes multiple patches! Now you want a comment about existing driver behavior added to patch that doesn't change that behavior? Documenting the driver behavior would seem to be a job for the driver maintainer. If doing this is something I need to do, then maybe I should maintain it? ------------------------------------------------------------------------------ Minimize network downtime and maximize team effectiveness. Reduce network management and security costs.Learn how to hire the most talented Cisco Certified professionals. Visit the Employer Resources Portal http://www.cisco.com/web/learning/employer_resources/index.html
Dear Trent Piepho, > On Apr 2, 2013 7:27 PM, "Marek Vasut" <marex@denx.de> wrote: > > > The only useful thing mxs_spi_setup_transfer() (which > > > is no longer called) did in this instance was make that check. > > > > > > > btw. I was under the impression the MXS SSP block can handle other > > > > word-widths than 8 bit, no ? > > > > > > In theory it can, > > > > In practice too ;-) > > > > > however the driver only supports 8-bits and will > > > reject anything else. > > > > Then please at least add a comment about this. > > What does that have to do with this patch? You're adding a piece of code. This change is a) not documented in the commit message b) unrelated to this fix, seems to be more fitting in 10/12 or something -- maybe even shuffling the patches a bit might help So add a comment to make this clearer for people who hack on this after you. > There was no comment about > it before. You're insisting that changes be broken up to the extent > that changing one line of code takes multiple patches! Now you want a > comment about existing driver behavior added to patch that doesn't > change that behavior? > > Documenting the driver behavior would seem to be a job for the driver > maintainer. If doing this is something I need to do, then maybe I > should maintain it? You ain't rubbing people the right way at all, I tell you. This pushy/offensive behavior helps noone, calm down please. It is usually a good idea to sleep on your reply or take your time. Ad maintainership -- there's no maintainer of the driver, there's only Fabio, Shawn, me and the SPI subsystem guys. And it works well. It's everybodys' code and everyone's welcome to contribute. What's the problem? Best regards, Marek Vasut ------------------------------------------------------------------------------ Minimize network downtime and maximize team effectiveness. Reduce network management and security costs.Learn how to hire the most talented Cisco Certified professionals. Visit the Employer Resources Portal http://www.cisco.com/web/learning/employer_resources/index.html
On Tue, Apr 2, 2013 at 11:50 PM, Marek Vasut <marex@denx.de> wrote: > Dear Trent Piepho, > >> On Apr 2, 2013 7:27 PM, "Marek Vasut" <marex@denx.de> wrote: >> > > The only useful thing mxs_spi_setup_transfer() (which >> > > is no longer called) did in this instance was make that check. >> > > >> > > > btw. I was under the impression the MXS SSP block can handle other >> > > > word-widths than 8 bit, no ? >> > > >> > > In theory it can, >> > >> > In practice too ;-) >> > >> > > however the driver only supports 8-bits and will >> > > reject anything else. >> > >> > Then please at least add a comment about this. >> >> What does that have to do with this patch? > > You're adding a piece of code. This change is > a) not documented in the commit message > b) unrelated to this fix, seems to be more fitting in 10/12 or something -- > maybe even shuffling the patches a bit might help It's not changing the behavior and it's not new code. It's existing code that was in a different function, but that function does other things which can't be done at this point. That's in the patch description. It is obviously part of the this patch since not calling mxs_spi_setup_transfer() would include continuing to have the same behavior w.r.t. checking the spi device settings that was already in existence. > So add a comment to make this clearer for people who hack on this after you. > >> There was no comment about >> it before. You're insisting that changes be broken up to the extent >> that changing one line of code takes multiple patches! Now you want a >> comment about existing driver behavior added to patch that doesn't >> change that behavior? >> >> Documenting the driver behavior would seem to be a job for the driver >> maintainer. If doing this is something I need to do, then maybe I >> should maintain it? > > You ain't rubbing people the right way at all, I tell you. This pushy/offensive > behavior helps noone, calm down please. It is usually a good idea to sleep on > your reply or take your time. Well neither are you! I put up with your nitpicking far more than I should have. Move a comment from one line to another? Really? You want me to waste the time to make a new patch series just because you think a four word comment would look nicer one line up? That's ridiculous! I think you've gone beyond the point of finding flaws or mistakes (have you found any actual bugs in any of my code?) and are just being obstructionist. > Ad maintainership -- there's no maintainer of the driver, there's only Fabio, > Shawn, me and the SPI subsystem guys. And it works well. It's everybodys' code > and everyone's welcome to contribute. What's the problem? I've spent far more time dealing with demands to split tiny patches into even tinier patches and tweak the exact character placement to your personal desires than I did to find and fix the bugs in the first place! I think you should accept that if you didn't fix the driver before now, then someone else is going to fix it or replace it. So you had your opportunity to get every last character exactly the way you wanted it and that passed. I fixed it. So I get to use my judgement and write what I think is best. If you can find any actual bugs, or even CodingStyle errors, or merely that a majority of existing kernel code is stylistically different, or different way of doing something that has an object improvement, then I'm all ears. But there is no need for you to complain about completely irrelevant details. I've had people submit patches to my code that didn't do things the way I would have done it. But you know what, if I wanted it done my way I should have done it myself when I had the chance. Since I seem to know how this driver works better than anyone else who has come forward, maybe I should maintain it? That way I can spend my time fixing it instead or dealing making silly changes to patches. ------------------------------------------------------------------------------ Minimize network downtime and maximize team effectiveness. Reduce network management and security costs.Learn how to hire the most talented Cisco Certified professionals. Visit the Employer Resources Portal http://www.cisco.com/web/learning/employer_resources/index.html
diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c index 103c478..2a2147a 100644 --- a/drivers/spi/spi-mxs.c +++ b/drivers/spi/spi-mxs.c @@ -115,21 +115,16 @@ static int mxs_spi_setup_transfer(struct spi_device *dev, static int mxs_spi_setup(struct spi_device *dev) { - int err = 0; - if (!dev->bits_per_word) dev->bits_per_word = 8; if (dev->mode & ~(SPI_CPOL | SPI_CPHA)) return -EINVAL; - err = mxs_spi_setup_transfer(dev, NULL); - if (err) { - dev_err(&dev->dev, - "Failed to setup transfer, error = %d\n", err); - } + if (dev->bits_per_word != 8) + return -EINVAL; - return err; + return 0; } static uint32_t mxs_spi_cs_to_reg(unsigned cs)
Despite many warnings in the SPI documentation and code, the spi-mxs driver sets shared chip registers in the ->setup method. This method can be called when transfers are in progress on other slaves controlled by the master. Setting registers or any other shared state will corrupt those transfers. So fix mxs_spi_setup() to not call mxs_spi_setup_transfer(). Signed-off-by: Trent Piepho <tpiepho@gmail.com> Cc: Marek Vasut <marex@denx.de> Cc: Fabio Estevam <fabio.estevam@freescale.com> Cc: Shawn Guo <shawn.guo@linaro.org> --- drivers/spi/spi-mxs.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)