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,