Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 28 Mar 2023 11:39:29 +0200
From:   Andi Shyti <andi.shyti@kernel.org>
To:     Piyush Malgujar <pmalgujar@marvell.com>
Cc:     broonie@kernel.org, linux-spi@vger.kernel.org,
        linux-kernel@vger.kernel.org, jannadurai@marvell.com,
        cchavva@marvell.com, Suneel Garapati <sgarapati@marvell.com>
Subject: Re: [PATCH 1/2] spi: octeontx2: Add support for octeontx2 spi
 controller
Message-ID: <20230328093929.qkg2oseuibot3afl@intel.intel>
References: <20230327180753.2279-1-pmalgujar@marvell.com>
 <20230327180753.2279-2-pmalgujar@marvell.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230327180753.2279-2-pmalgujar@marvell.com>
Precedence: bulk

Hi Suneel,

[...]

> +static int tbi_clk_en = 1;

bool?

> +module_param(tbi_clk_en, uint, 0644);
> +MODULE_PARM_DESC(tbi_clk_en,
> +		 "Use Fixed Time Base 100MHz Reference Clock (0=Disable, 1=Enable [default])");

can we avoid using module parameters? You can have these defined
in device tree and you can also make sysfs interfaces, as well.

> +static int cfg_mode_delay = 30;
> +module_param(cfg_mode_delay, uint, 0644);
> +MODULE_PARM_DESC(cfg_mode_delay,
> +		 "Delay in micro-seconds for mode change in MPI CFG register (30 [default])");
> +
> +static void octeontx2_spi_wait_ready(struct octeontx2_spi *p)
> +{
> +	union mpix_sts mpi_sts;
> +	unsigned int loops = 0;
> +
> +	mpi_sts.u64 = 0;
> +	do {
> +		if (loops++)
> +			__delay(500);

mmhhh... why have you chosen __delay() ?

> +		mpi_sts.u64 = readq(p->register_base + OCTEONTX2_SPI_STS(p));
> +	} while (mpi_sts.s.busy);

[...]

> +	if (mpi_cfg.u64 != p->last_cfg) {
> +		p->last_cfg = mpi_cfg.u64;
> +		writeq(mpi_cfg.u64, p->register_base + OCTEONTX2_SPI_CFG(p));
> +		mpi_cfg.u64 = readq(p->register_base + OCTEONTX2_SPI_CFG(p));
> +		udelay(cfg_mode_delay); /* allow CS change to settle */

before "udelaying" anything that the user gives you, I would
check what cfg_mode_delay is.

Andi