Callback in Linux kernel driver in order to hide device's low-level protocol

Question

I'm am writing a Linux kernel driver for HD44780 LCDs connected via I2C bus. In my last change I tried to decouple low-level code (which talks to the device via I2C) from device's logic (printing characters, parsing special characters, managing screen geometry etc.). I tried to solve it via a callback and it seems to work:

LCD data:

#define BUF_SIZE    64

struct hd44780_geometry {
    int cols;
    int rows;
    int start_addrs[];
};

struct hd44780 {
    struct cdev cdev;
    struct device *device;
    struct i2c_client *i2c_client;
    struct hd44780_geometry *geometry;
    int addr;
    char buf[BUF_SIZE];

    struct {
        void (*f)(struct hd44780 *, int data);
        void *arg;
    } raw_callback;

    struct mutex lock;
    struct list_head list;
};

extern struct hd44780_geometry hd44780_geometry_20x4;

void hd44780_write(struct hd44780 *, char *, size_t);
void hd44780_init_lcd(struct hd44780 *);
void hd44780_print(struct hd44780 *, char *);

Initialization and callback code:

static void pcf8574_raw_write(struct hd44780 *lcd, int data)
{
    i2c_smbus_write_byte(lcd->raw_callback.arg, data);
}

static void hd44780_write_nibble(struct hd44780 *lcd, int data)
{
    pcf8574_raw_write(lcd, data);
    /* Theoretically wait for tAS = 40ns, practically it's already elapsed */

    pcf8574_raw_write(lcd, data | E);
    /* Again, "wait" for pwEH = 230ns */

    pcf8574_raw_write(lcd, data);
    /* And again, "wait" for about tCYC_E - pwEH = 270ns */
}

static int hd44780_file_open(struct inode *inode, struct file *filp)
{
    filp->private_data = container_of(inode->i_cdev, struct hd44780, cdev);
    return 0;
}

static int hd44780_file_release(struct inode *inode, struct file *filp)
{
    return 0;
}

static ssize_t hd44780_file_write(struct file *filp, const char __user *buf, size_t count, loff_t *offp)
{
    struct hd44780 *lcd;
    size_t n;

    lcd = filp->private_data;
    n = min(count, (size_t)BUF_SIZE);

    // TODO: Consider using an interruptible lock
    mutex_lock(&lcd->lock);

    // TODO: Support partial writes during errors?
    if (copy_from_user(lcd->buf, buf, n))
        return -EFAULT;

    hd44780_write(lcd, lcd->buf, n);

    mutex_unlock(&lcd->lock);

    return n;
}

static void hd44780_init(struct hd44780 *lcd, struct hd44780_geometry *geometry,
        struct i2c_client *i2c_client)
{
    lcd->geometry = geometry;
    lcd->i2c_client = i2c_client;
    lcd->raw_callback.f = hd44780_write_nibble;
    lcd->raw_callback.arg = i2c_client;
    lcd->addr = 0x00;
    mutex_init(&lcd->lock); 
}

static struct file_operations fops = {
    .open = hd44780_file_open,
    .release = hd44780_file_release,
    .write = hd44780_file_write,
};

static int hd44780_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
    dev_t devt;
    struct hd44780 *lcd;
    struct device *device;
    int ret, minor;

    minor = atomic_inc_return(&next_minor);
    devt = MKDEV(MAJOR(dev_no), minor);

    lcd = (struct hd44780 *)kmalloc(sizeof(struct hd44780), GFP_KERNEL);
    if (!lcd) {
        return -ENOMEM;
    }

    hd44780_init(lcd, &hd44780_geometry_20x4, client);

    spin_lock(&hd44780_list_lock);
    list_add(&lcd->list, &hd44780_list);
    spin_unlock(&hd44780_list_lock);

    cdev_init(&lcd->cdev, &fops);
    ret = cdev_add(&lcd->cdev, devt, 1);
    if (ret) {
        pr_warn("Can't add cdev\n");
        goto exit;
    }

    device = device_create(hd44780_class, NULL, devt, NULL, "lcd%d", MINOR(devt));
    if (IS_ERR(device)) {
        ret = PTR_ERR(device);
        pr_warn("Can't create device\n");
        goto del_exit;
    }
    lcd->device = device;

    hd44780_init_lcd(lcd);
    hd44780_print(lcd, "Hello, world!");

    return 0;

del_exit:
    cdev_del(&lcd->cdev);

    spin_lock(&hd44780_list_lock);
    list_del(&lcd->list);
    spin_unlock(&hd44780_list_lock);
exit:
    kfree(lcd);

    return ret;
}

And a piece of code that shows how the callback is called:

static void hd44780_write_nibble(struct hd44780 *lcd, int data) {
    lcd->raw_callback.f(lcd, data);
}

static void hd44780_write_command(struct hd44780 *lcd, int data)
{
    int h = (data >> 4) & 0x0F;
    int l = data & 0x0F;
    int cmd_h, cmd_l;

    cmd_h = (h << 4) | (RS & 0x00) | (RW & 0x00) | BL;
    hd44780_write_nibble(lcd, cmd_h);

    cmd_l = (l << 4) | (RS & 0x00) | (RW & 0x00) | BL;
    hd44780_write_nibble(lcd, cmd_l);

    udelay(37);
}

The crucial thing here is to pass the i2c_client to the hd44780 struct so that the callback can use it later.

Is there a more pretty/recommended way to solve this problem?

You can find full source code here. This was originally asked here.

Answer 1

Pass arg instead of lcd

Right now, you are passing lcd to your callback function, but the callback function doesn't need lcd. What it needs is the callback argument that you stored in lcd->raw_callback.arg. I think you should pass that argument directly to the callback function, like this:

static void hd44780_write_nibble(struct hd44780 *lcd, int data)
{
    lcd->raw_callback.f(lcd->raw_callback.arg, data);
}

The callback function would look like this:

static void hd44780_write_nibble(void *arg, int data)
{
    pcf8574_raw_write(arg, data);
    /* Theoretically wait for tAS = 40ns, practically it's already elapsed */

    pcf8574_raw_write(arg, data | E);
    /* Again, "wait" for pwEH = 230ns */

    pcf8574_raw_write(arg, data);
    /* And again, "wait" for about tCYC_E - pwEH = 270ns */
}

And pcf8574_raw_write() would look like:

static void pcf8574_raw_write(void *arg, int data)
{
    i2c_smbus_write_byte(arg, data);
}

Naming

I would rename raw_callback to write_nibble_callback. I don't know what to expect when I call raw_callback.f().

It's strange that you have two static functions both called hd44780_write_nibble(). I realize that they are on two conceptually different layers of your program, but if I were debugging your program I would get confused as to which function I was in.