Re: [PATCHv1 2/3] spi: Add spi driver for Socionext Synquacer platform

[Jassi Brar <jaswinder.singh@xxxxxxxxxx>]

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 linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html