Re: [PATCH v2 2/2] drm/tiny: Add driver for Sharp Memory LCD

[Alex Lanzano]

On Sat, Jul 27, 2024 at 11:03:19AM GMT, Krzysztof Kozlowski wrote:
> On 26/07/2024 21:44, Alex Lanzano wrote:
> > Add support for the monochrome Sharp Memory LCDs.
> > 
> > Signed-off-by: Alex Lanzano <lanzano.alex@xxxxxxxxx>
> > Co-developed-by: Mehdi Djait <mehdi.djait@xxxxxxxxxxx>
> > Signed-off-by: Mehdi Djait <mehdi.djait@xxxxxxxxxxx>
> > ---
> >  MAINTAINERS                         |   7 +
> >  drivers/gpu/drm/tiny/Kconfig        |  20 +
> >  drivers/gpu/drm/tiny/Makefile       |   1 +
> >  drivers/gpu/drm/tiny/sharp-memory.c | 726 ++++++++++++++++++++++++++++
> >  4 files changed, 754 insertions(+)
> >  create mode 100644 drivers/gpu/drm/tiny/sharp-memory.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 71b739b40921..b5b08247a994 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -7123,6 +7123,13 @@ S:	Maintained
> >  F:	Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml
> >  F:	drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
> >  
> > +DRM DRIVER FOR SHARP MEMORY LCD
> > +M:	Alex Lanzano <lanzano.alex@xxxxxxxxx>
> > +S:	Maintained
> > +T:	git git://anongit.freedesktop.org/drm/drm-misc
> 
> 
> Do you have drm-misc commit rights? If not, drop it. There is no point
> to put some other maintainer's tree in your entry. Git tree is already
> present in the maintainer's entry who is going to apply the patches.
> 
> 

Will remove.

> > +F:	Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml
> 
> If you rename the file in your patchset, you must rename it EVERYWHERE.
> 
> 

Will do.

> > +F:	drivers/gpu/drm/tiny/sharp-memory.c
> > +
> >  DRM DRIVER FOR SITRONIX ST7586 PANELS
> 
> 
> ...
> 

Oh! Did you need me to rename sharp-memory.c as well?

> > +	smd->spi = spi;
> > +	drm = &smd->drm;
> > +	ret = drmm_mode_config_init(drm);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to initialize drm config\n");
> > +
> > +	smd->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
> > +	if (smd->enable_gpio == NULL)
> > +		dev_warn(&spi->dev, "Enable gpio not defined\n");
> > +
> > +	/*
> > +	 * VCOM is a signal that prevents DC bias from being built up in
> > +	 * the panel resulting in pixels being forever stuck in one state.
> > +	 *
> > +	 * This driver supports three different methods to generate this
> > +	 * signal depending on EXTMODE pin:
> > +	 *
> > +	 * software (EXTMODE = L) - This mode uses a kthread to
> > +	 * periodically send a "maintain display" message to the display,
> > +	 * toggling the vcom bit on and off with each message
> > +	 *
> > +	 * external (EXTMODE = H) - This mode relies on an external
> > +	 * clock to generate the signal on the EXTCOMM pin
> > +	 *
> > +	 * pwm (EXTMODE = H) - This mode uses a pwm device to generate
> > +	 * the signal on the EXTCOMM pin
> > +	 *
> > +	 */
> > +	smd->vcom = 0;
> > +	if (device_property_read_string(&spi->dev, "vcom-mode", &vcom_mode_str))
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "Unable to find vcom-mode node in device tree\n");
> > +
> > +	if (!strcmp("software", vcom_mode_str)) {
> > +		smd->vcom_mode = SHARP_MEMORY_SOFTWARE_VCOM;
> > +
> > +	} else if (!strcmp("external", vcom_mode_str)) {
> > +		smd->vcom_mode = SHARP_MEMORY_EXTERNAL_VCOM;
> > +
> > +	} else if (!strcmp("pwm", vcom_mode_str)) {
> > +		smd->vcom_mode = SHARP_MEMORY_PWM_VCOM;
> > +		ret = sharp_memory_init_pwm_vcom_signal(smd);
> > +		if (ret)
> > +			return dev_err_probe(dev, ret,
> > +					     "Failed to initialize external COM signal\n");
> > +	} else {
> > +		return dev_err_probe(dev, -EINVAL, "Invalid value set for vcom-mode\n");
> > +	}
> > +
> > +	drm->mode_config.funcs = &sharp_memory_mode_config_funcs;
> > +
> > +	/* Set the DRM display mode depending on panel model */
> > +	model = (uintptr_t)spi_get_device_match_data(spi);
> > +	switch (model) {
> > +	case LS013B7DH03:
> > +		smd->mode = &sharp_memory_ls013b7dh03_mode;
> > +		break;
> > +
> > +	case LS010B7DH04:
> > +		smd->mode = &sharp_memory_ls010b7dh04_mode;
> > +		break;
> > +
> > +	case LS011B7DH03:
> > +		smd->mode = &sharp_memory_ls011b7dh03_mode;
> > +		break;
> > +
> > +	case LS012B7DD01:
> > +		smd->mode = &sharp_memory_ls012b7dd01_mode;
> > +		break;
> > +
> > +	case LS013B7DH05:
> > +		smd->mode = &sharp_memory_ls013b7dh05_mode;
> > +		break;
> > +
> > +	case LS018B7DH02:
> > +		smd->mode = &sharp_memory_ls018b7dh02_mode;
> > +		break;
> > +
> > +	case LS027B7DH01:
> > +		fallthrough;
> > +	case LS027B7DH01A:
> > +		smd->mode = &sharp_memory_ls027b7dh01_mode;
> > +		break;
> > +
> > +	case LS032B7DD02:
> > +		smd->mode = &sharp_memory_ls032b7dd02_mode;
> > +		break;
> > +
> > +	case LS044Q7DH01:
> > +		smd->mode = &sharp_memory_ls044q7dh01_mode;
> > +		break;
> 
> This is over-complicated. Just store the mode in device match data.
> 
> 

Will simplify in v3

> > +
> > +	default:
> > +		return dev_err_probe(&spi->dev, -EINVAL, "Invalid DRM display mode\n");
> > +	}
> 
> 
> Best regards,
> Krzysztof
> 

Thank you for taking the time to review!

Best regards,
Alex