lkml.org 
[lkml]   [2021]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v1 1/7] Add Driver for SUNIX PCI(e) I/O expansion board
From
Date
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...


\
 
 \ /
  Last update: 2021-04-01 00:50    [W:0.053 / U:0.404 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site