Message-ID: <e29eaccc-863a-21d4-e669-0b708604d723@redhat.com>
Date:   Wed, 9 Feb 2022 16:54:01 +0100
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Noralf Trønnes <noralf@...nnes.org>,
        Maxime Ripard <maxime@...no.tech>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        linux-fbdev@...r.kernel.org,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Sam Ravnborg <sam@...nborg.org>,
        dri-devel@...ts.freedesktop.org, Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Lee Jones <lee.jones@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Mark Brown <broonie@...nel.org>,
        Maxime Ripard <mripard@...nel.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
        linux-pwm@...r.kernel.org
Subject: Re: [PATCH v3 3/7] drm: Add driver for Solomon SSD130X OLED displays

Hello Andy,

Thanks for your feedback.

On 2/9/22 16:12, Andy Shevchenko wrote:
> 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' ?
>

The Kconfig was not sorted alphabetically so I just added it at
the end. Same for the Makefile. But I will change it in v4 just
to not keep adding unsorted entries.

[snip]

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

Ok.
 
>> + * Copyright 2022 Red Hat Inc.
>> + * Authors: Javier Martinez Canillas <javierm@...hat.com>
>> + *
>> + * 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.
>

I like to have all this at the beginning, it makes easier to read IMO.

[snip]

>> +
>> +	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'm using bulk writes for the data but not for the commands. Because I
tried to do that before and didn't work. But I'll give it a try again
now that I switched to regmap. Maybe it was some mistake on my end.
 
> I have noticed that all of the callers are using
> - 1 -- makes no sense at all, can be replaced with regmap_write()

Yes, I just used for consistency. That way all the places sending a
command would use the same function call.

> - 2
> - 3
> 
> Can be helpers for two and three arguments, with use of bulk call.
> 
> What do you think?
> 

Agreed, as mentioned I'll give it a try to sending all the data as a
bulk write with regmap.

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

I think is easier to read when doing it in the caller, specially since there
is only a single call. Than calling it unconditionally and making it a no-op
if there isn't a reset GPIO.

>> +		ssd130x_reset(ssd130x);
> 
> ...
> 
>> +	/* Set COM direction */
>> +	com_invdir = 0xc0 | ssd130x->com_invdir << 3;
> 
> Can 0xc0 and 3 be GENMASK()'ed and defined?
>

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

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

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

Ok.
 
> ...
> 
>> +		for (i = 0; i < ARRAY_SIZE(ssd130x->lookup_table); ++i) {
> 
> i++ should work as well.
>

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

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

Ok.
 
> ...
> 
>> +		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;
>
> ?
> 

Mark mentioned that the regulator shouldn't really be optional.
So half of this part is going away.

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

No, because this isn't a resource provided by other driver. If this
failed is mostly due a memory allocation error.
 
>> +	}
> 
> ...
> 
>> +	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.
>

Same. This is an error and not a reason to defer the probe.

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

And same.
 
>> +	}
> 
> ...
> 
> I have feelings that half of my comments were ignored...
> Maybe I missed the discussion(s).
> 

I assure you that no comments from you or anyone were ignored.

I may had missed something but if if I did was a mistake and
not intentionally. I keep a changelog for each revision in
the patches with the name of the reviewer so people can check
and compare.

If something that you mentioned is not there, I apologize and
please point me out so I can address it in v4.

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

Powered by blists - more mailing lists