Hi Thomas, Thank you for your review and feedback!
On Mon, Apr 07, 2025 at 10:30:28AM +0200, Thomas Zimmermann wrote: > Hi > > Am 04.04.25 um 15:50 schrieb Marcus Folkesson: > > Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller. > > The controller has a SPI, I2C and 8bit parallel interface, this > > driver is for the I2C interface only. > > > > Signed-off-by: Marcus Folkesson <[email protected]> > > Reviewed-by: Thomas Zimmermann <[email protected]> > > I have a few points below, but it's all minor details. The driver looks good > overall. > > > --- > > drivers/gpu/drm/tiny/Kconfig | 11 + > > drivers/gpu/drm/tiny/Makefile | 1 + > > drivers/gpu/drm/tiny/st7571-i2c.c | 720 > > ++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 732 insertions(+) > > [...] > > > + select REGMAP_I2C > > + help > > + DRM driver for Sitronix ST7571 panels controlled over I2C. > > + > > + if M is selected the module will be called st7571-i2c. > > Is there a reason why it is called _i2c? There's another interface, I > assume? Yes exactly, the ST7571 has a SPI and 8bit parallel interface as well. All common parts has been written so that it could easily be put into a common file later on. It should pretty much just be to create a new driver with another regmap interface to support other interfaces then. I only have the I2C interface to test with for now, so I leave it to the future. [...] > > +static const struct drm_connector_helper_funcs > > st7571_connector_helper_funcs = { > > + .get_modes = st7571_connector_get_modes, > > +}; > > + > > +static const uint32_t st7571_primary_plane_formats[] = { > > + DRM_FORMAT_C1, > > + DRM_FORMAT_C2, > > +}; > > I assume that you only get fbcon output for now. Some ancient X11window > managers might also work. Today's GUIs usually need something like XRGB to > work. We do have DRM helpers to convert from XRGB to Cn, but they currently > don't support formats with multiple pixels per byte. It's on my TODO list > and you driver could add XRGB support at some point. Yes I do. Oh, sounds good. I will keep myself updated. [...] > > + ret = st7571_lcd_init(st7571); > > + if (ret) > > + return ret; > > + > > > > + ret = st7571_mode_config_init(st7571); > > + if (ret) { > > + dev_err(&client->dev, "Failed to initialize mode config\n"); > > + return ret; > > + } > > + > > + ret = st7571_plane_init(st7571); > > + if (ret) { > > + dev_err(dev->dev, "Failed to initialize primary plane\n"); > > + return ret; > > + } > > + > > + ret = st7571_crtc_init(st7571); > > + if (ret < 0) { > > + dev_err(dev->dev, "Failed to initialize CRTC\n"); > > + return ret; > > + } > > + > > + ret = st7571_encoder_init(st7571); > > + if (ret < 0) { > > + dev_err(dev->dev, "Failed to initialize encoder\n"); > > + return ret; > > + } > > + > > + ret = st7571_connector_init(st7571); > > + if (ret < 0) { > > + dev_err(dev->dev, "Failed to initialize connector\n"); > > + return ret; > > + } > > + > > + drm_mode_config_reset(dev); > > No need for all these individual _init functions. For such simple drivers, > it might be easier to put all pipeline setup here. Or put everything > including (drm_mode_config_reset()) into st7571_mode_config_init(). Just an > advise; your choice. Yeah I know. It's partly to make it easier for me separate things in my head since I'm new to this subsystem, and partly as I plan to move these functions to a common file later when I add support for the other interfaces. When I started to dig into the code of other drivers, clear lines as this would have helped me, so I think I would like to keep it as it is if it doesn't hurt too many eyes. [...] > > > + > > + ret = drm_dev_register(dev, 0); > > + if (ret) { > > + dev_err(dev->dev, "Failed to register DRM device\n"); > > + return ret; > > + } > > + > > + drm_client_setup(dev, NULL); > > + return 0; > > +} > > + > > +static void st7571_remove(struct i2c_client *client) > > +{ > > + struct st7571_device *st7571 = i2c_get_clientdata(client); > > + > > + drm_dev_unplug(&st7571->dev); > > > + drm_atomic_helper_shutdown(&st7571->dev); > > This would disable the mode-setting pipeline. But as the device has been > unplugged already, it won't really do anything. You can leave it out. > > Best regards > Thomas Best regards, Marcus Folkesson
signature.asc
Description: PGP signature
