Re: [PATCH] spi: Add FSI-attached SPI controller driver

[Eddie James]

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


>