Re: [PATCH v2 2/4] drm/tiny: Add driver for Solomon SSD130X OLED displays

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Feb 04, 2022 at 02:43:45PM +0100, Javier Martinez Canillas wrote:
> 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.

...

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

> + *

No need for this blank line.

> + */

...

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


> +	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.

> +	const struct ssd130x_deviceinfo *device_info;
> +
> +	unsigned area_color_enable : 1;
> +	unsigned com_invdir : 1;
> +	unsigned com_lrremap : 1;
> +	unsigned com_seq : 1;
> +	unsigned lookup_table_set : 1;
> +	unsigned low_power : 1;
> +	unsigned seg_remap : 1;
> +	u32 com_offset;
> +	u32 contrast;
> +	u32 dclk_div;
> +	u32 dclk_frq;
> +	u32 height;
> +	u8 lookup_table[4];
> +	u32 page_offset;
> +	u32 col_offset;
> +	u32 prechargep1;
> +	u32 prechargep2;
> +
> +	struct backlight_device *bl_dev;
> +	struct pwm_device *pwm;
> +	struct gpio_desc *reset;
> +	struct regulator *vbat_reg;
> +	u32 vcomh;
> +	u32 width;
> +	/* Cached address ranges */
> +	u8 col_start;
> +	u8 col_end;
> +	u8 page_start;
> +	u8 page_end;
> +};

...

> +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.

...

> +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);

> +	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.

> +out_end:
> +	va_end(ap);
> +
> +	return ret;
> +}


...

> +	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"?

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

...

> +	/* 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.

> +	if (ret < 0)
> +		return ret;

...

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

i++ should work same way.

> +		}

...

> +	/* 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(...);

?

...

> +	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).

...

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

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

One line?

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

...

> +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;

> +	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.

> +	return MODE_OK;
> +}

...

> +poweroff:

out_power_off: ?

...

> +	if (!fb)
> +		return;

Can it happen?

...

> +	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?

...

> +	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.

...

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

> +		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.

> +	}
> +
> +	ssd130x->vbat_reg = devm_regulator_get_optional(dev, "vbat");
> +	if (IS_ERR(ssd130x->vbat_reg)) {
> +		ret = PTR_ERR(ssd130x->vbat_reg);
> +		if (ret == -ENODEV) {
> +			ssd130x->vbat_reg = NULL;

> +		} else {
> +			dev_err(dev, "Failed to get VBAT regulator: %d\n", ret);
> +			return ret;
> +		}

Ditto ?

> +	}

...

> +	if (IS_ERR(ssd130x)) {
> +		ret = PTR_ERR(ssd130x);
> +		dev_err(dev, "Failed to allocate DRM device: %d\n", ret);
> +		return ret;
> +	}

Ditto.

...

> +	i2c_set_clientdata(client, ssd130x);

Wondering if you can split i2c part from the core part and perhaps use regmap
to access the device.

...

> +	if (IS_ERR(bl)) {
> +		ret = PTR_ERR(bl);
> +		dev_err(dev, "Unable to register backlight device: %d\n", ret);
> +		return ret;

	return dev_err_probe();

> +	}

...

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

Ditto.

...

> +	{},

Comma is not needed in terminator entry.

...

> +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,
> +};

> +

Redundant blank line.

> +module_i2c_driver(ssd130x_i2c_driver);

-- 
With Best Regards,
Andy Shevchenko