On Wed, Feb 09, 2022 at 10:03:10AM +0100, Javier Martinez Canillas wrote:

This adds a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon
OLED display controllers.

It's only the core part of the driver and a bus specific driver is needed
for each transport interface supported by the display controllers.

Thank you for the update, my comments below.

...

 source "drivers/gpu/drm/sprd/Kconfig"
 
+source "drivers/gpu/drm/solomon/Kconfig"

'o' before 'p' ?

...

 obj-$(CONFIG_DRM_SPRD) += sprd/
+obj-y			+= solomon/

Ditto ?

...

+/*
+ * DRM driver for Solomon SSD130X OLED displays

Solomon SSD130x (with lower letter it's easy to read and realize that it's
not a model name).

+ * Copyright 2022 Red Hat Inc.
+ * Authors: Javier Martinez Canillas [off-list ref]
+ *
+ * Based on drivers/video/fbdev/ssd1307fb.c
+ * Copyright 2012 Free Electrons
+ */

+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/property.h>
+#include <linux/pwm.h>
+#include <linux/regulator/consumer.h>

...

+#define DRIVER_NAME	"ssd130x"
+#define DRIVER_DESC	"DRM driver for Solomon SSD130X OLED displays"
+#define DRIVER_DATE	"20220131"
+#define DRIVER_MAJOR	1
+#define DRIVER_MINOR	0

Not sure it has a value when being defined. Only one string is reused and even
if hard coded twice linker will optimize it.

...

+/*
+ * Helper to write command (SSD130X_COMMAND). The fist variadic argument
+ * is the command to write and the following are the command options.
+ */
+static int ssd130x_write_cmd(struct ssd130x_device *ssd130x, int count,
+				    /* u8 cmd, u8 option, ... */...)
+{
+	va_list ap;
+	u8 value;
+	int ret;
+
+	va_start(ap, count);
+
+	do {
+		value = va_arg(ap, int);
+		ret = regmap_write(ssd130x->regmap, SSD130X_COMMAND, (u8)value);
+		if (ret)
+			goto out_end;
+	} while (--count);
+
+out_end:
+	va_end(ap);
+
+	return ret;

Can bulk operation be used in the callers instead?

I have noticed that all of the callers are using
- 1 -- makes no sense at all, can be replaced with regmap_write()
- 2
- 3

Can be helpers for two and three arguments, with use of bulk call.

What do you think?

+}

...

+static void ssd130x_reset(struct ssd130x_device *ssd130x)
+{
+	/* Reset the screen */
+	gpiod_set_value_cansleep(ssd130x->reset, 1);
+	udelay(4);
+	gpiod_set_value_cansleep(ssd130x->reset, 0);
+	udelay(4);

I don't remember if reset pin is mandatory. fbtft does

	if (!gpiod->reset)
		return;

	...do reset...

+}

...

+	if (ssd130x->reset)

A-ha, why not in the callee?

+		ssd130x_reset(ssd130x);

...

+	/* Set COM direction */
+	com_invdir = 0xc0 | ssd130x->com_invdir << 3;

Can 0xc0 and 3 be GENMASK()'ed and defined?

...

+	/* Set clock frequency */
+	dclk = ((ssd130x->dclk_div - 1) & 0xf) | (ssd130x->dclk_frq & 0xf) << 4;

GENMASK() ?

...

+		u32 mode = ((ssd130x->area_color_enable ? 0x30 : 0) |
+			    (ssd130x->low_power ? 5 : 0));

With if's it will look better.

		u32 mode = 0;

		if (ssd130x->area_color_enable)
			mode |= 0x30;
		if (ssd130x->low_power)
			mode |= 5;

...

+	/* Turn on the DC-DC Charge Pump */
+	chargepump = BIT(4) | (ssd130x->device_info->need_chargepump ? BIT(2) : 0);

Ditto.

...

+		for (i = 0; i < ARRAY_SIZE(ssd130x->lookup_table); ++i) {

i++ should work as well.

+			u8 val = ssd130x->lookup_table[i];
+
+			if (val < 31 || val > 63)
+				dev_warn(ssd130x->dev,
+					 "lookup table index %d value out of range 31 <= %d <= 63\n",
+					 i, val);
+			ret = ssd130x_write_cmd(ssd130x, 1, val);
+			if (ret < 0)
+				return ret;
+		}

...

+	u8 *buf = NULL;

+

Redundant blank line, not sure if checkpatch catches this.

+	struct drm_rect fullscreen = {
+		.x1 = 0,
+		.x2 = ssd130x->width,
+		.y1 = 0,
+		.y2 = ssd130x->height,
+	};

...

+power_off:

out_power_off: ?

...

+		ret = PTR_ERR(ssd130x->vbat_reg);
+		if (ret == -ENODEV)
+			ssd130x->vbat_reg = NULL;
+		else
+			return dev_err_probe(dev, ret, "Failed to get VBAT regulator\n");

Can it be

		ret = PTR_ERR(ssd130x->vbat_reg);
		if (ret != -ENODEV)
			return dev_err_probe(dev, ret, "Failed to get VBAT regulator\n");

		ssd130x->vbat_reg = NULL;

?

...

+	ssd130x = devm_drm_dev_alloc(dev, &ssd130x_drm_driver,
+				     struct ssd130x_device, drm);
+	if (IS_ERR(ssd130x)) {

+		dev_err(dev, "Failed to allocate DRM device: %d\n", ret);
+		return ssd130x;

return dev_err_probe() ?

+	}

...

+	bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
+					    &ssd130xfb_bl_ops, NULL);
+	if (IS_ERR(bl)) {
+		ret = PTR_ERR(bl);
+		dev_err(dev, "Unable to register backlight device: %d\n", ret);
+		return ERR_PTR(ret);

Ditto.

+	}

...

+	ret = drm_dev_register(drm, 0);
+	if (ret) {
+		dev_err(dev, "DRM device register failed: %d\n", ret);
+		return ERR_PTR(ret);

Ditto.

+	}

...

I have feelings that half of my comments were ignored...
Maybe I missed the discussion(s).


-- 
With Best Regards,
Andy Shevchenko