On 9 January 2018 at 23:11, Mark Brown <broonie@xxxxxxxxxx> wrote: > On Tue, Jan 09, 2018 at 06:40:48PM +0530, jassisinghbrar@xxxxxxxxx wrote: > > This is basically fine, a couple of style things plus the question I had > on the bindings: > >> +++ b/drivers/spi/spi-synquacer.c >> @@ -0,0 +1,664 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Synquacer HSSPI controller driver > > Mixing C and C++ style comments on adjacent lines looks messy, just make > the whole thing C++. > Fixed. >> + speed = xfer->speed_hz ? : spi->max_speed_hz; >> + bpw = xfer->bits_per_word ? : spi->bits_per_word; > > I'm not a big fan of all the ternery operator use in the driver, it's > not super helpful for legibility. > This actually goes away now. >> + if (sspi->bpw == 8) >> + words = xfer->len / 1; >> + else if (sspi->bpw == 16) >> + words = xfer->len / 2; >> + else >> + words = xfer->len / 4; > > This should be a switch statement; a safety check for invalid values > wouldn't go amiss either. > ok Thank you. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
