Re: [PATCH 6/9] spi: imx: remove unnecessary bit clearing in mx51_ecspi_config

[Vladimir Zapolskiy <vladimir_zapolskiy@xxxxxxxxxx>]

On 04.03.2016 13:47, Sascha Hauer wrote:
> On Fri, Mar 04, 2016 at 12:52:15PM +0200, Vladimir Zapolskiy wrote:
>> Hi Sascha,
>>
>> On 24.02.2016 10:20, Sascha Hauer wrote:
>>> This reverts patch 1476253cef (spi: imx: fix ecspi mode setup)
>>> The patch tried to fix something by clearing bits in the cfg variable,
>>> but cfg is initialized to zero on function entry. There are no bits to
>>> clear.
>>>
>>> Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
>>
>> I believe here the expected fix should be
>>
>> ----8<----
>>
>> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
>> index 6a4ff27..9c9bae0 100644
>> --- a/drivers/spi/spi-imx.c
>> +++ b/drivers/spi/spi-imx.c
>> @@ -316,9 +316,10 @@ static void __maybe_unused mx51_ecspi_trigger(struct
>> spi_imx_data *spi_imx)
>>  static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
>>  		struct spi_imx_config *config)
>>  {
>> -	u32 ctrl = MX51_ECSPI_CTRL_ENABLE, cfg = 0, dma = 0;
>> +	u32 ctrl = MX51_ECSPI_CTRL_ENABLE, dma = 0;
>>  	u32 tx_wml_cfg, rx_wml_cfg, rxt_wml_cfg;
>>  	u32 clk = config->speed_hz, delay, reg;
>> +	u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
>>
> 
> With this change applied we would indeed have to clear and not only set the bits,
> but why would we do this?

Locally we have quite a similar to 1476253cef change (well, correct version
of it) authored by Knut Wohlrab, here is his comment from a discussion:

Please notice that iMX6 ECSPI controller support 4 channels/chip selects at
one SPI bus. The original iMX driver implementation(*) did not care about
the SPI_CPHA, SPI_CPOL and SPI_CS_HIGH configuration of the not selected
channels and set this to "0". Depending on the configuration of this
channels this could cause unwanted edges at the clock line when using this
channels the next time. To prevent this, we introduce the read of the
actual configuration/register content and set only the necessary changes.

(*) here it means the version prior to 1476253cef ("spi: imx: fix ecspi mode
setup)

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html