Message-ID: <d4e8c16c-5586-3233-0b99-be15a4c0f7aa@redhat.com>
Date:   Fri, 4 Feb 2022 20:19:12 +0100
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        linux-fbdev@...r.kernel.org, Sam Ravnborg <sam@...nborg.org>,
        dri-devel@...ts.freedesktop.org,
        Noralf Trønnes <noralf@...nnes.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Maxime Ripard <maxime@...no.tech>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Lee Jones <lee.jones@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...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 v2 2/4] drm/tiny: Add driver for Solomon SSD130X OLED
 displays

Hello Andy,

Thanks for your feedback.

On 2/4/22 15:26, Andy Shevchenko wrote:
> On Fri, Feb 04, 2022 at 02:43:45PM +0100, Javier Martinez Canillas wrote:
>> Add a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon OLED
>> controllers that can be programmed via an I2C interface. This is a port
>> of the ssd1307fb driver that already supports these devices.
>>
>> A Device Tree binding is not added because the DRM driver is compatible
>> with the existing binding for the ssd1307fb driver.
> 
> ...
> 
>> +/*
>> + * DRM driver for Solomon SSD130X OLED displays
>> + *
>> + * Copyright 2022 Red Hat Inc.
>> + *
>> + * Based on drivers/video/fbdev/ssd1307fb.c
>> + * Copyright 2012 Free Electrons
> 
>> + *
> 
> No need for this blank line.
>

Ok.
 
>> + */
> 
> ...
> 
>> +struct ssd130x_device {
>> +	struct drm_device drm;
>> +	struct drm_simple_display_pipe pipe;
>> +	struct drm_display_mode mode;
>> +	struct drm_connector connector;
> 
> 
>> +	struct i2c_client *client;
> 
> Can we logically separate hw protocol vs hw interface from day 1, please?
> This will allow to add SPI support for this panel much easier.
> 
> Technically I would like to see here
> 
> 	struct device *dev;
>
> and probably (I haven't looked into design)
> 
> 	struct ssd130x_ops *ops;
> 
> or something alike.
>

Sure. I wanted to keep the driver simple, making the writes bus agnostic and
adding a level of indirection would make it more complex. But I agree that
it will also make easier to add more buses later. I will do that for v3.

[snip]

> 
>> +static inline int ssd130x_write_value(struct i2c_client *client, u8 value)
> 
> Not sure inline does anything useful here.
> Ditto for the rest similar cases.
>

Ok, I'll drop them.
 
> ...
> 
>> +static inline int ssd130x_write_cmd(struct i2c_client *client, int count,
>> +				    /* u8 cmd, u8 value, ... */...)
>> +{
>> +	va_list ap;
>> +	u8 value;
>> +	int ret;
>> +
>> +	va_start(ap, count);
> 
>> +	while (count--) {
>> +		value = va_arg(ap, int);
>> +		ret = ssd130x_write_value(client, (u8)value);
>> +		if (ret)
>> +			goto out_end;
>> +	}
> 
> I'm wondering if this can be written in a form
> 
> 	do {
> 		...
> 	} while (--count);
> 
> In this case it will give a hint that count can't be 0.
>

Sure, I don't have a strong preference. I will change it.

[snip]
 
>> +	ssd130x->pwm = pwm_get(dev, NULL);
>> +	if (IS_ERR(ssd130x->pwm)) {
>> +		dev_err(dev, "Could not get PWM from device tree!\n");
> 
> "device tree" is a bit confusing here if I run this on ACPI.
> Maybe something like "firmware description"?
>

Indeed.
 
>> +		return PTR_ERR(ssd130x->pwm);
>> +	}
> 
> ...
> 
>> +	/* Set initial contrast */
>> +	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_CONTRAST, ssd130x->contrast);
> 
> Creating a local variable for client allows to:
> - make lines shorter and might even be less LOCs
> - allow to convert struct device to client in one place
>   (as per my above comment)
> 
> Ditto for other similar cases.
>

Ok.
 
[snip]

>> +	/* Switch to horizontal addressing mode */
>> +	ret = ssd130x_write_cmd(ssd130x->client, 2, SSD130X_SET_ADDRESS_MODE,
>> +				SSD130X_SET_ADDRESS_MODE_HORIZONTAL);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
> 
> Can it be
> 
> 	return ssd130x_write_cmd(...);
> 
> ?
> 
> ...
>

Yes.

>> +	unsigned int line_length = DIV_ROUND_UP(width, 8);
>> +	unsigned int pages = DIV_ROUND_UP(height, 8);
> 
> For power of two there are more efficient roundup()/rounddown()
> (or with _ in the names, I don't remember by heart).
>

Oh, I didn't know about round_{up,down}(). Thanks a lot for the pointer.

> ...
> 
>> +			for (k = 0; k < m; k++) {
> 
>> +				u8 byte = buf[(8 * i + k) * line_length +
>> +					       j / 8];
> 
> One line?
>

Yes.

>> +				u8 bit = (byte >> (j % 8)) & 1;
>> +
>> +				data |= bit << k;
>> +			}
> 
> ...
> 
>> +static int ssd130x_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
>> +					   const struct drm_display_mode *mode)
>> +{
>> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(pipe->crtc.dev);
>> +
>> +	if (mode->hdisplay != ssd130x->mode.hdisplay &&
>> +	    mode->vdisplay != ssd130x->mode.vdisplay)
>> +		return MODE_ONE_SIZE;
> 
>> +	else if (mode->hdisplay != ssd130x->mode.hdisplay)
>> +		return MODE_ONE_WIDTH;
>> +	else if (mode->vdisplay != ssd130x->mode.vdisplay)
>> +		return MODE_ONE_HEIGHT;
> 
> 'else' in both cases is redundant.
>

Indeed.
 
>> +	return MODE_OK;
>> +}
> 
> ...
> 
>> +poweroff:
> 
> out_power_off: ?
>

Ok.
 
> ...
> 
>> +	if (!fb)
>> +		return;
> 
> Can it happen?
>

I don't know, but saw that the handler of other drivers checked for this so
preferred to play safe and do the same.
 
> ...
> 
>> +	drm_mode_probed_add(connector, mode);
>> +	drm_set_preferred_mode(connector, mode->hdisplay, mode->vdisplay);
>> +
>> +	return 1;
> 
> Positive code, what is the meaning of it?
>

It's the number of connector modes. The driver only supports 1.
 
> ...
> 
>> +	if (device_property_read_u32(dev, "solomon,prechargep2", &ssd130x->prechargep2))
>> +		ssd130x->prechargep2 = 2;
> 
> You can drop conditionals for the optional properties
>


 
> 	ssd130x->prechargep2 = 2;
> 	device_property_read_u32(dev, "solomon,prechargep2", &ssd130x->prechargep2);
> 
> and so on for the similar.
>

Ok.
 
> ...
> 
>> +	ssd130x->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(ssd130x->reset)) {
> 
>> +		ret = PTR_ERR(ssd130x->reset);
>> +		dev_err(dev, "Failed to get reset gpio: %d\n", ret);
>> +		return ret;
> 
> Why not
> 
> 	return dev_err_probe()?
> 
> Each time you call it for deferred probe, it will spam logs.
>

Right. I'll change in all the places you pointed out.

[snip] 

> ...
> 
>> +	{},
> 
> Comma is not needed in terminator entry.
>

Right.
 
> ...
> 
>> +static struct i2c_driver ssd130x_i2c_driver = {
>> +	.driver = {
>> +		.name = DRIVER_NAME,
>> +		.of_match_table = ssd130x_of_match,
>> +	},
>> +	.probe_new = ssd130x_probe,
>> +	.remove = ssd130x_remove,
>> +	.shutdown = ssd130x_shutdown,
>> +};
> 
>> +
> 
> Redundant blank line.
>

Ok.
 
>> +module_i2c_driver(ssd130x_i2c_driver);
> 

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Powered by blists - more mailing lists