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

[Andy Shevchenko]

On Wed, Feb 09, 2022 at 04:54:01PM +0100, Javier Martinez Canillas wrote:
> On 2/9/22 16:12, Andy Shevchenko wrote:
> > On Wed, Feb 09, 2022 at 10:03:10AM +0100, Javier Martinez Canillas wrote:

...

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

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

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

...

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

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?

> >> +		ssd130x_reset(ssd130x);

...

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

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.

> >> +	}

...

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

> >> +	}

...

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

> >> +	}

...

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