linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Serge Semin <fancer.lancer@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>,
	Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>,
	Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>,
	Lars Povlsen <lars.povlsen@microchip.com>,
	"wuxu . wu" <wuxu.wu@huawei.com>, Feng Tang <feng.tang@intel.com>,
	Rob Herring <robh+dt@kernel.org>, <linux-spi@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 02/21] spi: dw: Add DWC SSI capability
Date: Fri, 2 Oct 2020 20:18:49 +0300	[thread overview]
Message-ID: <20201002171849.jhio6nz6r6jigrf3@mobilestation> (raw)
In-Reply-To: <20201002101929.GW3956970@smile.fi.intel.com>

On Fri, Oct 02, 2020 at 01:19:29PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 02, 2020 at 01:28:10AM +0300, Serge Semin wrote:
> > Currently DWC SSI core is supported by means of setting up the
> > core-specific update_cr0() callback. It isn't suitable for multiple
> > reasons. First of all having exported several methods doing the same thing
> > but for different chips makes the code harder to maintain. Secondly the
> > spi-dw-core driver exports the methods, then the spi-dw-mmio driver sets
> > the private data callback with one of them so to be called by the core
> > driver again. That makes the code logic too complicated. Thirdly using
> > callbacks for just updating the CR0 register is problematic, since in case
> > if the register needed to be updated from different parts of the code,
> > we'd have to create another callback (for instance the SPI device-specific
> > parameters don't need to be calculated each time the SPI transfer is
> > submitted, so it's better to pre-calculate the CR0 data at the SPI-device
> > setup stage).
> > 
> > So keeping all the above in mind let's discard the update_cr0() callbacks,
> > define a generic and static dw_spi_update_cr0() method and create the
> > DW_SPI_CAP_DWC_SSI capability, which when enabled would activate the
> > alternative CR0 register layout.
> > 
> > While at it add the comments to the code path of the normal DW APB SSI
> > controller setup to make the dw_spi_update_cr0() method looking coherent.
> 

> What the point to increase indentation level and produce additional churn?
> Can't you simply leave functions, unexport them, and call in one conditional of
> whatever new function is called?

I forgot to mention that in the commit log, there is another reason why it's
better to create a generic dw_spi_update_cr0() instead of doing what you suggest.
As it will be seen from the following up patches, the dw_spi_update_cr0() function
(to be more precise it's successor, but anyway) will be used from the SPI memory
ops implementation. So if-else-ing here and there isn't a good idea for
maintainability. For the same reason of the maintainability it's better to have a
generic method which reflects all the config peculiarities, so in case of any
changes they would be not be forgotten to be introduced for both DWC SSI and DW
APB SSI parts of the setup procedures. As I see it that overbeats the additional
indentation level drawback.

-Sergey

> 
> I have an impression that split of the series is done in a way that first
> patches in the series are not optimized to what is done in the last patches in
> the series.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

  reply	other threads:[~2020-10-02 17:18 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-01 22:28 [PATCH v3 00/21] spi: dw: Add full Baikal-T1 SPI Controllers support Serge Semin
2020-10-01 22:28 ` [PATCH v3 01/21] spi: dw: Use an explicit set_cs assignment Serge Semin
2020-10-01 22:28 ` [PATCH v3 02/21] spi: dw: Add DWC SSI capability Serge Semin
2020-10-02 10:19   ` Andy Shevchenko
2020-10-02 17:18     ` Serge Semin [this message]
2020-10-02 18:26       ` Andy Shevchenko
2020-10-02 19:46         ` Serge Semin
2020-10-02 20:08           ` Serge Semin
2020-10-02 20:12             ` Serge Semin
2020-10-01 22:28 ` [PATCH v3 03/21] spi: dw: Detach SPI device specific CR0 config method Serge Semin
2020-10-02 10:22   ` Andy Shevchenko
2020-10-02 17:47     ` Serge Semin
2020-10-02 18:24       ` Andy Shevchenko
2020-10-02 19:56         ` Serge Semin
2020-10-01 22:28 ` [PATCH v3 04/21] spi: dw: Update SPI bus speed in a config function Serge Semin
2020-10-01 22:28 ` [PATCH v3 05/21] spi: dw: Simplify the SPI bus speed config procedure Serge Semin
2020-10-01 22:28 ` [PATCH v3 06/21] spi: dw: Update Rx sample delay in the config function Serge Semin
2020-10-01 22:28 ` [PATCH v3 07/21] spi: dw: Add DW SPI controller config structure Serge Semin
2020-10-01 22:28 ` [PATCH v3 08/21] spi: dw: Refactor data IO procedure Serge Semin
2020-10-01 22:28 ` [PATCH v3 09/21] spi: dw: Refactor IRQ-based SPI transfer procedure Serge Semin
2020-10-01 22:28 ` [PATCH v3 10/21] spi: dw: Perform IRQ setup in a dedicated function Serge Semin
2020-10-01 22:28 ` [PATCH v3 11/21] spi: dw: Unmask IRQs after enabling the chip Serge Semin
2020-10-01 22:28 ` [PATCH v3 12/21] spi: dw: Discard chip enabling on DMA setup error Serge Semin
2020-10-01 22:28 ` [PATCH v3 13/21] spi: dw: De-assert chip-select on reset Serge Semin
2020-10-01 22:28 ` [PATCH v3 14/21] spi: dw: Explicitly de-assert CS on SPI transfer completion Serge Semin
2020-10-01 22:28 ` [PATCH v3 15/21] spi: dw: Move num-of retries parameter to the header file Serge Semin
2020-10-01 22:28 ` [PATCH v3 16/21] spi: dw: Add generic DW SSI status-check method Serge Semin
2020-10-01 22:28 ` [PATCH v3 17/21] spi: dw: Add memory operations support Serge Semin
2020-10-01 22:28 ` [PATCH v3 18/21] spi: dw: Introduce max mem-ops SPI bus frequency setting Serge Semin
2020-10-01 22:28 ` [PATCH v3 19/21] spi: dw: Add poll-based SPI transfers support Serge Semin
2020-10-01 22:28 ` [PATCH v3 20/21] dt-bindings: spi: dw: Add Baikal-T1 SPI Controllers Serge Semin
2020-10-01 22:28 ` [PATCH v3 21/21] spi: dw: Add Baikal-T1 SPI Controller glue driver Serge Semin
2020-10-02 10:24 ` [PATCH v3 00/21] spi: dw: Add full Baikal-T1 SPI Controllers support Andy Shevchenko
2020-10-02 12:55   ` Mark Brown
2020-10-02 18:26     ` Andy Shevchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201002171849.jhio6nz6r6jigrf3@mobilestation \
    --to=sergey.semin@baikalelectronics.ru \
    --cc=Alexey.Malahov@baikalelectronics.ru \
    --cc=Pavel.Parkhomenko@baikalelectronics.ru \
    --cc=Ramil.Zaripov@baikalelectronics.ru \
    --cc=andy.shevchenko@gmail.com \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=fancer.lancer@gmail.com \
    --cc=feng.tang@intel.com \
    --cc=lars.povlsen@microchip.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=wuxu.wu@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).