Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <391203d8794c2fc5343f2bf991a5497d3d5b7fcd.camel@perches.com>
Subject: Re: [PATCH v1 1/7] Add Driver for SUNIX PCI(e) I/O expansion board
From:   Joe Perches <joe@perches.com>
To:     Moriis Ku <saumah@gmail.com>, lee.jones@linaro.org
Cc:     linux-kernel@vger.kernel.org
Date:   Wed, 31 Mar 2021 15:49:19 -0700
In-Reply-To: <20210330082350.6890-1-saumah@gmail.com>
References: <20210330082350.6890-1-saumah@gmail.com>
Content-Type: text/plain; charset="ISO-8859-1"
User-Agent: Evolution 3.38.1-1 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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...