[1/5] spi: do not fail if the CS line is not connected

Commit Message

Andi Shyti June 27, 2016, 10:57 a.m. UTC

Hi Mark,

> > > > This is true, but there are cases where the CS is not connected
> > > > and this case needs to be treated separately to allow the device
> > > > to work.
> 
> > > In what way?  It is just as easy for a device with no physical chip
> > > select to have a logical chip select of 0 that it does nothing with as
> > > it is for that device to handle any other number.
> 
> > That is indeed my case: the s3c64xx doesn't send anything, unless
> > I manually enable CS (from the next patches I need to write '0'
> > in the CS register). But I need smoething to tell to the
> > device that the CS line is not connected, for example a flag in
> > the DTS.
> 
> I'm sorry but I just don't understand what you're saying here.  You are
> saying that there is no chip select but you need to enable the chip
> select?

yes, sorry, I have to admit that didn't explain myself properly,
but code speaks better than words.

What I meant is that if we do not like num-cs = <0>, the
unlinked CS line can be handled only this way (case of the
s3c64xx driver):






Which is not something I like, because it means adding a new
flag in the dts.

What I want to suggest, instead, is to slightly change the logic
behind the num-cs property: i.e. if "num-cs = <0>", doesn't
necessarily mean that we don't have a CS controller, but, while
we can have as many as we wish, non of them is connected.

Looking around in the spi drivers, apparently I am not the only
one who has this need, as I pointed out in my previous mail.

Thanks a lot,
Andi

Comments

Mark Brown June 27, 2016, 1:06 p.m. UTC | #1

On Mon, Jun 27, 2016 at 07:57:16PM +0900, Andi Shyti wrote:

> What I meant is that if we do not like num-cs = <0>, the
> unlinked CS line can be handled only this way (case of the
> s3c64xx driver):

> +- broken-cs: the CS line is disconnected, therefore the device should not wait
> +  for the CS protocol to be established

So what you're saying here is that you just need a property for the
inability to read back the chip select status?  That seems like a
totally reasonable thing to have which fits in idiomatically with the
rest of the subsystem.  I might call it no-cs-readback or something.

> Which is not something I like, because it means adding a new
> flag in the dts.

> What I want to suggest, instead, is to slightly change the logic
> behind the num-cs property: i.e. if "num-cs = <0>", doesn't
> necessarily mean that we don't have a CS controller, but, while
> we can have as many as we wish, non of them is connected.

I disagree, I think from a system integration point of view this is just
a chip select which can't be changed and it's less likely that we will
run into nasty surprises later on with things assuming that chip selects
exist.  AFAICT you only need this property in your case because this
controller has some features that rely on readback of the chip select
status, that's not very common - normally it'd be write only.  I'd
expect most controllers would just say they have one chip select and
that'd be that.

Andi Shyti June 27, 2016, 2:08 p.m. UTC | #2

Hi Mark,

> > What I meant is that if we do not like num-cs = <0>, the
> > unlinked CS line can be handled only this way (case of the
> > s3c64xx driver):
> 
> > +- broken-cs: the CS line is disconnected, therefore the device should not wait
> > +  for the CS protocol to be established
> 
> So what you're saying here is that you just need a property for the
> inability to read back the chip select status?  That seems like a
> totally reasonable thing to have which fits in idiomatically with the
> rest of the subsystem.  I might call it no-cs-readback or something.
> 
> > Which is not something I like, because it means adding a new
> > flag in the dts.
> 
> > What I want to suggest, instead, is to slightly change the logic
> > behind the num-cs property: i.e. if "num-cs = <0>", doesn't
> > necessarily mean that we don't have a CS controller, but, while
> > we can have as many as we wish, non of them is connected.
> 
> I disagree, I think from a system integration point of view this is just
> a chip select which can't be changed and it's less likely that we will
> run into nasty surprises later on with things assuming that chip selects
> exist.  AFAICT you only need this property in your case because this
> controller has some features that rely on readback of the chip select
> status, that's not very common - normally it'd be write only.  I'd
> expect most controllers would just say they have one chip select and
> that'd be that.

thanks for your feedback, I will then do as you say, I will use
the no-cs-readback flag.

Thanks again,
Andi

Patch

diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt
index 6dbdeb3..62f19b6 100644
--- a/Documentation/devicetree/bindings/spi/spi-samsung.txt
+++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt
@@ -40,6 +40,9 @@  Optional Board Specific Properties:
 
 - cs-gpios: should specify GPIOs used for chipselects (see spi-bus.txt)
 
+- broken-cs: the CS line is disconnected, therefore the device should not wait
+  for the CS protocol to be established
+
 SPI Controller specific data in SPI slave nodes:
 
 - The spi slave nodes should provide the following information which is required
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 972367d..5e6ad59 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -315,6 +315,9 @@  static void s3c64xx_spi_set_cs(struct spi_device *spi, bool enable)
        struct s3c64xx_spi_driver_data *sdd =
                                        spi_master_get_devdata(spi->master);
 
+       if (sdd->cntrlr_info->broken_cs)
+               return;
+
        if (enable) {
                if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO)) {
                        writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);

[ ... ]

@@ -1015,6 +1020,8 @@  static struct s3c64xx_spi_info *s3c64xx_spi_parse_dt(struct device *dev)
                sci->num_cs = temp;
        }
 
+       sci->broken_cs = of_property_read_bool(dev->of_node, "broken-cs");
+
        return sci;
 }
 #else
diff --git a/include/linux/platform_data/spi-s3c64xx.h b/include/linux/platform_data/spi-s3c64xx.h
index fb5625b..75793b9 100644
--- a/include/linux/platform_data/spi-s3c64xx.h
+++ b/include/linux/platform_data/spi-s3c64xx.h
@@ -38,6 +38,7 @@  struct s3c64xx_spi_csinfo {
 struct s3c64xx_spi_info {
        int src_clk_nr;
        int num_cs;
+       bool broken_cs;
        int (*cfg_gpio)(void);
        dma_filter_fn filter;
        void *dma_tx;