Re: [PATCH v4 3/6] drm: Add driver for Solomon SSD130x OLED displays

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Feb 11, 2022 at 01:05:57PM +0100, Javier Martinez Canillas wrote:
> On 2/11/22 12:33, Andy Shevchenko wrote:
> > On Fri, Feb 11, 2022 at 10:19:24AM +0100, Javier Martinez Canillas wrote:

...

> >> + * Helper to write command (SSD130X_COMMAND). The fist variadic argument
> >> + * is the command to write and the following are the command options.
> > 
> > This is not correct explanation. Please, rephrase to show that _each_ of the
> > options is sent with a preceding command.
> >
> 
> It's a correct explanation IMO from the caller point of view. The first argument
> is the command sent (i.e: SSD130X_SET_ADDRESS_MODE) and the next ones are the
> the command options (i.e: SSD130X_SET_ADDRESS_MODE_HORIZONTAL).
> 
> The fact that each command and options are preceding with a SSD130X_COMMAND
> value is part of the protocol of the device and a detail that's abstracted
> away by this helper function to the callers.

My previous suggestion about bulk transaction was purely based on this
(misinterpreted) description. Can we make sure somehow that another reader
don't trap into the same?

...


> >> +	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_probe(dev, ret, "Unable to register backlight device\n");
> >> +		return ERR_PTR(ret);
> > 
> > 		dev_err_probe(dev, PTR_ERR(bl), "Unable to register backlight device\n");
> > 		return bl;
> > 
> > ?
> 
> No, because this function's return value is a struct ssd130x_device pointer,
> not a struct backlight_device pointer.

	return ERR_CAST(bl);

-- 
With Best Regards,
Andy Shevchenko