Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <de208903375a13c31717c668f83e71b3f0f9d6f5.camel@egauge.net>
Subject: Re: [PATCH 4/4] wilc1000: Add support for enabling CRC
From:   David Mosberger-Tang <davidm@egauge.net>
To:     Ajay.Kathat@microchip.com, linux-wireless@vger.kernel.org
Cc:     Claudiu.Beznea@microchip.com
Date:   Wed, 24 Feb 2021 09:25:36 -0700
In-Reply-To: <f58ec951-4ab4-2ed4-78d4-96ebb448f494@microchip.com>
References: <20210224055135.1509200-1-davidm@egauge.net>
         <20210224055135.1509200-4-davidm@egauge.net>
         <f58ec951-4ab4-2ed4-78d4-96ebb448f494@microchip.com>
Organization: eGauge Systems LLC
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.4-0ubuntu1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 2021-02-24 at 10:01 +0000, Ajay.Kathat@microchip.com wrote:

> This patch series also looks okay to me. I just have one input which is
> captured below.
> 
> We need to disable both crc7 and crc16 while retrying on failure attempt
> by adding below line
> 
> spi_priv->crc16_enabled = false;

Ah, you're right.  We can always probe with CRC16 off since the chip
returns valid register data regardless of whether crc16_enabled is
true or false.  I'm thinking something like this:

	spi_priv->crc16_enabled = false; // ignore CRC16 during CRC-
detection
	for (crc7 = 1; crc7 >= 0; --crc7) {
		spi_priv->crc7_enabled = crc7;
		ret = spi_internal_read(wilc, WILC_SPI_PROTOCOL_OFFSET,
&reg);
		if (ret == 0)
			break;
	}
	if (ret) {
		dev_err(&spi->dev, "Failed with all possible CRC
settings.\n");
		return ret;
	}

> By default the CRC checks are disabled, so if the kernel module is
> reloaded it should reattempt with both disabled.

Are you sure about that?  My test devices resets into:

 PROTOCOL_REG = 0x2e

which should be CRC7 and CRC16 on, right?

  --david