Messages in this thread |  | | Date | Thu, 30 Jan 2020 14:46:09 +0000 | From | Mark Brown <> | Subject | Re: [PATCH] spi: Add FSI-attached SPI controller driver |
| |
On Wed, Jan 29, 2020 at 02:08:24PM -0600, Eddie James wrote:
Overall this looks good, some comments below but they're all fairly minor.
> +++ b/drivers/spi/spi-fsi.c > @@ -0,0 +1,547 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (C) IBM Corporation 2020 > + */
Please make the entire comment a C++ one so things look more intentional.
> + > +static int fsi_spi_data_in(u64 in, u8 *rx, int len) > +{ > + int i; > + int num_bytes = len > 8 ? 8 : len;
Please write normal conditional statements to improve legibility, the ternery operator isn't really needed here.
> +static int fsi_spi_reset(struct fsi_spi *ctx) > +{ > + int rc; > + > + dev_info(ctx->dev, "Resetting SPI controller.\n");
This should be lowered to dev_dbg() at most, it's not really adding anything otherwise.
> +static int fsi_spi_remove(struct device *dev) > +{ > + return 0; > +}
Remove empty functions, if they can safely be empty then it should be possible to omit them.
> +static const struct fsi_device_id fsi_spi_ids[] = { > + { FSI_ENGID_SPI, FSI_VERSION_ANY }, > + { } > +};
This needs a MODULE_DEVICE_TABLE annotation. [unhandled content-type:application/pgp-signature] |  |