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
