| Message ID | 1444942305-24038-1-git-send-email-fcooper@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 15, 2015 at 03:51:45PM -0500, Franklin S Cooper Jr wrote: > Some devices depend on the master controller driver setup function being > called before calling any chipselect functions. > > Insure that this is done otherwise uninitialized structures may be > accessed causing a kernel panic. I think this is a sensible change but can we please have a better changelog - why is this the best thing to do rather than fixing the driver to not crash? There are a bunch of good reasons to do the master setup before asserting chip select but a driver crashing isn't one of them, it's not clear that this isn't a bug in the driver.
On 10/16/2015 05:28 AM, Mark Brown wrote: > On Thu, Oct 15, 2015 at 03:51:45PM -0500, Franklin S Cooper Jr wrote: >> Some devices depend on the master controller driver setup function being >> called before calling any chipselect functions. >> >> Insure that this is done otherwise uninitialized structures may be >> accessed causing a kernel panic. > I think this is a sensible change but can we please have a better > changelog - why is this the best thing to do rather than fixing the > driver to not crash? There are a bunch of good reasons to do the master > setup before asserting chip select but a driver crashing isn't one of > them, it's not clear that this isn't a bug in the driver. Mark, Makes sense. How about something like this? SPI controllers may need to be properly setup before chip selects can be used. Therefore, wait until the spi controller has a chance to perform their setup procedure before trying to use the chip select. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 16, 2015 at 09:27:50AM -0500, Franklin S Cooper Jr. wrote: > Makes sense. How about something like this? > SPI controllers may need to be properly setup before chip selects > can be used. Therefore, wait until the spi controller has a chance > to perform their setup procedure before trying to use the chip > select. Yes, that's a lot better - there's also the fact that if we assert chip select before we've got the pins in a good state we may cause the device to see signals that confuse it as we change.
On 10/16/2015 09:43 AM, Mark Brown wrote: > On Fri, Oct 16, 2015 at 09:27:50AM -0500, Franklin S Cooper Jr. wrote: > >> Makes sense. How about something like this? >> SPI controllers may need to be properly setup before chip selects >> can be used. Therefore, wait until the spi controller has a chance >> to perform their setup procedure before trying to use the chip >> select. > Yes, that's a lot better - there's also the fact that if we assert chip > select before we've got the pins in a good state we may cause the device > to see signals that confuse it as we change. Ok. I'll submit a rev2 with my updated commit message and incorporate your suggestion also. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 4c638f3..9d5525a 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -2059,11 +2059,11 @@ int spi_setup(struct spi_device *spi) if (!spi->max_speed_hz) spi->max_speed_hz = spi->master->max_speed_hz; - spi_set_cs(spi, false); - if (spi->master->setup) status = spi->master->setup(spi); + spi_set_cs(spi, false); + dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n", (int) (spi->mode & (SPI_CPOL | SPI_CPHA)), (spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",