Re: [PATCH 2/2] drm/tilcdc: Remove unnecessary struct tilcdc_panel_info members

Hi Jyri,

Thank you for the patch.

On Fri, Feb 15, 2019 at 10:13:16AM +0200, Jyri Sarha wrote:

Most of the struct tilcdc_panel_info data members, that are also exposed in dts binding, are essentially display IP register bits that should not need customization per connected display basis. This patch removes them, both from the binding and the struct. The removed data members have sane static values and there should be no need to expose them in the device tree, certainly not in the panel node.

The removed data members are: ac_bias, ac_bias_intrpt, dma_burst_sz, bpp, fdd, sync-ctrl, tft_alt_mode, and raster_order. All of those are removed from the driver implementation and from bundled tilcdc panel binding. The driver now configures the tilcdc with the sane static values, instead of whatever the driver finds from the struct tilcdc_panel_info.

The patch does does not cause any functional change to any platform supported by Linux mainline kernel and devicetree files.

Signed-off-by: Jyri Sarha jsarha@ti.com

.../bindings/display/tilcdc/panel.txt | 20 +++++--- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 50 +++++-------------- drivers/gpu/drm/tilcdc/tilcdc_drv.h | 27 ---------- drivers/gpu/drm/tilcdc/tilcdc_external.c | 16 ------ drivers/gpu/drm/tilcdc/tilcdc_panel.c | 9 ---- drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 8 --- 6 files changed, 24 insertions(+), 106 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/tilcdc/panel.txt b/Documentation/devicetree/bindings/display/tilcdc/panel.txt index 808216310ea2..b3d63e3db714 100644 --- a/Documentation/devicetree/bindings/display/tilcdc/panel.txt +++ b/Documentation/devicetree/bindings/display/tilcdc/panel.txt @@ -3,15 +3,19 @@ Device-Tree bindings for tilcdc DRM generic panel output driver Required properties:

• compatible: value should be "ti,tilcdc,panel".
• panel-info: configuration info to configure LCDC correctly for the panel
• • ac-bias: AC Bias Pin Frequency
• • ac-bias-intrpt: AC Bias Pin Transitions per Interrupt
• • dma-burst-sz: DMA burst size
• • bpp: Bits per pixel
• • fdd: FIFO DMA Request Delay
  • sync-edge: Horizontal and Vertical Sync Edge: 0=rising 1=falling
• • sync-ctrl: Horizontal and Vertical Sync: Control: 0=ignore
• • raster-order: Raster Data Order Select: 1=Most-to-least 0=Least-to-most
• • fifo-th: DMA FIFO threshold

• • For compatibility with the older driver versions it is still good

• keep these obsolete properties (with their recommended vaules):
  

• - ac-bias           = <255>;
  

• - ac-bias-intrpt    = <0>;
  

• - dma-burst-sz      = <16>;
  

• - bpp               = <16>;
  

• - fdd               = <0x80>;
  

• - sync-ctrl         = <1>;
  

• - raster-order      = <0>;
  

• - fifo-th           = <0>;
  

I don't think you need to keep this text, the properties will be available from the git history. We actually tend to do it the other way around, and remove properties from the bindings and keep them in the driver for backward-compatibility purpose. If removal of the properties don't cause issues (I'll trust you on that) then backward-compatibility code isn't mandatory.

• display-timings: typical videomode of lcd panel. Multiple video modes can be listed if the panel supports multiple timings, but the 'native-mode' should be the preferred/default resolution. Refer to

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 1067e702c22c..ea2e4afad75c 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -295,29 +295,11 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc) return;

/* Configure the Burst Size and fifo threshold of DMA: */

• reg = tilcdc_read(dev, LCDC_DMA_CTRL_REG) & ~0x00000770;
• switch (info->dma_burst_sz) {
• case 1:

• reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_1);
  

• break;
  

• case 2:

• reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_2);
  

• break;
  

• case 4:

• reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_4);
  

• break;
  

• case 8:

• reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_8);
  

• break;
  

• case 16:

• reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_16);
  

• break;
  

• default:

• dev_err(dev->dev, "invalid burst size\n");
  

• return;
  

• }
• reg |= (info->fifo_th << 8);
• tilcdc_write(dev, LCDC_DMA_CTRL_REG, reg);

• tilcdc_write_mask(dev, LCDC_DMA_CTRL_REG,

• 	  LCDC_DMA_FIFO_THRESHOLD(0) |
  

• 	  LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_16),
  

• 	  LCDC_DMA_FIFO_THRESHOLD_MASK |
  

• 	  LCDC_DMA_BURST_SIZE_MASK);
  
  

  /* Configure timings: */ hbp = mode->htotal - mode->hsync_end;

@@ -331,9 +313,11 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc) mode->hdisplay, mode->vdisplay, hbp, hfp, hsw, vbp, vfp, vsw);

/* Set AC Bias Period and Number of Transitions per Interrupt: */

• reg = tilcdc_read(dev, LCDC_RASTER_TIMING_2_REG) & ~0x000fff00;
• reg |= LCDC_AC_BIAS_FREQUENCY(info->ac_bias) |

• LCDC_AC_BIAS_TRANSITIONS_PER_INT(info->ac_bias_intrpt);
  

• reg = tilcdc_read(dev, LCDC_RASTER_TIMING_2_REG);

• reg &= ~LCDC_AC_BIAS_FREQUENCY_MASK;

• reg |= LCDC_AC_BIAS_FREQUENCY(255);

• reg &= ~LCDC_AC_BIAS_TRANSITIONS_PER_INT_MASK;

• reg |= LCDC_AC_BIAS_TRANSITIONS_PER_INT(0);

  /*

  • subtract one from hfp, hbp, hsw because the hardware uses

@@ -383,8 +367,6 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc) LCDC_V2_TFT_24BPP_MODE | LCDC_V2_TFT_24BPP_UNPACK | 0x000ff000 /* Palette Loading Delay bits */); reg |= LCDC_TFT_MODE; /* no monochrome/passive support */

• if (info->tft_alt_mode)

• reg |= LCDC_TFT_ALT_ENABLE;
  

  if (priv->rev == 2) { switch (fb->format->format) { case DRM_FORMAT_BGR565:

@@ -403,7 +385,6 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc) return; } }

• reg |= info->fdd < 12; tilcdc_write(dev, LCDC_RASTER_CTRL_REG, reg);

  if (info->invert_pxl_clk)

@@ -411,10 +392,8 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc) else tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_PIXEL_CLOCK);

• if (info->sync_ctrl)

• tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_CTRL);
  

• else

• tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_CTRL);
  

• /* Take sync polarity from LCDC_SYNC_EDGE bit */

• tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_CTRL);

  if (info->sync_edge) tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_EDGE);

@@ -431,11 +410,6 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc) else tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_VSYNC);

• if (info->raster_order)

• tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ORDER);
  

• else

• tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ORDER);
  

• tilcdc_crtc_set_clk(crtc);

  tilcdc_crtc_load_palette(crtc);

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h index 62cea5ff5558..acf27e22c409 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h @@ -125,38 +125,11 @@ void tilcdc_module_cleanup(struct tilcdc_module *mod); */ struct tilcdc_panel_info {

• /* AC Bias Pin Frequency */

• uint32_t ac_bias;

• /* AC Bias Pin Transitions per Interrupt */

• uint32_t ac_bias_intrpt;

• /* DMA burst size */

• uint32_t dma_burst_sz;

• /* Bits per pixel */

• uint32_t bpp;

• /* FIFO DMA Request Delay */

• uint32_t fdd;

• /* TFT Alternative Signal Mapping (Only for active) */

• bool tft_alt_mode;

• /* Invert pixel clock */ bool invert_pxl_clk;

  /* Horizontal and Vertical Sync Edge: 0=rising 1=falling */ uint32_t sync_edge;

• /* Horizontal and Vertical Sync: Control: 0=ignore */

• uint32_t sync_ctrl;

• /* Raster Data Order Select: 1=Most-to-least 0=Least-to-most */

• uint32_t raster_order;

• /* DMA FIFO threshold */

• uint32_t fifo_th;

};

#define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__) diff --git a/drivers/gpu/drm/tilcdc/tilcdc_external.c b/drivers/gpu/drm/tilcdc/tilcdc_external.c index b4eaf9bc87f8..9626cccf0d17 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_external.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_external.c @@ -16,28 +16,12 @@ #include "tilcdc_external.h"

static const struct tilcdc_panel_info panel_info_tda998x = {

• .ac_bias                = 255,
  

• .ac_bias_intrpt         = 0,
  

• .dma_burst_sz           = 16,
  

• .bpp                    = 16,
  

• .fdd                    = 0x80,
  

• .tft_alt_mode           = 0,
  

  .invert_pxl_clk = 1, .sync_edge = 1,

• .sync_ctrl              = 1,
  

• .raster_order           = 0,
  

};

static const struct tilcdc_panel_info panel_info_default = {

• .ac_bias                = 255,
  

• .ac_bias_intrpt         = 0,
  

• .dma_burst_sz           = 16,
  

• .bpp                    = 16,
  

• .fdd                    = 0x80,
  

• .tft_alt_mode           = 0,
  

  .sync_edge = 0,

• .sync_ctrl              = 1,
  

• .raster_order           = 0,
  

};

static int tilcdc_external_mode_valid(struct drm_connector *connector, diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c b/drivers/gpu/drm/tilcdc/tilcdc_panel.c index a1acab39d87f..1f788171cdbb 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c @@ -292,18 +292,9 @@ static struct tilcdc_panel_info *of_get_panel_info(struct device_node *np) if (!info) goto put_node;

• ret |= of_property_read_u32(info_np, "ac-bias", &info->ac_bias);

• ret |= of_property_read_u32(info_np, "ac-bias-intrpt", &info->ac_bias_intrpt);

• ret |= of_property_read_u32(info_np, "dma-burst-sz", &info->dma_burst_sz);

• ret |= of_property_read_u32(info_np, "bpp", &info->bpp);

• ret |= of_property_read_u32(info_np, "fdd", &info->fdd); ret |= of_property_read_u32(info_np, "sync-edge", &info->sync_edge);

• ret |= of_property_read_u32(info_np, "sync-ctrl", &info->sync_ctrl);

• ret |= of_property_read_u32(info_np, "raster-order", &info->raster_order);

• ret |= of_property_read_u32(info_np, "fifo-th", &info->fifo_th);

  /* optional: */

• info->tft_alt_mode = of_property_read_bool(info_np, "tft-alt-mode"); info->invert_pxl_clk = of_property_read_bool(info_np, "invert-pxl-clk");

  if (ret) {

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c index daebf1aa6b0a..12404401b337 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c @@ -34,15 +34,7 @@ struct tfp410_module {

static const struct tilcdc_panel_info dvi_info = {

• .ac_bias                = 255,
  

• .ac_bias_intrpt         = 0,
  

• .dma_burst_sz           = 16,
  

• .bpp                    = 16,
  

• .fdd                    = 0x80,
  

• .tft_alt_mode           = 0,
  

  .sync_edge = 0,

• .sync_ctrl              = 1,
  

• .raster_order           = 0,
  

};

/*