Понедельник, 2 декабря 2013, 15:59 +01:00 от Gerhard Sittig <g...@denx.de>:
> On Sun, Dec 01, 2013 at 11:59 +0400, Alexander Shiyan wrote:
> >
> > This patch takes the CS active level from the GPIO bindings,
> > so we remove the special property "spi-cs-high" for chipselects
> > that use GPIO.
>
> I'm afraid this description does not reflect what the patch does.
> This is not saying that the patch is wrong, but suggests that the
> description may be in need of an update.
>
> Whether there is some gain in the change is a different matter,
> see below.
>
>
> > Signed-off-by: Alexander Shiyan <shc_w...@mail.ru>
> > ---
> > drivers/spi/spi.c | 28 ++++++++++++++++++----------
> > include/linux/spi/spi.h | 2 ++
> > 2 files changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index 98f4b77..d2ba1ec 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -413,8 +413,12 @@ int spi_add_device(struct spi_device *spi)
> > goto done;
> > }
> >
> > - if (master->cs_gpios)
> > + if (master->cs_gpios) {
> > spi->cs_gpio = master->cs_gpios[spi->chip_select];
> > + if (!(master->cs_gpios_flags[spi->chip_select] &
> > + OF_GPIO_ACTIVE_LOW))
> > + spi->mode |= SPI_CS_HIGH;
> > + }
> >
> > /* Drivers may modify this initial i/o setup, but will
> > * normally rely on the device being setup. Devices
>
> OK, so you accept GPIO flags as an _additional_ source of
> information, to derive "CS high" attributes in the SPI subsystem
> in the (potential) absence of explicit specs in the device tree.
>
> I'm not quite certain whether the GPIO pin's "not being inverted"
> (not being low-active, the de-facto default for a GPIO pin) and
> the SPI chip select's being high-active really are the same
> thing. Unless I'm missing something, this is quite a change in
> semantics. (Should this automatic deriving attributes emit a
> message as well, to raise awareness? Are we spamming users then,
> or do these go unnoticed und ignored?)
>
> Does this patch _force_ those who use GPIOs for SPI CS to adjust
> their GPIO configuration in device trees? Or make SPI
> communication fail if the device tree won't get adjusted? Should
> you adjust users as well, and put a big fat remark into the
> documentation?
>
> A quick 'git grep spi-cs-high' reveals just one in-tree user, but
> I'd expect quite a lot out-of-tree users which assume "regular,
> straight semantics".
>
>
> > @@ -1027,10 +1031,14 @@ static void of_register_spi_devices(struct
> > spi_master *master)
> > spi->mode |= SPI_CPHA;
> > if (of_find_property(nc, "spi-cpol", NULL))
> > spi->mode |= SPI_CPOL;
> > - if (of_find_property(nc, "spi-cs-high", NULL))
> > - spi->mode |= SPI_CS_HIGH;
> > if (of_find_property(nc, "spi-3wire", NULL))
> > spi->mode |= SPI_3WIRE;
> > + if (of_find_property(nc, "spi-cs-high", NULL)) {
> > + if (master->cs_gpios)
> > + dev_notice(&master->dev,
> > + "\"spi-cs-high\" property is
> > obsolete.\n");
> > + spi->mode |= SPI_CS_HIGH;
> > + }
> >
> > /* Device DUAL/QUAD mode */
> > if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
>
> Here you keep the evaluation of the 'spi-cs-high' property.
> Which is a good thing, and should not go away as Mark has
> suggested, at least not without proper period of migration.
>
> You introduce a diagnostic message if SPI CS is high-active (and
> this spec comes from the SPI node) and the CS is backed by a GPIO
> pin.
>
> Considering that you cannot remove the "CS high-active" attribute
> in general, and need to keep the 'spi-cs-high' property for those
> cases where the signal is not driven by means of GPIO, and keep
> backwards compatibility for GPIO backed signals -- is there
> really any gain in the change?
>
> I'm afraid that forcing users to specify the same "this SPI
> slave's CS is high-active" information in different ways
> depending on whether the signal is generated by GPIO or by other
> means is not a good thing. I'm even questioning the benefit of
> introducing the optional second source of this information by
> deriving it from the GPIO flags. Am I missing something?
Using GPIO flags is basically better.
Imagine that tomorrow we may want the option to define open-drain
output on the CS pin. Specific pullup or pulldown.
Another separate properties?
---
N�����r��y����b�X��ǧv�^�){.n�+����{������^n�r���z���h�����&�� �G���h�(�階�ݢj"���m������z�ޖ���f���h���~�m�
