On Tue, Jan 13, 2015 at 05:16:42PM -0800, Felipe F. Tonello wrote:

This driver will basically translate serial communication to i2c communication
between the user-space and the GPS module.

It creates a /dev/ttyS device node.

There are specific tty termios flags in order to the tty line discipliner not
to change the date it is pushed to user space.

I don't understand this sentance, what date?  What is pushed where?
What termios files?  What is a "discipliner"?

I don't think there is a "serial tty GPS device" here, right?

+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
+#include <linux/i2c.h>
+#include <linux/workqueue.h>
+
+/*
+ * Version Information
+ */
+#define DRIVER_VERSION "v0.1"
+#define DRIVER_DESC "u-blox 6 I2C GPS driver"
+
+#define UBLOX_GPS_MAJOR 0
+#define UBLOX_GPS_NUM 1 /* Only support 1 GPS at a time */
+
+/* By default u-blox GPS fill its buffer every 1 second (1000 msecs) */
+#define READ_TIME 1000
+
+static struct tty_port *ublox_gps_tty_port;
+static struct i2c_client *ublox_gps_i2c_client;

Why only one device/client?  Why can't you have more than one in the
system?  Please don't make this work for only one device, these can all
be device-private.

+static int ublox_gps_is_open;

Why do you need this?  And again, don't make it global for the whole
driver, but unique per-device

+static struct file *ublox_gps_filp;

I don't understand why you even need this, what problem is this solving?

+static void ublox_gps_read_worker(struct work_struct *private);
+
+static DECLARE_DELAYED_WORK(ublox_gps_wq, ublox_gps_read_worker);

Again, make device-specific.

+static void ublox_gps_read_worker(struct work_struct *private)
+{
+	s32 gps_buf_size, buf_size = 0;
+	u8 *buf;
+
+	if (!ublox_gps_is_open)
+		return;

How can this happen?

+
+	/* check if driver was removed */
+	if (!ublox_gps_i2c_client)
+		return;

How can this happen?

+
+	gps_buf_size = i2c_smbus_read_word_data(ublox_gps_i2c_client, 0xfd);
+	if (gps_buf_size < 0) {
+		dev_warn(&ublox_gps_i2c_client->dev, KBUILD_MODNAME ": couldn't read register(0xfd) from GPS.\n");

Something to be fixed for all of your dev_*() calls, don't put
KBUILD_MODNAME, it's not needed as dev_* handles everything properly to
point to the specific driver and device that created the message.

+		/* try one more time */
+		goto end;
+	}
+
+	/* 0xfd is the MSB and 0xfe is the LSB */
+	gps_buf_size = ((gps_buf_size & 0xf) << 8) | ((gps_buf_size & 0xf0) >> 8);

I don't understand the comment here.

+
+	if (gps_buf_size > 0) {
+
+		buf = kcalloc(gps_buf_size, sizeof(*buf), GFP_KERNEL);
+		if (!buf) {
+			dev_warn(&ublox_gps_i2c_client->dev, KBUILD_MODNAME ": couldn't allocate memory.\n");

No need to send any error if memory is gone, that already happened in
the syslog.

+			/* try one more time */
+			goto end;
+		}
+
+		do {
+			buf_size = i2c_master_recv(ublox_gps_i2c_client, (char *)buf, gps_buf_size);
+			if (buf_size < 0) {
+				dev_warn(&ublox_gps_i2c_client->dev, KBUILD_MODNAME ": couldn't read data from GPS.\n");
+				kfree(buf);
+				/* try one more time */
+				goto end;
+			}
+
+			tty_insert_flip_string(ublox_gps_tty_port, buf, buf_size);
+
+			gps_buf_size -= buf_size;
+
+			/* There is a small chance that we need to split the data over
+			   several buffers. If this is the case we must loop */
+		} while (unlikely(gps_buf_size > 0));
+
+		tty_flip_buffer_push(ublox_gps_tty_port);
+
+		kfree(buf);
+	}
+
+end:
+	/* resubmit the workqueue again */
+	schedule_delayed_work(&ublox_gps_wq, msecs_to_jiffies(READ_TIME)); /* 1 sec delay */
+}
+
+static int ublox_gps_serial_open(struct tty_struct *tty, struct file *filp)
+{
+	if (ublox_gps_is_open)
+		return -EBUSY;
+
+	ublox_gps_filp = filp;
+	ublox_gps_tty_port = tty->port;
+	ublox_gps_tty_port->low_latency = true; /* make sure we push data immediately */
+	ublox_gps_is_open = true;
+
+	schedule_delayed_work(&ublox_gps_wq, 0);
+
+	return 0;
+}
+
+static void ublox_gps_serial_close(struct tty_struct *tty, struct file *filp)
+{
+	if (!ublox_gps_is_open)

How can this happen?

+		return;
+
+	/* avoid stop when the denied (in open) file structure closes itself */
+	if (ublox_gps_filp != filp)
+		return;

I don't understand, how can something "close itself"?

+
+	ublox_gps_is_open = false;
+	ublox_gps_filp = NULL;
+	ublox_gps_tty_port = NULL;
+}
+
+static int ublox_gps_serial_write(struct tty_struct *tty, const unsigned char *buf,
+	int count)
+{
+	if (!ublox_gps_is_open)
+		return 0;
+
+	/* check if driver was removed */
+	if (!ublox_gps_i2c_client)
+		return 0;
+
+	/* we don't write back to the GPS so just return same value here */
+	return count;
+}

So write does nothing, why even have it at all?

+static int ublox_gps_write_room(struct tty_struct *tty)
+{
+	if (!ublox_gps_is_open)
+		return 0;
+
+	/* check if driver was removed */
+	if (!ublox_gps_i2c_client)
+		return 0;
+
+	/* we don't write back to the GPS so just return some value here */
+	return 1024;

Why have this function at all if it does nothing?

+}
+
+static const struct tty_operations ublox_gps_serial_ops = {
+	.open = ublox_gps_serial_open,
+	.close = ublox_gps_serial_close,
+	.write = ublox_gps_serial_write,
+	.write_room = ublox_gps_write_room,
+};
+
+static struct tty_driver *ublox_gps_tty_driver;
+
+static int ublox_gps_probe(struct i2c_client *client,
+	const struct i2c_device_id *id)
+{
+	int result = 0;
+
+	ublox_gps_tty_driver = alloc_tty_driver(UBLOX_GPS_NUM);
+	if (!ublox_gps_tty_driver)
+		return -ENOMEM;
+
+	ublox_gps_tty_driver->owner = THIS_MODULE;
+	ublox_gps_tty_driver->driver_name = "ublox_gps";
+	ublox_gps_tty_driver->name = "ttyS";
+	ublox_gps_tty_driver->major = UBLOX_GPS_MAJOR;
+	ublox_gps_tty_driver->minor_start = 0;
+	ublox_gps_tty_driver->type = TTY_DRIVER_TYPE_SERIAL;
+	ublox_gps_tty_driver->subtype = SERIAL_TYPE_NORMAL;
+	ublox_gps_tty_driver->flags = TTY_DRIVER_REAL_RAW;
+	ublox_gps_tty_driver->init_termios = tty_std_termios;
+	ublox_gps_tty_driver->init_termios.c_iflag = IGNCR | IXON;
+	ublox_gps_tty_driver->init_termios.c_oflag = OPOST;
+	ublox_gps_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD |
+		HUPCL | CLOCAL;
+	ublox_gps_tty_driver->init_termios.c_ispeed = 9600;
+	ublox_gps_tty_driver->init_termios.c_ospeed = 9600;
+	tty_set_operations(ublox_gps_tty_driver, &ublox_gps_serial_ops);
+	result = tty_register_driver(ublox_gps_tty_driver);
+	if (result) {
+		dev_err(&ublox_gps_i2c_client->dev, KBUILD_MODNAME ": %s - tty_register_driver failed\n",
+			__func__);
+		goto err;
+	}
+
+	ublox_gps_i2c_client = client;
+	ublox_gps_filp = NULL;
+	ublox_gps_tty_port = NULL;
+	ublox_gps_is_open = false;
+
+	/* i2c_set_clientdata(client, NULL); */
+
+	dev_info(&ublox_gps_i2c_client->dev, KBUILD_MODNAME ": " DRIVER_VERSION ": "
+		DRIVER_DESC "\n");

Be quiet for "normal" operations please, your driver should never send
anything to the kernel log unless some error happens.

thanks,

greg k-h