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"?
> That is necessary so the NMEA
> data is not corrupted.
> * iflags: added IGNCR: Ignore carriage return on input.
> * oflags: removed ONLCR: (XSI) Map NL to CR-NL on output.
>
> Signed-off-by: Felipe F. Tonello <eu@xxxxxxxxxxxxxxxxx>
> ---
> drivers/tty/serial/Kconfig | 9 ++
> drivers/tty/serial/Makefile | 1 +
> drivers/tty/serial/ublox6-gps-i2c.c | 245 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 255 insertions(+)
> create mode 100644 drivers/tty/serial/ublox6-gps-i2c.c
>
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 26cec64..49913eb 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1552,6 +1552,15 @@ config SERIAL_MEN_Z135
> This driver can also be build as a module. If so, the module will be called
> men_z135_uart.ko
>
> +config SERIAL_UBLOX_GPS
> + tristate "u-blox 6 I2C GPS support"
> + depends on I2C
> + default n
> + help
> + This driver uses i2c to communicate with the u-blox 6 GPS module
> + and has a serial tty device interface to the user-space, making
> + it easy to read/write data from/to the GPS.
> +
> endmenu
>
> config SERIAL_MCTRL_GPIO
> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
> index 0080cc3..cba2b5c 100644
> --- a/drivers/tty/serial/Makefile
> +++ b/drivers/tty/serial/Makefile
> @@ -92,6 +92,7 @@ obj-$(CONFIG_SERIAL_ARC) += arc_uart.o
> obj-$(CONFIG_SERIAL_RP2) += rp2.o
> obj-$(CONFIG_SERIAL_FSL_LPUART) += fsl_lpuart.o
> obj-$(CONFIG_SERIAL_MEN_Z135) += men_z135_uart.o
> +obj-$(CONFIG_SERIAL_UBLOX_GPS) += ublox6-gps-i2c.o
>
> # GPIOLIB helpers for modem control lines
> obj-$(CONFIG_SERIAL_MCTRL_GPIO) += serial_mctrl_gpio.o
> diff --git a/drivers/tty/serial/ublox6-gps-i2c.c b/drivers/tty/serial/ublox6-gps-i2c.c
> new file mode 100644
> index 0000000..16dd1cf
> --- /dev/null
> +++ b/drivers/tty/serial/ublox6-gps-i2c.c
> @@ -0,0 +1,245 @@
> +/* u-blox 6 I2C GPS driver
> + *
> + * Copyright (C) 2015 Felipe F. Tonello <eu@xxxxxxxxxxxxxxxxx>
> + *
> + * Driver that translates a serial tty GPS device to a i2c GPS device
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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
