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 <javi...@redhat.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
