Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 11 Feb 2022 17:49:07 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Javier Martinez Canillas <javierm@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Maxime Ripard <maxime@cerno.tech>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        dri-devel@lists.freedesktop.org,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Sam Ravnborg <sam@ravnborg.org>,
        Noralf =?iso-8859-1?Q?Tr=F8nnes?= <noralf@tronnes.org>,
        Daniel Vetter <daniel@ffwll.ch>,
        David Airlie <airlied@linux.ie>,
        Lee Jones <lee.jones@linaro.org>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thierry Reding <thierry.reding@gmail.com>,
        Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= 
        <u.kleine-koenig@pengutronix.de>, linux-pwm@vger.kernel.org
Subject: Re: [PATCH v4 3/6] drm: Add driver for Solomon SSD130x OLED displays
Message-ID: <YgaFc++EP3/hv+iA@smile.fi.intel.com>
References: <20220211091927.2988283-1-javierm@redhat.com>
 <20220211091927.2988283-4-javierm@redhat.com>
 <YgZJpi4llqr93U9Y@smile.fi.intel.com>
 <eed8200c-7716-ce4f-dac5-bd6f0345b631@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <eed8200c-7716-ce4f-dac5-bd6f0345b631@redhat.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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