Re: [PATCH v3 3/7] drm: Add driver for Solomon SSD130X OLED displays

[Andy Shevchenko]

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 <javierm@xxxxxxxxxx>
> + *
> + * 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