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

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 in

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.

StackExchange Code Review Q#100364, answer score: 4