Re: [PATCH v2 2/2] drm/panel: Add Ilitek ILI9341 parallel RGB panel driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sam,
thank you for comments.

st 10. 7. 2019 v 15:47 odesílatel Sam Ravnborg <sam@xxxxxxxxxxxx> napsal:
>
> Hi Josef.
>
> Thanks for updating the driver.
>
> On Mon, Jul 08, 2019 at 04:56:18PM +0200, Josef Lusticky wrote:
> > Add driver for Ilitek ILI9341 panels in parallel RGB mode
> >
> > Signed-off-by: Josef Lusticky <josef@xxxxxxxxxxx>
> > + */
> > +
> > +#include <linux/backlight.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#include <video/mipi_display.h>
> > +
> > +#include <drm/drm_modes.h>
> > +#include <drm/drm_panel.h>
> > +#include <drm/drm_print.h>
> > +#include <drm/tinydrm/mipi-dbi.h>
> Good to see drivers that no longer uses drmP.h :-)
>
> > +
> > +/* ILI9341 extended register set (Vendor Command Set) */
> > +#define ILI9341_IFMODE         0xB0 // clock polarity
> > +#define ILI9341_IFCTL          0xF6 // interface conrol
> > +#define ILI9341_PGAMCTRL       0xE0 // positive gamma control
> > +#define ILI9341_NGAMCTRL       0xE1 // negative gamma control
> > +
> > +#define ILI9341_MADCTL_MV      BIT(5)
> > +#define ILI9341_MADCTL_MX      BIT(6)
> > +#define ILI9341_MADCTL_MY      BIT(7)
> > +
> > +/**
> > + * struct ili9341_config - the display specific configuration
> > + * @width_mm: physical panel width [mm]
> > + * @height_mm: physical panel height [mm]
> > + */
> > +struct ili9341_config {
> > +     u32 width_mm;
> > +     u32 height_mm;
> > +};
> > +
> > +struct ili9341 {
> > +     struct drm_panel panel;
> > +     struct mipi_dbi *mipi;
> > +     const struct ili9341_config *conf;
> > +};
> > +
> > +static inline struct ili9341 *panel_to_ili9341(struct drm_panel *panel)
> > +{
> > +     return container_of(panel, struct ili9341, panel);
> > +}
> > +
> > +static int ili9341_deinit(struct drm_panel *panel, struct ili9341 *ili)
> > +{
> > +     mipi_dbi_command(ili->mipi, MIPI_DCS_SET_DISPLAY_OFF);
> > +     mipi_dbi_command(ili->mipi, MIPI_DCS_ENTER_SLEEP_MODE);
> > +     msleep(5);
> > +     return 0;
> > +}
> > +
> > +static int ili9341_init(struct drm_panel *panel, struct ili9341 *ili)
> > +{
> > +     /* HW / SW Reset display and wait */
> > +     if (ili->mipi->reset)
> > +             mipi_dbi_hw_reset(ili->mipi);
> > +
> > +     mipi_dbi_command(ili->mipi, MIPI_DCS_SOFT_RESET);
> > +     msleep(120);
> Consider a usleep_range here - to have the waiting a little more relaxed
> in the system.
>

I am using msleep here as it is recommended for sleeping for 10ms+
by Documentation/timers/timers-howto.txt.
Anyway, I'll change this to call the
mipi_dbi_poweron_reset():mipi_dbi.c function,
which does the same as above.
Plus, I should change the msleep(5) in ili9341_deinit() to
usleep_range(5000, 20000).

> > +
> > +     /* Polarity */
> > +     mipi_dbi_command(ili->mipi, ILI9341_IFMODE, 0xC0);
> > +
> > +     /* Interface control */
> > +     mipi_dbi_command(ili->mipi, ILI9341_IFCTL, 0x09, 0x01, 0x26);
> > +
> > +     /* Pixel format */
> > +     mipi_dbi_command(ili->mipi, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_18BIT << 4);
> > +
> > +     /* Gamma */
> > +     mipi_dbi_command(ili->mipi, MIPI_DCS_SET_GAMMA_CURVE, 0x01);
> > +     mipi_dbi_command(ili->mipi, ILI9341_PGAMCTRL,
> > +             0x0f, 0x31, 0x2b, 0x0c, 0x0e, 0x08, 0x4e, 0xf1,
> > +             0x37, 0x07, 0x10, 0x03, 0x0e, 0x09, 0x00);
> > +     mipi_dbi_command(ili->mipi, ILI9341_NGAMCTRL,
> > +             0x00, 0x0e, 0x14, 0x03, 0x11, 0x07, 0x31, 0xc1,
> > +             0x48, 0x08, 0x0f, 0x0c, 0x31, 0x36, 0x0f);
> > +
> > +     /* Rotation */
> > +     mipi_dbi_command(ili->mipi, MIPI_DCS_SET_ADDRESS_MODE, ILI9341_MADCTL_MX);
> > +
> > +     /* Exit sleep mode */
> > +     mipi_dbi_command(ili->mipi, MIPI_DCS_EXIT_SLEEP_MODE);
> > +     msleep(120);
> > +
> > +     mipi_dbi_command(ili->mipi, MIPI_DCS_SET_DISPLAY_ON);
> > +
> > +     return 0;
> > +}
> > +
> > +static int ili9341_unprepare(struct drm_panel *panel)
> > +{
> > +     struct ili9341 *ili = panel_to_ili9341(panel);
> > +
> > +     return ili9341_deinit(panel, ili);
> > +}
> > +
> > +static int ili9341_prepare(struct drm_panel *panel)
> > +{
> > +     struct ili9341 *ili = panel_to_ili9341(panel);
> > +     int ret;
> > +
> > +     ret = ili9341_init(panel, ili);
> > +     if (ret < 0)
> > +             ili9341_unprepare(panel);
> > +     return ret;
> > +}
> > +
> > +static int ili9341_enable(struct drm_panel *panel)
> > +{
> > +     struct ili9341 *ili = panel_to_ili9341(panel);
> > +
> > +     return backlight_enable(ili->mipi->backlight);
> > +}
> > +
> > +static int ili9341_disable(struct drm_panel *panel)
> > +{
> > +     struct ili9341 *ili = panel_to_ili9341(panel);
> > +
> > +     return backlight_disable(ili->mipi->backlight);
> > +}
> > +
> > +static const struct drm_display_mode prgb_240x320_mode = {
> > +     .clock = 6350,
> > +
> > +     .hdisplay = 240,
> > +     .hsync_start = 240 + 10,
> > +     .hsync_end = 240 + 10 + 10,
> > +     .htotal = 240 + 10 + 10 + 20,
> > +
> > +     .vdisplay = 320,
> > +     .vsync_start = 320 + 4,
> > +     .vsync_end = 320 + 4 + 2,
> > +     .vtotal = 320 + 4 + 2 + 2,
> > +
> > +     .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> > +     .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED
> > +};
> > +
> > +static int ili9341_get_modes(struct drm_panel *panel)
> > +{
> > +     struct drm_connector *connector = panel->connector;
> > +     struct ili9341 *ili = panel_to_ili9341(panel);
> > +     struct drm_display_mode *mode;
> > +
> > +     mode = drm_mode_duplicate(panel->drm, &prgb_240x320_mode);
> > +     if (!mode) {
> > +             DRM_DEV_ERROR(panel->drm->dev, "bad mode or failed to add mode\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     drm_mode_set_name(mode);
> > +
> > +     mode->width_mm = ili->conf->width_mm;
> > +     mode->height_mm = ili->conf->height_mm;
> > +
> > +     connector->display_info.width_mm = mode->width_mm;
> > +     connector->display_info.height_mm = mode->height_mm;
> > +     connector->display_info.bus_flags |= DRM_BUS_FLAG_DE_HIGH |
> > +             DRM_BUS_FLAG_PIXDATA_POSEDGE | DRM_BUS_FLAG_SYNC_NEGEDGE;
> > +
> > +     drm_mode_probed_add(connector, mode);
> > +
> > +     return 1; /* Number of modes */
> > +}
> > +
> > +static const struct drm_panel_funcs ili9341_drm_funcs = {
> > +     .disable = ili9341_disable,
> > +     .unprepare = ili9341_unprepare,
> > +     .prepare = ili9341_prepare,
> > +     .enable = ili9341_enable,
> > +     .get_modes = ili9341_get_modes,
> > +};
> > +
> > +static int ili9341_probe(struct spi_device *spi)
> > +{
> > +     struct device *dev = &spi->dev;
> > +     struct ili9341 *ili;
> > +     struct mipi_dbi *mipi;
> > +     struct gpio_desc *dc_gpio;
> > +     int ret;
> > +
> > +     mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
> > +     if (!mipi)
> > +             return -ENOMEM;
> > +
> > +     ili = devm_kzalloc(dev, sizeof(*ili), GFP_KERNEL);
> > +     if (!ili)
> > +             return -ENOMEM;
> > +
> > +     ili->mipi = mipi;
> > +
> > +     spi_set_drvdata(spi, ili);
> > +
> > +     /*
> > +      * Every new incarnation of this display must have a unique
> > +      * data entry for the system in this driver.
> > +      */
> > +     ili->conf = of_device_get_match_data(dev);
> > +     if (!ili->conf) {
> > +             DRM_DEV_ERROR(dev, "missing device configuration\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     ili->mipi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +     if (IS_ERR(ili->mipi->reset)) {
> > +             DRM_DEV_ERROR(dev, "failed to get gpio 'reset'\n");
> > +             return PTR_ERR(ili->mipi->reset);
> > +     }
> > +
> > +     ili->mipi->backlight = devm_of_find_backlight(dev);
> > +     if (IS_ERR(ili->mipi->backlight)) {
> > +             DRM_DEV_ERROR(dev, "failed to get backlight\n");
> > +             return PTR_ERR(ili->mipi->backlight);
> > +     }
> > +
> > +     dc_gpio = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
> > +     if (IS_ERR(dc_gpio)) {
> > +             DRM_DEV_ERROR(dev, "failed to get gpio 'dc'\n");
> > +             return PTR_ERR(dc_gpio);
> > +     }
> > +
> > +     ret = mipi_dbi_spi_init(spi, ili->mipi, dc_gpio);
> > +     if (ret) {
> > +             DRM_DEV_ERROR(dev, "MIPI-DBI SPI setup failed\n");
> > +             return ret;
> > +     }
> > +
> > +     drm_panel_init(&ili->panel);
> > +     ili->panel.dev = dev;
> > +     ili->panel.funcs = &ili9341_drm_funcs;
> > +
> > +     return drm_panel_add(&ili->panel);
> > +}
> > +
> > +static int ili9341_remove(struct spi_device *spi)
> > +{
> > +     struct ili9341 *ili = spi_get_drvdata(spi);
> > +
> > +     drm_panel_remove(&ili->panel);
> > +
> > +     ili9341_disable(&ili->panel);
> > +     ili9341_unprepare(&ili->panel);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct ili9341_config dt024ctft_data = {
> > +     .width_mm = 37,
> > +     .height_mm = 49,
> > +};
> > +
> > +static const struct of_device_id ili9341_of_match[] = {
> > +     { .compatible = "displaytech,dt024ctft", .data = &dt024ctft_data },
> > +     { /* sentinel */ }
> > +};
> If another display is supported then the drm_display_mode may not match.
> So the above may not prove enough in the future.
> for now it should be fine.
>
>
> > +MODULE_DEVICE_TABLE(of, ili9341_of_match);
> > +
> > +static struct spi_driver ili9341_driver = {
> > +     .probe = ili9341_probe,
> > +     .remove = ili9341_remove,
> > +     .driver = {
> > +             .name = "panel-ilitek-ili9341",
> > +             .of_match_table = ili9341_of_match,
> > +     },
> > +};
> > +module_spi_driver(ili9341_driver);
> > +
> > +MODULE_AUTHOR("Josef Lusticky <josef@xxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("ILI9341 LCD panel driver");
> > +MODULE_LICENSE("GPL");
>
> Looks good.
> Reviewed-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
>
>         Sam

Thank you!
Josef
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux