Review on spi driver

Review on spi driver

[Arthur LAMBERT]

Hi,

Not sure that it is the good place to do that. I am quite new in kernel dev and
it will be great to have some review/advise on a implementation in kernel.

My need : Read periodically data from a sensor with spi. I know that data is available
thanks to a GPIO. Data are available every 2 ms.

First step for me was to :

Declare a irq handler on the ready signal GPIO thanks to gpio_to_irq and request_irq
function. First I try to read from spi in my irq handler.I finally understand that I
cannot make call which implies sleep in irq handler like spi_sync for example.
I finally use wake_up_interruptible call in my irq handler to make the spi work
in another piece of code.

static irqreturn_t irq_handler(int irq, void *id)
{
	flag = 1;
	wake_up_interruptible(&wq);
	return IRQ_HANDLED;
}

On my first try I just use a kernel thread which read data from spi and then which send
my data to userspace thanks to a netlink socket.

   while(!kthread_should_stop())
   {
	wait_event_interruptible(wq, flag != 0 || kthread_should_stop());
	if (kthread_should_stop()) do_exit (0);
	flag = 0;

	// read data from spi
	(...)
	// send data in netlink socket
	skb = nlmsg_new(NLMSG_ALIGN(msg_size + 1), GFP_ATOMIC);
	nlh = nlmsg_put(skb, 0, 1, NLMSG_DONE, msg_size + 1, 0);
	memcpy(nlmsg_data(nlh), spi->rx_buf, msg_size);
	ret = nlmsg_multicast(nl_sk, skb, 0, MYGRP, GFP_ATOMIC);
	schedule(); // not sure that I need this line of code.
    }

Problem I was not sure that netlink socket was the best solution to my problem. Also
I was not able to use the same socket during my data acquisition. I must call nlmsg_new
on each new data sample. Other strange stuff, If I call nsmsg_free at the end of the
process I have big stability issue during my test. Moreover, if I am to slow to send
data from kernel space to user space, I will miss event from my irq handler and miss
sample from my sensor.

My second try was to use a character device to send data from kernel space to
userpsace. I was already using the character device to drive my kernel driver.
Open/Release to init/clean sensor. And ioctl to send command to the sensor like start/stop
acquisition. So my plan was just to add a read callback on the character device.

static const struct file_operations fops = {
       .owner = THIS_MODULE,
       .unlocked_ioctl = ads_ioctl,
       .open = ads_open,
       .release = ads_release,
       .read = ads_read,
       .unlocked_ioctl = ads_ioctl,
};

static int ads_read(struct file *filp, char __user *data, size_t len, loff_t *ppos)
{
	struct ads_dev *dev = filp->private_data;

	wait_event_interruptible(wq, flag != 0);
	flag = 0;
	// read data from spi
	(...)
	copy_to_user(data, spi->rx_buf, DATA_SIZE);
	return DATA_SIZE;
};

This code seems to be easy compare to the netlink solution. Even in the user space code
side. Just need to use a read instead of using socket and libnl. But the problem is
always there. If the read operation over the spi and the copy_to_user take more than 2 ms, I
will lost sample from my sensor.

My last plan was to use mixed the two previous solutions and use a kthread + the character
device. The irq wake up the thread. I read data over spi. I put the data in a queue.
The read callback from character device will gather data from the queue. I have always
a synchrone call which can take more than 2 ms. But At least I can send the data to user space
in a synchrone way.

But I was not able to find a proper way to use a queue with semaphore. So it was quite basic.
A static queue + offset + wake_up_interruptible/wait_event_interruptible to synchronize
kernel thread and my read callback.

static uint8_t queue[50 * DATA_SIZE];
static int read_offset = 0;
static int write_offset = 0;

I guess that my problems are quite common in kernel driver which want to gather some data over spi.
Is there a good kernel driver that I can take as good practice/example to implement my solution ?
Feel free to say that my code is stupid !

Thanks,
Arthur.

Review on spi driver

[Andrey Skvortsov]

On 16-10-07 18:28, Arthur LAMBERT wrote:
> 
> Hi,
> 
> Not sure that it is the good place to do that. I am quite new in kernel dev and
> it will be great to have some review/advise on a implementation in kernel.
> 
> My need : Read periodically data from a sensor with spi. I know that data is available
> thanks to a GPIO. Data are available every 2 ms.
> 
> First step for me was to :
> 
> Declare a irq handler on the ready signal GPIO thanks to gpio_to_irq and request_irq
> function. First I try to read from spi in my irq handler.I finally understand that I
> cannot make call which implies sleep in irq handler like spi_sync for example.
> I finally use wake_up_interruptible call in my irq handler to make the spi work
> in another piece of code.
> 
> static irqreturn_t irq_handler(int irq, void *id)
> {
> 	flag = 1;
> 	wake_up_interruptible(&wq);
> 	return IRQ_HANDLED;
> }
> 
> On my first try I just use a kernel thread which read data from spi and then which send
> my data to userspace thanks to a netlink socket.
> 
>    while(!kthread_should_stop())
>    {
> 	wait_event_interruptible(wq, flag != 0 || kthread_should_stop());
> 	if (kthread_should_stop()) do_exit (0);
> 	flag = 0;
> 
> 	// read data from spi
> 	(...)
> 	// send data in netlink socket
> 	skb = nlmsg_new(NLMSG_ALIGN(msg_size + 1), GFP_ATOMIC);
> 	nlh = nlmsg_put(skb, 0, 1, NLMSG_DONE, msg_size + 1, 0);
> 	memcpy(nlmsg_data(nlh), spi->rx_buf, msg_size);
> 	ret = nlmsg_multicast(nl_sk, skb, 0, MYGRP, GFP_ATOMIC);
> 	schedule(); // not sure that I need this line of code.
>     }
> 
> Problem I was not sure that netlink socket was the best solution to my problem. Also
> I was not able to use the same socket during my data acquisition. I must call nlmsg_new
> on each new data sample. Other strange stuff, If I call nsmsg_free at the end of the
> process I have big stability issue during my test. Moreover, if I am to slow to send
> data from kernel space to user space, I will miss event from my irq handler and miss
> sample from my sensor.
> 
> My second try was to use a character device to send data from kernel space to
> userpsace. I was already using the character device to drive my kernel driver.
> Open/Release to init/clean sensor. And ioctl to send command to the sensor like start/stop
> acquisition. So my plan was just to add a read callback on the character device.
> 
> static const struct file_operations fops = {
>        .owner = THIS_MODULE,
>        .unlocked_ioctl = ads_ioctl,
>        .open = ads_open,
>        .release = ads_release,
>        .read = ads_read,
>        .unlocked_ioctl = ads_ioctl,
> };
> 
> static int ads_read(struct file *filp, char __user *data, size_t len, loff_t *ppos)
> {
> 	struct ads_dev *dev = filp->private_data;
> 
> 	wait_event_interruptible(wq, flag != 0);
> 	flag = 0;
> 	// read data from spi
> 	(...)
> 	copy_to_user(data, spi->rx_buf, DATA_SIZE);
> 	return DATA_SIZE;
> };
> 
> This code seems to be easy compare to the netlink solution. Even in the user space code
> side. Just need to use a read instead of using socket and libnl. But the problem is
> always there. If the read operation over the spi and the copy_to_user take more than 2 ms, I
> will lost sample from my sensor.
> 
> My last plan was to use mixed the two previous solutions and use a kthread + the character
> device. The irq wake up the thread. I read data over spi. I put the data in a queue.
> The read callback from character device will gather data from the queue. I have always
> a synchrone call which can take more than 2 ms. But At least I can send the data to user space
> in a synchrone way.
> 
> But I was not able to find a proper way to use a queue with semaphore. So it was quite basic.
> A static queue + offset + wake_up_interruptible/wait_event_interruptible to synchronize
> kernel thread and my read callback.
> 
> static uint8_t queue[50 * DATA_SIZE];
> static int read_offset = 0;
> static int write_offset = 0;
> 
> I guess that my problems are quite common in kernel driver which want to gather some data over spi.
> Is there a good kernel driver that I can take as good practice/example to implement my solution ?
> Feel free to say that my code is stupid !
> 

Hi Arthur,

from my point of view your device driver can use IIO (Industrial IO)
subsystem. There are a lot of supported ADCs there.
If your ADC is not supported yet, you have great opportunity to submit
new driver to the mainline kernel.

--
Best regards,
Andrey Skvortsov