Re: [PATCH v1 1/2] drm/i2c: tda998x: access chip registers via a regmap

[Russell King - ARM Linux admin]

On Fri, Jun 21, 2019 at 05:13:33PM -0400, Sven Van Asbroeck wrote:
> On Fri, Jun 21, 2019 at 11:15 AM Russell King - ARM Linux admin
> <linux@xxxxxxxxxxxxxxx> wrote:
> >
> > Another con is the need to keep the functions that detail the register
> > properties up to date, which if they're wrong may result in unexpected
> > behaviour.
> >
> > I subscribe to the "keep it simple" approach, and regmap, although
> > useful, seems like a giant sledgehammer for this.
> >
> 
> Thank you for the review !
> 
> I added this back when I was debugging audio artifacts related to this
> chip. The regmap's debugfs binding was extremely useful. So I
> dressed it up a bit in the hope that it would have some general use.
> 
> But if the cons outweigh the pros, then this is as far as this patch
> will go...

I won't disagree that debugfs access to the registers is useful,
especially as I keep this patch locally for that exact purpose:

8<===
From: Russell King <rmk+kernel@xxxxxxxxxxxxxxx>
Subject: [PATCH] drm/i2c: tda998x: debugfs register access

Add support for dumping the register contents via debugfs, with a
mechanism to write to the TDA998x registers to allow experimentation.

Signed-off-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxx>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 114 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 5312fde8b624..6ad1fd1d28a5 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -16,10 +16,13 @@
  */
 
 #include <linux/component.h>
+#include <linux/ctype.h>
+#include <linux/debugfs.h>
 #include <linux/gpio/consumer.h>
 #include <linux/hdmi.h>
 #include <linux/module.h>
 #include <linux/platform_data/tda9950.h>
+#include <linux/seq_file.h>
 #include <linux/irq.h>
 #include <sound/asoundef.h>
 #include <sound/hdmi-codec.h>
@@ -1239,6 +1242,116 @@ static int tda998x_audio_codec_init(struct tda998x_priv *priv,
 }
 
 /* DRM connector functions */
+static u8 tda998x_debugfs_pages[] = {
+	0x00, 0x01, 0x02, 0x09, 0x10, 0x11, 0x12, 0x13
+};
+
+static void *tda998x_regs_start(struct seq_file *s, loff_t *pos)
+{
+	return *pos < ARRAY_SIZE(tda998x_debugfs_pages) ? pos : NULL;
+}
+
+static void *tda998x_regs_next(struct seq_file *s, void *v, loff_t *pos)
+{
+	(*pos)++;
+	return *pos < ARRAY_SIZE(tda998x_debugfs_pages) ? pos : NULL;
+}
+
+static void tda998x_regs_stop(struct seq_file *s, void *v)
+{
+}
+
+static int tda998x_regs_show(struct seq_file *s, void *v)
+{
+	struct tda998x_priv *priv = s->private;
+	u8 page = tda998x_debugfs_pages[*(loff_t *)v];
+	unsigned int i, j;
+	u8 buf[16];
+
+	seq_printf(s, "==================== page %02x ======================\n", page);
+	seq_printf(s, "     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f\n");
+	for (i = 0; i < 256; i += sizeof(buf)) {
+		reg_read_range(priv, REG(page, i), buf, sizeof(buf));
+
+		seq_printf(s, "%02x:", i);
+		for (j = 0; j < sizeof(buf); j++)
+			seq_printf(s, " %02x", buf[j]);
+		seq_printf(s, " : ");
+		for (j = 0; j < sizeof(buf); j++)
+			seq_printf(s, "%c",
+				   isascii(buf[j]) && isprint(buf[j]) ?
+				   buf[j] : '.');
+		seq_putc(s, '\n');
+	}
+	return 0;
+}
+
+static const struct seq_operations tda998x_regs_sops = {
+	.start	= tda998x_regs_start,
+	.next	= tda998x_regs_next,
+	.stop	= tda998x_regs_stop,
+	.show	= tda998x_regs_show,
+};
+
+static int tda998x_regs_open(struct inode *inode, struct file *file)
+{
+	int ret = seq_open(file, &tda998x_regs_sops);
+	if (ret < 0)
+		return ret;
+
+	((struct seq_file *)file->private_data)->private = inode->i_private;
+
+	return 0;
+}
+
+static ssize_t tda998x_regs_write(struct file *file, const char __user *buf,
+				  size_t count, loff_t *offset)
+{
+	struct tda998x_priv *priv =
+		((struct seq_file *)file->private_data)->private;
+	unsigned int page, addr, mask, val;
+	unsigned char rval;
+	char buffer[16];
+
+	memset(buffer, 0, sizeof(buffer));
+	if (count > sizeof(buffer) - 1)
+		count = sizeof(buffer) - 1;
+	if (copy_from_user(buffer, buf, count))
+		return -EFAULT;
+
+	if (sscanf(buffer, "%x %x %x %x", &page, &addr, &mask, &val) != 4)
+		return -EINVAL;
+	if (page > 0xff || addr > 0xff || mask > 0xff || val > 0xff)
+		return -ERANGE;
+
+	rval = reg_read(priv, REG(page, addr));
+	rval &= ~mask;
+	rval |= val & mask;
+	reg_write(priv, REG(page, addr), rval);
+
+	printk("i2c write %02x @ page:%02x address:%02x\n", rval, page, addr);
+
+	return count;
+}
+
+static const struct file_operations tda998x_regs_fops = {
+	.owner = THIS_MODULE,
+	.open = tda998x_regs_open,
+	.read = seq_read,
+	.write = tda998x_regs_write,
+	.llseek = seq_lseek,
+	.release = seq_release,
+};
+
+static int tda998x_late_register(struct drm_connector *connector)
+{
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+
+	debugfs_create_file("tda998x-regs", 0600, connector->debugfs_entry,
+			    priv, &tda998x_regs_fops);
+
+	return 0;
+}
 
 static enum drm_connector_status
 tda998x_connector_detect(struct drm_connector *connector, bool force)
@@ -1259,6 +1372,7 @@ static const struct drm_connector_funcs tda998x_connector_funcs = {
 	.reset = drm_atomic_helper_connector_reset,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = tda998x_connector_detect,
+	.late_register = tda998x_late_register,
 	.destroy = tda998x_connector_destroy,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-- 
2.7.4


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up