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++. > + 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. > + 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.
Attachment:
signature.asc
Description: PGP signature
