[RFC 1/7] staging: fbtft: add lcd register abstraction

[Dan Carpenter]

On Tue, Mar 03, 2015 at 01:19:52PM +0100, Noralf Trønnes wrote:
> >>+static ssize_t lcdreg_debugfs_read_wr(struct file *file,
> >>+				      const char __user *user_buf,
> >>+				      size_t count, loff_t *ppos)
> >>+{
> >>+	struct lcdreg *reg = file->private_data;
> >>+	struct lcdreg_transfer tr = {
> >>+		.index = (reg->quirks & LCDREG_INDEX0_ON_READ) ? 0 : 1,
> >>+		.width = reg->debugfs_read_width,
> >>+		.count = 1,
> >>+	};
> >>+	u32 txbuf[1];
> >>+	char *buf = NULL;
> >>+	int ret;
> >>+
> >>+	ret = lcdreg_userbuf_to_u32(user_buf, count, txbuf, ARRAY_SIZE(txbuf));
> >>+	if (ret != 1)
> >>+		return -EINVAL;
> >>+
> >>+	tr.buf = kmalloc(lcdreg_bytes_per_word(tr.width), GFP_KERNEL);
> >>+	if (!tr.buf)
> >>+		return -ENOMEM;
> >>+
> >>+	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> >>+
> >>+	lcdreg_lock(reg);
> >>+	ret = lcdreg_read(reg, txbuf[0], &tr);
> >>+	lcdreg_unlock(reg);
> >>+	if (ret)
> >>+		goto error_out;
> >>+
> >>+	if (!reg->debugfs_read_result) {
> >>+		reg->debugfs_read_result = kmalloc(16, GFP_KERNEL);
> >>+		if (!reg->debugfs_read_result) {
> >>+			ret = -ENOMEM;
> >>+			goto error_out;
> >>+		}
> >>+	}
> >Allocating here is strange.  We only free ->debugfs_read_result on
> >error so it's sort of buggy as well.  Also is it racy if we have
> >multiple readers and writers at once?
> I spent some time on figuring out how to do this.
> To read a register, first write the register number to the 'read' file.
> Then get the result by reading the 'read' file.
> So I have to keep the result somewhere, but you're right this leaks
> memory. Maybe I can use devm_kmalloc here.
> And yes this is racy. I'm not sure if this is a problem.
> This is to be used during development or debugging, so
> the user should have this under control.
> 
> But I'm open to other ways of doing this. This is the best I could
> come up with after several iterations on this code.
> My other solutions where worse :-)

We can't do it probe()?  We could add another flag to say if we had
written to it.  I haven't looked at this closely so whatever you decide
is fine.

> >>+#if defined(CONFIG_DYNAMIC_DEBUG)
> >>+
> >>+#define lcdreg_dbg_transfer_buf(transfer)                            \
> >>+do {                                                                 \
> >>+	int groupsize = lcdreg_bytes_per_word(transfer->width);      \
> >>+	size_t len = min_t(size_t, 32, transfer->count * groupsize); \
> >>+								     \
> >>+	print_hex_dump_debug("    buf=", DUMP_PREFIX_NONE, 32,       \
> >>+			groupsize, transfer->buf, len, false);       \
> >>+} while (0)
> >>+
> >>+#elif defined(DEBUG)
> >>+
> >>+#define lcdreg_dbg_transfer_buf(transfer)                            \
> >>+do {                                                                 \
> >>+	int groupsize = lcdreg_bytes_per_word(transfer->width);      \
> >>+	size_t len = min_t(size_t, 32, transfer->count * groupsize); \
> >>+								     \
> >>+	print_hex_dump(KERN_DEBUG, "    buf=", DUMP_PREFIX_NONE, 32, \
> >>+			groupsize, transfer->buf, len, false);       \
> >>+} while (0)
> >I don't understand this.  Why can't we use print_hex_dump_debug() for
> >both?  In other words:
> >
> >#if defined(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
> >...
> >#else
> The reason I do this is to be able to switch on/off debugging
> quickly when developing.
> I have dynamic debug enabled, but it's more cumbersome to use, so I
> define DEBUG
> when I need this output.
> print_hex_dump_debug() doesn't look at DEBUG, it only cares about
> DYNAMIC_DEBUG.
> But, it's not suited for production code, so I guess I have to use a
> script or something
> to easily switch dynamic debug on/off instead.
> 

Fine fine.  Whatever you decide is ok.

regards,
dan carpenter