linux-kernel - Re: [PATCH v5 3/6] drm: Add driver for Solomon SSD130x OLED displays

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 11 Feb 2022 18:13:12 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Javier Martinez Canillas <javierm@...hat.com>
Cc:     linux-kernel@...r.kernel.org,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        linux-fbdev@...r.kernel.org, Sam Ravnborg <sam@...nborg.org>,
        Maxime Ripard <maxime@...no.tech>,
        dri-devel@...ts.freedesktop.org,
        Noralf Trønnes <noralf@...nnes.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Lee Jones <lee.jones@...aro.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v5 3/6] drm: Add driver for Solomon SSD130x OLED displays

On Fri, Feb 11, 2022 at 03:33:55PM +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.

...

> +#define SSD130X_SET_CLOCK_DIV_MASK		GENMASK(3, 0)
> +#define SSD130X_SET_CLOCK_DIV_SET(val)		FIELD_PREP(SSD130X_SET_CLOCK_DIV_MASK, (val))
> +#define SSD130X_SET_CLOCK_FREQ_MASK		GENMASK(7, 4)
> +#define SSD130X_SET_CLOCK_FREQ_SET(val)		FIELD_PREP(SSD130X_SET_CLOCK_FREQ_MASK, (val))
> +#define SSD130X_SET_PRECHARGE_PERIOD1_MASK	GENMASK(3, 0)
> +#define SSD130X_SET_PRECHARGE_PERIOD1_SET(val)	FIELD_PREP(SSD130X_SET_PRECHARGE_PERIOD1_MASK, (val))
> +#define SSD130X_SET_PRECHARGE_PERIOD2_MASK	GENMASK(7, 4)
> +#define SSD130X_SET_PRECHARGE_PERIOD2_SET(val)	FIELD_PREP(SSD130X_SET_PRECHARGE_PERIOD2_MASK, (val))

> +#define SSD130X_SET_COM_PINS_CONFIG1_MASK	GENMASK(4, 4)

BIT(4)

> +#define SSD130X_SET_COM_PINS_CONFIG1_SET(val)	FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG1_MASK, (!val))
> +#define SSD130X_SET_COM_PINS_CONFIG2_MASK	GENMASK(5, 5)

BIT(5)

> +#define SSD130X_SET_COM_PINS_CONFIG2_SET(val)	FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG2_MASK, (val))

I would put GENMASK() directly into FIELD(), but it's up to you
(and I haven't checked the use of *_MASK anyway).


...

> +static int ssd130x_write_data(struct ssd130x_device *ssd130x, u8 *values, int count)
> +{
> +	int ret;
> +
> +	ret = regmap_bulk_write(ssd130x->regmap, SSD130X_DATA, values, count);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

return regmap_bulk_write(...);

> +}

...

> +/*
> + * Helper to write command (SSD130X_COMMAND). The fist variadic argument
> + * is the command to write and the following are the command options.
> + *
> + * Note that the ssd130x protocol requires each command and option to be
> + * written as a SSD130X_COMMAND device register value. That is why a call
> + * to regmap_write(..., SSD130X_COMMAND, ...) is done for each argument.
> + */

Thanks!

> +static int ssd130x_write_cmd(struct ssd130x_device *ssd130x, int count,
> +				    /* u8 cmd, u8 option, ... */...)
> +{
> +	va_list ap;
> +	u8 value;
> +	int ret;
> +
> +	va_start(ap, count);
> +
> +	do {
> +		value = va_arg(ap, int);
> +		ret = regmap_write(ssd130x->regmap, SSD130X_COMMAND, (u8)value);

Wondering if you really need this casting. value is u8 by definition.

> +		if (ret)
> +			goto out_end;
> +	} while (--count);
> +
> +out_end:
> +	va_end(ap);
> +
> +	return ret;
> +}

...

> +	ssd130x = devm_drm_dev_alloc(dev, &ssd130x_drm_driver,
> +				     struct ssd130x_device, drm);
> +	if (IS_ERR(ssd130x)) {

> +		dev_err_probe(dev, PTR_ERR(ssd130x),
> +			      "Failed to allocate DRM device\n");
> +		return ssd130x;

This...

> +	}

...

> +	bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
> +					    &ssd130xfb_bl_ops, NULL);
> +	if (IS_ERR(bl))
> +		return ERR_PTR(dev_err_probe(dev, PTR_ERR(bl),
> +					     "Unable to register backlight device\n"));

Can be consistent with this then.

-- 
With Best Regards,
Andy Shevchenko