Re: [PATCH v4 5/6] spi: slave: Add SPI slave handler reporting uptime at previous message

2017-05-22 Thread Andy Shevchenko 
On Mon, May 22, 2017 at 1:13 PM, Geert Uytterhoeven
 wrote:
> On Thu, May 18, 2017 at 6:01 PM, Andy Shevchenko
>  wrote:
>> On Wed, May 17, 2017 at 3:47 PM, Geert Uytterhoeven
>>  wrote:

>>> +   rem_ns = do_div(ts, 10) / 1000;
>>
>> You divide ts by 10^9, which makes it seconds if it was nanoseconds.
>> But reminder is still in nanoseconds and you divide it by 10^3.
>
>> If I didn't miss anything it should be called like
>> rem_ns -> reminder_ms
>
> Thanks, that must be a remainder from before I reworked the calculation.
> Will change it to rem_us (it's in microseconds, not milliseconds).

Yeah, correct, thanks.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 5/6] spi: slave: Add SPI slave handler reporting uptime at previous message

2017-05-22 Thread Geert Uytterhoeven 
Hi Andy,

On Thu, May 18, 2017 at 6:01 PM, Andy Shevchenko
 wrote:
> On Wed, May 17, 2017 at 3:47 PM, Geert Uytterhoeven
>  wrote:
>> Add an example SPI slave handler responding with the uptime at the time
>> of reception of the last SPI message.
>>
>> This can be used by an external microcontroller as a dead man's switch.
>
>> +static int spi_slave_time_submit(struct spi_slave_time_priv *priv)
>> +{
>> +   u32 rem_ns;
>> +   int ret;
>> +   u64 ts;
>> +
>> +   ts = local_clock();
>> +   rem_ns = do_div(ts, 10) / 1000;
>
> You divide ts by 10^9, which makes it seconds if it was nanoseconds.
>
> But reminder is still in nanoseconds and you divide it by 10^3.

> If I didn't miss anything it should be called like
>
> rem_ns -> reminder_ms

Thanks, that must be a remainder from before I reworked the calculation.

Will change it to rem_us (it's in microseconds, not milliseconds).

>> +   priv->buf[0] = cpu_to_be32(ts);
>> +   priv->buf[1] = cpu_to_be32(rem_ns);
>> +
>> +   spi_message_init_with_transfers(>msg, >xfer, 1);
>> +
>> +   priv->msg.complete = spi_slave_time_complete;
>> +   priv->msg.context = priv;
>> +
>> +   ret = spi_async(priv->spi, >msg);
>> +   if (ret)
>> +   pr_err("%s: spi_async() failed %d\n", __func__, ret);
>
> Perhaps dev_err() ?

OK, and after that the __func__ is no longer needed.

>> +static int spi_slave_time_probe(struct spi_device *spi)
>> +{
>> +   struct spi_slave_time_priv *priv;
>> +   int ret;
>> +
>> +   /*
>> +* bits_per_word cannot be configured in platform data
>> +*/
>> +   spi->bits_per_word = 8;
>
> Is it worth to define it? If so, can we use device properties for that?

No, it can be removed, as 8 is the default.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH v4 5/6] spi: slave: Add SPI slave handler reporting uptime at previous message

2017-05-18 Thread Andy Shevchenko 
On Wed, May 17, 2017 at 3:47 PM, Geert Uytterhoeven
 wrote:
> Add an example SPI slave handler responding with the uptime at the time
> of reception of the last SPI message.
>
> This can be used by an external microcontroller as a dead man's switch.

> +static int spi_slave_time_submit(struct spi_slave_time_priv *priv)
> +{
> +   u32 rem_ns;
> +   int ret;
> +   u64 ts;
> +
> +   ts = local_clock();
> +   rem_ns = do_div(ts, 10) / 1000;

You divide ts by 10^9, which makes it seconds if it was nanoseconds.

But reminder is still in nanoseconds and you divide it by 10^3.

If I didn't miss anything it should be called like

rem_ns -> reminder_ms

> +
> +   priv->buf[0] = cpu_to_be32(ts);
> +   priv->buf[1] = cpu_to_be32(rem_ns);
> +
> +   spi_message_init_with_transfers(>msg, >xfer, 1);
> +
> +   priv->msg.complete = spi_slave_time_complete;
> +   priv->msg.context = priv;
> +
> +   ret = spi_async(priv->spi, >msg);
> +   if (ret)
> +   pr_err("%s: spi_async() failed %d\n", __func__, ret);

Perhaps dev_err() ?

> +
> +   return ret;
> +}

> +static int spi_slave_time_probe(struct spi_device *spi)
> +{
> +   struct spi_slave_time_priv *priv;
> +   int ret;
> +
> +   /*
> +* bits_per_word cannot be configured in platform data
> +*/
> +   spi->bits_per_word = 8;

Is it worth to define it? If so, can we use device properties for that?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 5/6] spi: slave: Add SPI slave handler reporting uptime at previous message

2017-05-17 Thread Geert Uytterhoeven 
Add an example SPI slave handler responding with the uptime at the time
of reception of the last SPI message.

This can be used by an external microcontroller as a dead man's switch.

Signed-off-by: Geert Uytterhoeven 
---
v4:
  - No changes,

v3:
  - Add #include ,

v2:
  - Resolve semantic differences in patch description, file header, and
module description,
  - Use spi_async() instead of spi_read(),
  - Submit the next transfer from the previous transfer's completion
callback, removing the need for a thread,
  - Let .remove() call spi_slave_abort() to cancel the current ongoing
transfer, and wait for the completion to terminate,
  - Remove FIXME about hanging kthread_stop().
---
 drivers/spi/Kconfig  |   6 ++
 drivers/spi/Makefile |   1 +
 drivers/spi/spi-slave-time.c | 127 +++
 3 files changed, 134 insertions(+)
 create mode 100644 drivers/spi/spi-slave-time.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index f21499b1f71ab7c3..9972ee2a8454a2fc 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -797,6 +797,12 @@ config SPI_SLAVE
 
 if SPI_SLAVE
 
+config SPI_SLAVE_TIME
+   tristate "SPI slave handler reporting boot up time"
+   help
+ SPI slave handler responding with the time of reception of the last
+ SPI message.
+
 endif # SPI_SLAVE
 
 endif # SPI
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index e50852c6fcb87d8b..fb078693dbe40da4 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -107,3 +107,4 @@ obj-$(CONFIG_SPI_XTENSA_XTFPGA) += 
spi-xtensa-xtfpga.o
 obj-$(CONFIG_SPI_ZYNQMP_GQSPI) += spi-zynqmp-gqspi.o
 
 # SPI slave protocol handlers
+obj-$(CONFIG_SPI_SLAVE_TIME)   += spi-slave-time.o
diff --git a/drivers/spi/spi-slave-time.c b/drivers/spi/spi-slave-time.c
new file mode 100644
index ..c2940f3f18ecd22e
--- /dev/null
+++ b/drivers/spi/spi-slave-time.c
@@ -0,0 +1,127 @@
+/*
+ * SPI slave handler reporting uptime at reception of previous SPI message
+ *
+ * This SPI slave handler sends the time of reception of the last SPI message
+ * as two 32-bit unsigned integers in binary format and in network byte order,
+ * representing the number of seconds and fractional seconds (in microseconds)
+ * since boot up.
+ *
+ * Copyright (C) 2016 Glider bvba
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+
+struct spi_slave_time_priv {
+   struct spi_device *spi;
+   struct completion finished;
+   struct spi_transfer xfer;
+   struct spi_message msg;
+   __be32 buf[2];
+};
+
+static int spi_slave_time_submit(struct spi_slave_time_priv *priv);
+
+static void spi_slave_time_complete(void *arg)
+{
+   struct spi_slave_time_priv *priv = arg;
+   int ret;
+
+   ret = priv->msg.status;
+   if (ret)
+   goto terminate;
+
+   ret = spi_slave_time_submit(priv);
+   if (ret)
+   goto terminate;
+
+   return;
+
+terminate:
+   pr_info("%s: Terminating\n", __func__);
+   complete(>finished);
+}
+
+static int spi_slave_time_submit(struct spi_slave_time_priv *priv)
+{
+   u32 rem_ns;
+   int ret;
+   u64 ts;
+
+   ts = local_clock();
+   rem_ns = do_div(ts, 10) / 1000;
+
+   priv->buf[0] = cpu_to_be32(ts);
+   priv->buf[1] = cpu_to_be32(rem_ns);
+
+   spi_message_init_with_transfers(>msg, >xfer, 1);
+
+   priv->msg.complete = spi_slave_time_complete;
+   priv->msg.context = priv;
+
+   ret = spi_async(priv->spi, >msg);
+   if (ret)
+   pr_err("%s: spi_async() failed %d\n", __func__, ret);
+
+   return ret;
+}
+
+static int spi_slave_time_probe(struct spi_device *spi)
+{
+   struct spi_slave_time_priv *priv;
+   int ret;
+
+   /*
+* bits_per_word cannot be configured in platform data
+*/
+   spi->bits_per_word = 8;
+
+   ret = spi_setup(spi);
+   if (ret < 0)
+   return ret;
+
+   priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
+   if (!priv)
+   return -ENOMEM;
+
+   priv->spi = spi;
+   init_completion(>finished);
+   priv->xfer.tx_buf = priv->buf;
+   priv->xfer.len = sizeof(priv->buf);
+
+   ret = spi_slave_time_submit(priv);
+   if (ret)
+   return ret;
+
+   spi_set_drvdata(spi, priv);
+   return 0;
+}
+
+static int spi_slave_time_remove(struct spi_device *spi)
+{
+   struct spi_slave_time_priv *priv = spi_get_drvdata(spi);
+
+   spi_slave_abort(spi);
+   wait_for_completion(>finished);
+   return 0;
+}
+
+static struct spi_driver spi_slave_time_driver = {
+   .driver = {
+   .name   = "spi-slave-time",
+   },
+   .probe