On 2/4/20 5:02 AM, Andy Shevchenko wrote:
On Mon, Feb 3, 2020 at 10:33 PM Eddie James <eajames@xxxxxxxxxxxxxxxxxx> wrote:
On 1/30/20 10:37 AM, Andy Shevchenko wrote:
On Wed, Jan 29, 2020 at 10:09 PM Eddie James <eajames@xxxxxxxxxxxxx> wrote:
...
+ struct device *dev;
Isn't fsl->dev the same?
Perhaps kernel doc to explain the difference?
No, it's not the same, as dev here is the SPI controller. I'll add a
comment.
Why to have duplication then?
Nothing is being duplicated, the two variables are storing entirely
different information, both of which are necessary for each SPI
controller that this driver is driving.
+ struct fsi_device *fsi;
...
+ for (i = 0; i < num_bytes; ++i)
+ rx[i] = (u8)((in >> (8 * ((num_bytes - 1) - i))) & 0xffULL);
Redundant & 0xffULL part.
Isn't it NIH of get_unalinged_be64 / le64 or something similar?
No, these are shift in/out operations. The read register will also have
previous operations data in them and must be extracted with only the
correct number of bytes.
Why not to call put_unaligned() how the tail in this case (it's 0 or
can be easily made to be 0) will affect the result?
The shift-in is not the same as any byte-swap or unaligned operation.
For however many bytes we've read, we start at that many bytes
left-shifted in the register and copy out to our buffer, moving right
for each next byte... I don't think there is an existing function for
this operation.
+ return num_bytes;
+}
+static int fsi_spi_data_out(u64 *out, const u8 *tx, int len)
+{
Ditto as for above function. (put_unaligned ...)
Ditto.
I don't understand how this could work for transfers of less than 8
bytes, any put_unaligned would access memory that it doesn't own.
+}
...
+static int fsi_spi_transfer_data(struct fsi_spi *ctx,
+ struct spi_transfer *transfer)
+{
Can you refactor to tx and rx parts?
Why?
It's way too long function to read. Indentation level also can improve
readability.
That's basically what refactoring is for.
The body is 65 lines, I don't think it is too long.
Since the function is used multiple times I think it makes more sense to
encapsulate the check for tx/rx in the function rather than split it and
have to check each time the functions are used.
+ return 0;
+}
...
+ if ((clock_cfg & (SPI_FSI_CLOCK_CFG_MM_ENABLE |
+ SPI_FSI_CLOCK_CFG_ECC_DISABLE |
+ SPI_FSI_CLOCK_CFG_MODE |
+ SPI_FSI_CLOCK_CFG_SCK_RECV_DEL |
+ SPI_FSI_CLOCK_CFG_SCK_DIV)) != wanted_clock_cfg)
+ rc = fsi_spi_write_reg(ctx, SPI_FSI_CLOCK_CFG,
+ wanted_clock_cfg);
Missed {} ?
No? It's one line under the if.
One statement, but *two* lines.
What does checkpatch.pl tell you about this?
Right.
checkpatch.pl says nothing about this, I think it meets the coding
standards as is.
Thanks for the review,
Eddie
