Hello Andy,

Thanks for your feedback.

On 2/4/22 15:26, Andy Shevchenko wrote:

On Fri, Feb 04, 2022 at 02:43:45PM +0100, Javier Martinez Canillas wrote:

quoted

Add a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon OLED
controllers that can be programmed via an I2C interface. This is a port
of the ssd1307fb driver that already supports these devices.

A Device Tree binding is not added because the DRM driver is compatible
with the existing binding for the ssd1307fb driver.

...

quoted

+/*
+ * DRM driver for Solomon SSD130X OLED displays
+ *
+ * Copyright 2022 Red Hat Inc.
+ *
+ * Based on drivers/video/fbdev/ssd1307fb.c
+ * Copyright 2012 Free Electrons

quoted

+ *

No need for this blank line.

Ok.
 

quoted

+ */

...

quoted

+struct ssd130x_device {
+	struct drm_device drm;
+	struct drm_simple_display_pipe pipe;
+	struct drm_display_mode mode;
+	struct drm_connector connector;

quoted

+	struct i2c_client *client;

Can we logically separate hw protocol vs hw interface from day 1, please?
This will allow to add SPI support for this panel much easier.

Technically I would like to see here

	struct device *dev;

and probably (I haven't looked into design)

	struct ssd130x_ops *ops;

or something alike.

Sure. I wanted to keep the driver simple, making the writes bus agnostic and
adding a level of indirection would make it more complex. But I agree that
it will also make easier to add more buses later. I will do that for v3.

[snip]

quoted

+static inline int ssd130x_write_value(struct i2c_client *client, u8 value)

Not sure inline does anything useful here.
Ditto for the rest similar cases.

Ok, I'll drop them.
 

...

quoted

+static inline int ssd130x_write_cmd(struct i2c_client *client, int count,
+				    /* u8 cmd, u8 value, ... */...)
+{
+	va_list ap;
+	u8 value;
+	int ret;
+
+	va_start(ap, count);

quoted

+	while (count--) {
+		value = va_arg(ap, int);
+		ret = ssd130x_write_value(client, (u8)value);
+		if (ret)
+			goto out_end;
+	}

I'm wondering if this can be written in a form

	do {
		...
	} while (--count);

In this case it will give a hint that count can't be 0.

Sure, I don't have a strong preference. I will change it.

[snip]
 

quoted

+	ssd130x->pwm = pwm_get(dev, NULL);
+	if (IS_ERR(ssd130x->pwm)) {
+		dev_err(dev, "Could not get PWM from device tree!\n");

"device tree" is a bit confusing here if I run this on ACPI.
Maybe something like "firmware description"?

Indeed.
 

quoted

+		return PTR_ERR(ssd130x->pwm);
+	}

...

quoted

+	/* Set initial contrast */
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_CONTRAST, ssd130x->contrast);

Creating a local variable for client allows to:
- make lines shorter and might even be less LOCs
- allow to convert struct device to client in one place
  (as per my above comment)

Ditto for other similar cases.

Ok.
 
[snip]

quoted

+	/* Switch to horizontal addressing mode */
+	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_ADDRESS_MODE,
+				SSD130X_SET_ADDRESS_MODE_HORIZONTAL);
+	if (ret < 0)
+		return ret;
+
+	return 0;

Can it be

	return ssd130x_write_cmd(...);

?

...

Yes.

quoted

+	unsigned int line_length = DIV_ROUND_UP(width, 8);
+	unsigned int pages = DIV_ROUND_UP(height, 8);

For power of two there are more efficient roundup()/rounddown()
(or with _ in the names, I don't remember by heart).

Oh, I didn't know about round_{up,down}(). Thanks a lot for the pointer.

...

quoted

+			for (k = 0; k < m; k++) {

quoted

+				u8 byte = buf[(8 * i + k) * line_length +
+					       j / 8];

One line?

Yes.

quoted

+				u8 bit = (byte >> (j % 8)) & 1;
+
+				data |= bit << k;
+			}

...

quoted

+static int ssd130x_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
+					   const struct drm_display_mode *mode)
+{
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
+
+	if (mode->hdisplay != ssd130x->mode.hdisplay &&
+	    mode->vdisplay != ssd130x->mode.vdisplay)
+		return MODE_ONE_SIZE;

quoted

+	else if (mode->hdisplay != ssd130x->mode.hdisplay)
+		return MODE_ONE_WIDTH;
+	else if (mode->vdisplay != ssd130x->mode.vdisplay)
+		return MODE_ONE_HEIGHT;

'else' in both cases is redundant.

Indeed.
 

quoted

+	return MODE_OK;
+}

...

quoted

+poweroff:

out_power_off: ?

Ok.
 

...

quoted

+	if (!fb)
+		return;

Can it happen?

I don't know, but saw that the handler of other drivers checked for this so
preferred to play safe and do the same.
 

...

quoted

+	drm_mode_probed_add(connector, mode);
+	drm_set_preferred_mode(connector, mode->hdisplay, mode->vdisplay);
+
+	return 1;

Positive code, what is the meaning of it?

It's the number of connector modes. The driver only supports 1.
 

...

quoted

+	if (device_property_read_u32(dev, "solomon,prechargep2", &ssd130x->prechargep2))
+		ssd130x->prechargep2 = 2;

You can drop conditionals for the optional properties

	ssd130x->prechargep2 = 2;
	device_property_read_u32(dev, "solomon,prechargep2", &ssd130x->prechargep2);

and so on for the similar.

Ok.
 

...

quoted

+	ssd130x->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(ssd130x->reset)) {

quoted

+		ret = PTR_ERR(ssd130x->reset);
+		dev_err(dev, "Failed to get reset gpio: %d\n", ret);
+		return ret;

Why not

	return dev_err_probe()?

Each time you call it for deferred probe, it will spam logs.

Right. I'll change in all the places you pointed out.

[snip] 

...

quoted

+	{},

Comma is not needed in terminator entry.

Right.
 

...

quoted

+static struct i2c_driver ssd130x_i2c_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = ssd130x_of_match,
+	},
+	.probe_new = ssd130x_probe,
+	.remove = ssd130x_remove,
+	.shutdown = ssd130x_shutdown,
+};

quoted

+

Redundant blank line.

Ok.
 

quoted

+module_i2c_driver(ssd130x_i2c_driver);

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat