On Tue, Jul 15, 2014 at 12:38:58PM +0200, Javier Martinez Canillas wrote: > Hello Mark, Don't top post. > On Mon, Jul 14, 2014 at 7:25 PM, Mark Brown <broonie@xxxxxxxxxx> wrote: > > On Mon, Jul 14, 2014 at 11:11:44AM +0530, Naveen Krishna Chatradhi wrote: > So, the .line field is used to specify a GPIO, not a chip select. So > gpio_is_valid() should also be used to check that cs->line is a valid > GPIO. If board file does not want to use a GPIO and wants to use a > built-in chip select then it should either not use a struct > s3c64xx_spi_csinfo at all or set .line to -ENOENT in order to be > consistent with the SPI core. This sort of "let's make everyone instabuggy" change is not good practice, especially when it's mixed in with some other not obviously related change, has an unclear benefit and is barely mentioned in the commit log - I found this through code review, not through it being a clearly intentional and considered part of the change. > As I said before this patch is fixing a bug in the SPI driver, the > first version of the series was posted on June, 10 so many other > patches already depend on this fix. So it would be great if we can > move this forward since this is hurting the platform support as a > whole. So provide a clear, easy to understand patch series then. As I've said before this series is setting off lots of alarm bells - the large number of versions, the difficulty in understanding what it was supposed to do both for the overall goal of the series and the individual changes, and the fact that several people have found bugs for use cases other than your own (all the way up to removing non-DT support) are all saying that this is something that needs careful review and also making that review difficult. Code review is an important part of the process, we need people to work to make that review easy, address review comments and allow a reasonable time for the review to happen (something that's obviously going to be quicker with submissions that are easier to review).
Attachment:
signature.asc
Description: Digital signature
