Received-SPF: pass (google.com: domain of linux-kernel+bounces-105020-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Sat, 16 Mar 2024 03:01:02 +0200
From: Justin Swartz <justin.swartz@risingedge.co.za>
To: Mark Brown <broonie@kernel.org>, Matthias Brugger
 <matthias.bgg@gmail.com>, AngeloGioacchino Del Regno
 <angelogioacchino.delregno@collabora.com>
Cc: linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org
Subject: Re: [PATCH] spi: mt7621: allow GPIO chip select lines
In-Reply-To: <20240316005917.20526-1-justin.swartz@risingedge.co.za>
References: <20240316005917.20526-1-justin.swartz@risingedge.co.za>
Message-ID: <182aa3a1fcd05a2e25b55442f58ced5b@risingedge.co.za>
User-Agent: Roundcube Webmail/1.3.17

Please ignore this patch. It was accidentally sent without
"v2" nor --in-reply-to=...


On 2024-03-16 02:59, Justin Swartz wrote:
> Extract a magic number, from mt7621_spi_probe(), used to
> declare the number of chip select lines (which co-incides
> with the native chip select count of 2) to a macro.
> 
> Use the newly defined MT7621_NATIVE_CS_COUNT macro to
> instead populate both the spi_controller's max_native_cs
> and num_chipselect members.
> 
> Declare that the spi_controller should use_gpio_descriptors
> if present in the device properties (such as those declared
> in the cs-gpio property of a "ralink,mt7621-spi" compatible
> device-tree node) so that the SPI core will recalculcate
> num_chipselect to account for the GPIO descriptors that
> it should have populated in the cs_gpiod array member.
> 
> Remove the assignment of mt7621_spi_transfer_one_message()
> to the spi_controller's transfer_one_message hook.
> 
> Refactor the mt7621_spi_transfer_one_message() logic into
> mt7621_spi_prepare_message() and mt7621_spi_transfer_one()
> and assign both to the spi_controller's prepare_message
> and transfer_one hooks respectively.
> 
> Migrate the call mt7621_spi_transfer_one_message() made to
> mt7621_spi_flush() just before chip select deactivation,
> to the end of mt7621_spi_write_half_duplex() to ensure
> that any pending data is shifted out of MOSI before the SPI
> core deactivates the chip select line.
> 
> As chip select activation is now taken care of by the SPI
> core, due to the use of the transfer_one hook instead of
> transfer_one_message, the calls to mt7621_spi_set_cs()
> from mt7621_spi_transfer_one_message() have fallen away.
> 
> And although the SPI core will handle activation for GPIO
> chip select lines behind the scenes, it requires a callback
> to allow the driver to perform controller-specific
> operations to control its native chip select lines.
> 
> Rename mt7621_spi_set_cs() to mt7621_spi_set_native_cs()
> and make sure that it takes into account the activation
> polarity of the chip select line it's acting upon, as the
> passed enable parameter represents the desired line level
> and not the desired activation state, and then assign
> mt7621_set_cs() to the spi_controller's set_cs hook.
> 
> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
> ---
> 
> Changes from v1 to v2:
> 
>   Mark Brown recommended using the transfer_one hook
>   approach, so I did.
> 
> 
>  drivers/spi/spi-mt7621.c | 95 +++++++++++++++++++---------------------
>  1 file changed, 45 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/spi/spi-mt7621.c b/drivers/spi/spi-mt7621.c
> index 4e9053d03..3770b8e09 100644
> --- a/drivers/spi/spi-mt7621.c
> +++ b/drivers/spi/spi-mt7621.c
> @@ -52,6 +52,8 @@
>  #define MT7621_CPOL		BIT(4)
>  #define MT7621_LSB_FIRST	BIT(3)
> 
> +#define MT7621_NATIVE_CS_COUNT	2
> +
>  struct mt7621_spi {
>  	struct spi_controller	*host;
>  	void __iomem		*base;
> @@ -75,10 +77,11 @@ static inline void mt7621_spi_write(struct
> mt7621_spi *rs, u32 reg, u32 val)
>  	iowrite32(val, rs->base + reg);
>  }
> 
> -static void mt7621_spi_set_cs(struct spi_device *spi, int enable)
> +static void mt7621_spi_set_native_cs(struct spi_device *spi, bool 
> enable)
>  {
>  	struct mt7621_spi *rs = spidev_to_mt7621_spi(spi);
>  	int cs = spi_get_chipselect(spi, 0);
> +	bool active = spi->mode & SPI_CS_HIGH ? enable : !enable;
>  	u32 polar = 0;
>  	u32 host;
> 
> @@ -94,7 +97,7 @@ static void mt7621_spi_set_cs(struct spi_device
> *spi, int enable)
> 
>  	rs->pending_write = 0;
> 
> -	if (enable)
> +	if (active)
>  		polar = BIT(cs);
>  	mt7621_spi_write(rs, MT7621_SPI_POLAR, polar);
>  }
> @@ -154,6 +157,23 @@ static inline int
> mt7621_spi_wait_till_ready(struct mt7621_spi *rs)
>  	return -ETIMEDOUT;
>  }
> 
> +static int mt7621_spi_prepare_message(struct spi_controller *host,
> +				      struct spi_message *m)
> +{
> +	struct mt7621_spi *rs = spi_controller_get_devdata(host);
> +	struct spi_device *spi = m->spi;
> +	unsigned int speed = spi->max_speed_hz;
> +	struct spi_transfer *t = NULL;
> +
> +	mt7621_spi_wait_till_ready(rs);
> +
> +	list_for_each_entry(t, &m->transfers, transfer_list)
> +		if (t->speed_hz < speed)
> +			speed = t->speed_hz;
> +
> +	return mt7621_spi_prepare(spi, speed);
> +}
> +
>  static void mt7621_spi_read_half_duplex(struct mt7621_spi *rs,
>  					int rx_len, u8 *buf)
>  {
> @@ -243,59 +263,30 @@ static void mt7621_spi_write_half_duplex(struct
> mt7621_spi *rs,
>  	}
> 
>  	rs->pending_write = len;
> +	mt7621_spi_flush(rs);
>  }
> 
> -static int mt7621_spi_transfer_one_message(struct spi_controller 
> *host,
> -					   struct spi_message *m)
> +static int mt7621_spi_transfer_one(struct spi_controller *host,
> +				   struct spi_device *spi,
> +				   struct spi_transfer *t)
>  {
>  	struct mt7621_spi *rs = spi_controller_get_devdata(host);
> -	struct spi_device *spi = m->spi;
> -	unsigned int speed = spi->max_speed_hz;
> -	struct spi_transfer *t = NULL;
> -	int status = 0;
> -
> -	mt7621_spi_wait_till_ready(rs);
> 
> -	list_for_each_entry(t, &m->transfers, transfer_list)
> -		if (t->speed_hz < speed)
> -			speed = t->speed_hz;
> -
> -	if (mt7621_spi_prepare(spi, speed)) {
> -		status = -EIO;
> -		goto msg_done;
> -	}
> -
> -	/* Assert CS */
> -	mt7621_spi_set_cs(spi, 1);
> -
> -	m->actual_length = 0;
> -	list_for_each_entry(t, &m->transfers, transfer_list) {
> -		if ((t->rx_buf) && (t->tx_buf)) {
> -			/*
> -			 * This controller will shift some extra data out
> -			 * of spi_opcode if (mosi_bit_cnt > 0) &&
> -			 * (cmd_bit_cnt == 0). So the claimed full-duplex
> -			 * support is broken since we have no way to read
> -			 * the MISO value during that bit.
> -			 */
> -			status = -EIO;
> -			goto msg_done;
> -		} else if (t->rx_buf) {
> -			mt7621_spi_read_half_duplex(rs, t->len, t->rx_buf);
> -		} else if (t->tx_buf) {
> -			mt7621_spi_write_half_duplex(rs, t->len, t->tx_buf);
> -		}
> -		m->actual_length += t->len;
> +	if ((t->rx_buf) && (t->tx_buf)) {
> +		/*
> +		 * This controller will shift some extra data out
> +		 * of spi_opcode if (mosi_bit_cnt > 0) &&
> +		 * (cmd_bit_cnt == 0). So the claimed full-duplex
> +		 * support is broken since we have no way to read
> +		 * the MISO value during that bit.
> +		 */
> +		return -EIO;
> +	} else if (t->rx_buf) {
> +		mt7621_spi_read_half_duplex(rs, t->len, t->rx_buf);
> +	} else if (t->tx_buf) {
> +		mt7621_spi_write_half_duplex(rs, t->len, t->tx_buf);
>  	}
> 
> -	/* Flush data and deassert CS */
> -	mt7621_spi_flush(rs);
> -	mt7621_spi_set_cs(spi, 0);
> -
> -msg_done:
> -	m->status = status;
> -	spi_finalize_current_message(host);
> -
>  	return 0;
>  }
> 
> @@ -353,10 +344,14 @@ static int mt7621_spi_probe(struct 
> platform_device *pdev)
>  	host->mode_bits = SPI_LSB_FIRST;
>  	host->flags = SPI_CONTROLLER_HALF_DUPLEX;
>  	host->setup = mt7621_spi_setup;
> -	host->transfer_one_message = mt7621_spi_transfer_one_message;
> +	host->prepare_message = mt7621_spi_prepare_message;
> +	host->set_cs = mt7621_spi_set_native_cs;
> +	host->transfer_one = mt7621_spi_transfer_one;
>  	host->bits_per_word_mask = SPI_BPW_MASK(8);
>  	host->dev.of_node = pdev->dev.of_node;
> -	host->num_chipselect = 2;
> +	host->max_native_cs = MT7621_NATIVE_CS_COUNT;
> +	host->num_chipselect = MT7621_NATIVE_CS_COUNT;
> +	host->use_gpio_descriptors = true;
> 
>  	dev_set_drvdata(&pdev->dev, host);