| Message ID | 20170211035447.3819-1-viniciusfre@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Feb 11, 2017 at 12:54:47AM -0300, Vinicius Maciel wrote: > In order to work appropriately, the max11043 ADC chip and probably > others, needs SPI master samples the data at the correct edge. From > max11043 datasheet: "The data at DIN is latched on the rising edge > of SCLK". Same to DOUT. > > This patch add Master Sample Data Mode bit in normal sample mode. > It will affect only A20. > > Signed-off-by: Vinicius Maciel <viniciusfre@gmail.com> > Reviewed-by: Siarhei Siamashka <siarhei.siamashka@gmail.com> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> Maxime
On Sat, Feb 11, 2017 at 12:54:47AM -0300, Vinicius Maciel wrote: > In order to work appropriately, the max11043 ADC chip and probably > others, needs SPI master samples the data at the correct edge. From > max11043 datasheet: "The data at DIN is latched on the rising edge > of SCLK". Same to DOUT. > > This patch add Master Sample Data Mode bit in normal sample mode. > It will affect only A20. This should be controlled by the SPI mode settings, different chips have different requirements. If the controller supports multiple modes then it should expose that and let the drivers and system integrations choose.
On Sun, 26 Feb 2017 06:10:50 -0800 (PST) Vinicius Maciel <viniciusfre@gmail.com> wrote: > Em segunda-feira, 13 de fevereiro de 2017 09:52:53 UTC-3, Mark Brown > escreveu: > > > > This should be controlled by the SPI mode settings, different chips have > > different requirements. If the controller supports multiple modes then > > it should expose that and let the drivers and system integrations > > choose. > > > Do you mean SPI_CPHA/SPI_CPOL, right? Well, I don't know how this mode can > to fit in existing modes. Since this bit is to control when the data will > be sampled, in the same clock edge or at the edge that is half cycle > delayed. That has nothing to do with SPI_CPHA/SPI_CPOL. I'm not sure if it is realistic to make any progress with this issue any time soon, but I would suggest you to re-send the patch with an updated commit message, which could better explain what's going on. I still think that your bugfix is good, that's why you got my Reviewed-by tag. And we had an older discussion about it (Mark was not participating there, so probably he missed it): https://patchwork.kernel.org/patch/9567547/ Here is a short summary. Older Allwinner A10/A13 SoCs used to have reserved RAZ/WI (Read-As-Zero, Writes Ignored) bits 20-31 in the SPI_CTL register. Newer Allwinner A20 SoC introduced some sort of a wacky half-cycle delay mode and used the previously reserved bit 20 from the SPI_CTL to enable/disable it. Now the tricky part is that the meaning of this new bit is inverted for some unknown reason: the wacky mode is activated when the bit is set to zero, while the standard behaviour (same as A10/A13) is preserved when this bit is set to 1. The reset default of this bit is 1 on A20 (normal SPI behaviour). The problem of the current SPI driver is that it clears bits 20-31 of SPI_CTL, which results in normal SPI behaviour on A10/A13, and a wacky half-cycle delayed mode on A20. Your bugfix just makes A20 behave in the same way as A10/A13. We have exactly zero users of the wacky half-cycle delayed mode. Moreover, if it gets enabled accidentally (due to a bug in the SPI_CTL register initialization in the SPI driver as explained above), then it breaks your max11043 ADC chip. Also even if we assume that there is some hypothetical equipment, which actually needs this wacky mode to function preperly, then this equipment is not compatible with older A10/A13 SoCs because the older SoCs did not implement this mode at all. You could rephrase the above two paragraphs and use them as the commit message for the v3 patch. And maybe also add an extra comment to the SPI driver code. If Mark has any comments on this, then I would be very happy to hear them. So far his suggestion looks like he prefers to see a support for this wacky mode implemented in the SPI driver (a *new feature*, which has no actual users and can't be realistically tested). While your patch just prevents accidental and undesired activation of the wacky mode (an *important bugfix* for a real problem encountered in the wild).
diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c index c5cd635c28f3..6325be2ce8d9 100644 --- a/drivers/spi/spi-sun4i.c +++ b/drivers/spi/spi-sun4i.c @@ -44,6 +44,7 @@ #define SUN4I_CTL_CS_MANUAL BIT(16) #define SUN4I_CTL_CS_LEVEL BIT(17) #define SUN4I_CTL_TP BIT(18) +#define SUN4I_CTL_SDM BIT(20) #define SUN4I_INT_CTL_REG 0x0c #define SUN4I_INT_CTL_RF_F34 BIT(4) @@ -407,7 +408,8 @@ static int sun4i_spi_runtime_resume(struct device *dev) } sun4i_spi_write(sspi, SUN4I_CTL_REG, - SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER | SUN4I_CTL_TP); + SUN4I_CTL_ENABLE | SUN4I_CTL_MASTER | SUN4I_CTL_TP | + SUN4I_CTL_SDM); return 0;