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

On Wed, Feb 09, 2022 at 04:54:01PM +0100, Javier Martinez Canillas wrote:

On 2/9/22 16:12, Andy Shevchenko wrote:

quoted

On Wed, Feb 09, 2022 at 10:03:10AM +0100, Javier Martinez Canillas wrote:

...

quoted

quoted

+	do {
+		value = va_arg(ap, int);
+		ret = regmap_write(ssd130x->regmap, SSD130X_COMMAND, (u8)value);
+		if (ret)
+			goto out_end;
+	} while (--count);

quoted

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.
 

quoted

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.

quoted

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

Ah, it might be that it should be noinc bulk op. Need to be checked anyway.

...

quoted

quoted

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

quoted

+}

...

quoted

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

It doesn't matter where the check is and checking that in the callee seems
better as it relies on the reset functionality. Caller in such case shouldn't
even think if it's supported or not, not their business.

Last, but not least, if you think of power management, you probably want to
assert reset there as well, means additional checks?

quoted

quoted

+		ssd130x_reset(ssd130x);

...

quoted

quoted

+	if (IS_ERR(ssd130x)) {

quoted

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

Is it a problem? dev_err_probe() got update in the documentation explaining
that's fine to call even in such cases. The outcome is less amount of LOCs.

quoted

quoted

+	}

...

quoted

quoted

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

Ditto.

quoted

quoted

+	}

...

quoted

quoted

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

Ditto.

quoted

quoted

+	}

...

quoted

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.

It's just a feeling, because I repeating that dev_err_probe() a lot :-)
Nevertheless, now I see at least your point why you went that way.
But see my comments on it.

-- 
With Best Regards,
Andy Shevchenko