Messages in this thread | | | Subject | Re: [PATCH v1 1/7] Add Driver for SUNIX PCI(e) I/O expansion board | From | Joe Perches <> | Date | Wed, 31 Mar 2021 15:49:19 -0700 |
| |
On Tue, 2021-03-30 at 16:23 +0800, Moriis Ku wrote: > From: Morris <saumah@gmail.com> > > Signed-off-by: Morris <saumah@gmail.com> > --- > spi_pack.c | 1506 ++++++++++++++++++++++++++++++++++++++++++++++++++++
You might try to use scripts/checkpatch.pl on this and see if there is anything you want to change to have the code look more like the rest of the kernel.
Using ./scripts/checkpatch.pl --strict --fix <patch> from the top of the kernel tree should help quite a bit with making the code layout look more like typical kernel style.
> diff --git a/spi_pack.c b/spi_pack.c [] > +static void get_info(struct sunix_sdc_spi_channel *spi_chl, unsigned int incomeLength, unsigned int * translateLength) > +{ [] > + do > + { > + Address = spi_chl->info.phy2_base_start + spi_chl->info.memoffset; > + > + } while (false);
That's an odd and unnecessary use of a do while.
> + memset(TrBuff, 0, SUNIX_SDC_SPI_BUFF); > + TrLength = 0; > + > + pTrHeader->Version = 0x01; > + pTrHeader->CmdResponseEventData = pRxHeader->CmdResponseEventData | 0x8000; > + pTrHeader->ResponseStatus = nStatus; > + pTrHeader->Length = (nStatus == SDCSPI_STATUS_SUCCESS)?(31 + (cib_info->spi_number_of_device * 12)):0; > + TrLength = sizeof(SPI_HEADER);
Perhaps reorder the patch submission to define the structs first.
This can't compile as SPI_HEADER isn't defined SPI_HEADER and PSPI_HEADER should use not use typedefs and use the typical struct spi_header and struct spi_header *
> + if (pTrHeader->ResponseStatus == SDCSPI_STATUS_SUCCESS)
Hungarian isn't generally used in the kernel. CamelCase isn't generally used either.
> + { > + memcpy(&TrBuff[TrLength], spi_chl->info.model_name, 16); > + TrLength += 16; > + TrBuff[TrLength++] = spi_chl->info.bus_number; > + TrBuff[TrLength++] = spi_chl->info.dev_number; > + TrBuff[TrLength++] = spi_chl->info.line; > + TrBuff[TrLength++] = (unsigned char)((Address & 0xff000000) >> 24); > + TrBuff[TrLength++] = (unsigned char)((Address & 0x00ff0000) >> 16); > + TrBuff[TrLength++] = (unsigned char)((Address & 0x0000ff00) >> 8); > + TrBuff[TrLength++] = (unsigned char)((Address & 0x000000ff)); > + TrBuff[TrLength++] = (unsigned char)(spi_chl->info.irq);
You might consider using a single char * and increment that and not use TrLength at all and use p - TrBuff when necessary.
*p++ = spi_chl->info.bus_number; *p++ = spi_chl->info.dev_number; ...
> + TrBuff[TrLength++] = (unsigned char)((cib_info->spi_significand_of_clock & 0xff000000) >> 24); > + TrBuff[TrLength++] = (unsigned char)((cib_info->spi_significand_of_clock & 0x00ff0000) >> 16); > + TrBuff[TrLength++] = (unsigned char)((cib_info->spi_significand_of_clock & 0x0000ff00) >> 8); > + TrBuff[TrLength++] = (unsigned char)((cib_info->spi_significand_of_clock & 0x000000ff));
Perhaps
put_unaligned_le32(cib_info->spi_significant_of_clock, p); p += 4;
etc...
| |