Re: [PATCH v1] Input: ads7846: do not overwrite spi->mode flags set by spi framework

[Oleksij Rempel]

On Wed, Oct 21, 2020 at 09:29:35AM +0000, Ardelean, Alexandru wrote:
> 
> 
> > -----Original Message-----
> > From: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> > Sent: Wednesday, October 21, 2020 12:05 PM
> > To: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>; Ardelean, Alexandru
> > <alexandru.Ardelean@xxxxxxxxxx>
> > Cc: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>; kernel@xxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; linux-input@xxxxxxxxxxxxxxx; David Jander
> > <david@xxxxxxxxxxx>
> > Subject: [PATCH v1] Input: ads7846: do not overwrite spi->mode flags set by spi
> > framework
> > 
> > [External]
> > 
> > Do not overwrite spi->mode flags set by spi framework, otherwise the chip
> > select polarity will get lost.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> > ---
> >  drivers/input/touchscreen/ads7846.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/input/touchscreen/ads7846.c
> > b/drivers/input/touchscreen/ads7846.c
> > index 8fd7fc39c4fd..ea31956f3a90 100644
> > --- a/drivers/input/touchscreen/ads7846.c
> > +++ b/drivers/input/touchscreen/ads7846.c
> > @@ -1288,7 +1288,7 @@ static int ads7846_probe(struct spi_device *spi)
> >  	 * may not.  So we stick to very-portable 8 bit words, both RX and TX.
> >  	 */
> >  	spi->bits_per_word = 8;
> > -	spi->mode = SPI_MODE_0;
> 
> I think the patch is incorrect.
> The initial version is correct; assuming that the datasheet says that this driver operates in mode 0.
> If the initial mode is incorrect, maybe we need to change that.
> 
> What is unfortunate, is that you cannot [yet] override the mode parameters [polarity & phase] from the device-tree, in case there are some things in-between the SPI controller & SPI chip [level inverters for example].
> I was planning to do something for this.

Current kernel (v5.9) is doing following work:

  of_register_spi_device()
    of_spi_parse_dt()
      /* this will parse dt and set different flags in spi->mode
       * all of this flags are dropped by this driver
       */

        ...... and here we parse gpio properties ......

	/*
	 * For descriptors associated with the device, polarity inversion is
	 * handled in the gpiolib, so all gpio chip selects are "active high"
	 * in the logical sense, the gpiolib will invert the line if need be.
	 */
	if ((ctlr->use_gpio_descriptors) && ctlr->cs_gpiods &&
	    ctlr->cs_gpiods[spi->chip_select])
		spi->mode |= SPI_CS_HIGH;
        -------->  ^^^^^^^^
	as you can see, if we use gpio as chip select, then the SPI_CS_HIGH
	flag is set. And gpiolib make all needed level conversation.

Since this diver is removing SPI_CS_HIGH flag, we have fallowing call
sequence:

spi_set_cs(, enable)                                        <---- 1
  ....
  if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio)) {
     ....
     gpiod_set_value_cansleep(, !enable);                   <---- 0
       gpiod_set_value_nocheck()
         if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
	   value = !value;                                  <---- 1


So, at the end, we set GPIO to 1, even if DTS configured it as ACTIVE_LOW.
You may probably suggest to set gpio in DTS to active ACTIVE_HIGH. In
this case we would run it to following snippet:
of_get_named_gpiod_flags()
  of_gpio_flags_quirks()
    if (IS_ENABLED(CONFIG_SPI_MASTER) && !strcmp(propname, "cs-gpios")..
	/*
	 * SPI children have active low chip selects
	 * by default. This can be specified negatively
	 * by just omitting "spi-cs-high" in the
	 * device node, or actively by tagging on
	 * GPIO_ACTIVE_LOW as flag in the device
	 * tree. If the line is simultaneously
	 * tagged as active low in the device tree
	 * and has the "spi-cs-high" set, we get a
	 * conflict and the "spi-cs-high" flag will
	 * take precedence.
	 */
	if (of_property_read_bool(child, "spi-cs-high")) {
		if (*flags & OF_GPIO_ACTIVE_LOW) {
			pr_warn("%s GPIO handle specifies active low - ignored\n",
				of_node_full_name(child));
			*flags &= ~OF_GPIO_ACTIVE_LOW;
		}
	} else {
		if (!(*flags & OF_GPIO_ACTIVE_LOW))
			pr_info("%s enforce active low on chipselect handle\n",
				of_node_full_name(child));
		*flags |= OF_GPIO_ACTIVE_LOW;
	}

As you can see, I would need to configure my dts with spi-cs-high flag,
even if the hardware is actually ACTIVE_LOW. If I will go this way, I
would risk a regression as soon as this issue is fixed.

Since the spi framework is already parsing devicetree and set all needed
flags, I assume it is wrong to blindly drop all this flags in the
driver.

> > +	spi->mode |= SPI_MODE_0;
> >  	err = spi_setup(spi);
> >  	if (err < 0)
> >  		return err;
> > --
> > 2.28.0

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |