From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 161BAC433FE for ; Wed, 15 Dec 2021 17:44:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232645AbhLORod convert rfc822-to-8bit (ORCPT ); Wed, 15 Dec 2021 12:44:33 -0500 Received: from relay5-d.mail.gandi.net ([217.70.183.197]:60289 "EHLO relay5-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235513AbhLORod (ORCPT ); Wed, 15 Dec 2021 12:44:33 -0500 Received: (Authenticated sender: miquel.raynal@bootlin.com) by relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 113D61C0003; Wed, 15 Dec 2021 17:44:27 +0000 (UTC) Date: Wed, 15 Dec 2021 18:44:26 +0100 From: Miquel Raynal To: Boris Brezillon Cc: Richard Weinberger , Vignesh Raghavendra , Tudor Ambarus , Pratyush Yadav , Michael Walle , , Mark Brown , , Julien Su , Jaime Liao , Thomas Petazzoni , Xiangsheng Hou Subject: Re: [PATCH v5 12/13] spi: mxic: Use spi_mem_generic_supports_op() Message-ID: <20211215184426.67fd3912@xps13> In-Reply-To: <20211214172410.2b26c17e@collabora.com> References: <20211214114140.54629-1-miquel.raynal@bootlin.com> <20211214114140.54629-13-miquel.raynal@bootlin.com> <20211214172410.2b26c17e@collabora.com> Organization: Bootlin X-Mailer: Claws Mail 3.17.7 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Precedence: bulk List-ID: X-Mailing-List: linux-spi@vger.kernel.org Hi Boris, boris.brezillon@collabora.com wrote on Tue, 14 Dec 2021 17:24:10 +0100: > On Tue, 14 Dec 2021 12:41:39 +0100 > Miquel Raynal wrote: > > > This driver can be simplified a little bit by using > > spi_mem_generic_supports_op() instead of the > > spi_mem_default/dtr_supports_op() couple. The all_false boolean is > > inverted to become a dtr boolean, which checks if at least one of the > > operation member uses dtr mode. The idea behind this change is to > > simplify the introduction of the pipelined ECC engine. > > > > Signed-off-by: Miquel Raynal > > --- > > drivers/spi/spi-mxic.c | 10 +++------- > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c > > index 485a7f2afb44..5e71aa630504 100644 > > --- a/drivers/spi/spi-mxic.c > > +++ b/drivers/spi/spi-mxic.c > > @@ -452,7 +452,7 @@ static ssize_t mxic_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc, > > static bool mxic_spi_mem_supports_op(struct spi_mem *mem, > > const struct spi_mem_op *op) > > { > > - bool all_false; > > + struct spi_mem_controller_caps caps = {}; > > > > if (op->data.buswidth > 8 || op->addr.buswidth > 8 || > > op->dummy.buswidth > 8 || op->cmd.buswidth > 8) > > @@ -465,13 +465,9 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem, > > if (op->addr.nbytes > 7) > > return false; > > > > - all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr && > > - !op->data.dtr; > > + caps.dtr = op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr; > > Are you sure that's what you want to do? spi_mem_controller_caps is > supposed to encode the controller capabilities, not whether the > operation contains a DTR cycle or not. I'd expect this caps object to be > statically defined, with possibly one instance per-compat if the caps > depend on the HW revision. In order to keep the series easy to review I decided to go for the following approach: * Introduce the spi_mem_generic_supports_op_helper() which takes a capabilities structure. This helper gathers all the checks from spi_mem_default_supports_op() and spi_mem_dtr_supports_op(). These two helpers now call the new one with either a NULL pointer in the former case, or a structure with the .dtr parameter set to true in the latter. * Change the API of spi_mem_default_supports_op(), this involves updating many different drivers so this change does only that in a very transparent way, with no functional changes at all. All the drivers provide a NULL parameter for the capabilities structure. * Actually make use of the new parameter of spi_mem_default_supports_op() in the drivers Cadence and Macronix, which do have DTR support. This kills the spi_mem_dtr_supports_op() helper. * Kill the temporary spi_mem_generic_supports_op() helper by moving all the logic back into spi_mem_default_supports_op(). This approach is really straightforward and easily bisectable if needed. While working on this, I fixed the check we discussed on IRC about the command parameter when in a DTR operation. I also reverted the logic in the various checks, as you suggested. Thanks, Miquèl