Re: [PATCH v2 2/3] drm/st7571-i2c: add support for Sitronix ST7571 LCD controller

[Marcus Folkesson <marcus.folkesson@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4505 bytes --]

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 <marcus.folkesson@gmail.com>
> 
> Reviewed-by: Thomas Zimmermann <tzimmrmann@suse.de>
> 
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]