Re: [PATCH v3 04/22] drm/tilcdc: Add support for DRM bus flags and simplify panel config

[Andrew Davis <afd@ti.com>]

On 1/6/26 11:42 AM, Kory Maincent (TI.com) wrote:
> Migrate CRTC mode configuration to use standard DRM bus flags in
> preparation for removing the tilcdc_panel driver and its custom
> tilcdc_panel_info structure.
> 
> Add support for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE and
> DRM_BUS_FLAG_SYNC_DRIVE_NEGEDGE flags to control pixel clock and sync
> signal edge polarity, while maintaining backward compatibility with the
> existing tilcdc panel info structure.
> 
> Simplify several hardware parameters by setting them to fixed defaults
> based on common usage across existing device trees:
> - DMA burst size: 16 (previously configurable via switch statement)
> - AC bias frequency: 255 (previously panel-specific)
> - FIFO DMA request delay: 128 (previously panel-specific)
> 
> These parameters show no variation in real-world usage, so hardcoding
> them simplifies the driver without losing functionality.
> 
> Preserve FIFO threshold configurability by detecting the SoC type, as
> this parameter varies between AM33xx (8) and DA850 (16) platforms.
> 
> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> Signed-off-by: Kory Maincent (TI.com) <kory.maincent@bootlin.com>
> ---
> 
> Change in v2:
> - Use SoC type instead of devicetree parameter to set FIFO threshold
>    value.
> ---
>   drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 47 +++++++++++++-----------------------
>   drivers/gpu/drm/tilcdc/tilcdc_drv.c  | 29 ++++++++++++++++------
>   drivers/gpu/drm/tilcdc/tilcdc_drv.h  |  2 ++
>   3 files changed, 41 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index b06b1453db2dd..2309a9a0c925d 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -285,27 +285,15 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
>   
>   	/* 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;
> +	/* Use 16 bit DMA burst size by default */
> +	reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_16);
> +	if (priv->fifo_th) {
> +		int fifo_th_val = ilog2(priv->fifo_th) - 3;
> +
> +		reg |= (fifo_th_val << 8);
> +	} else {
> +		reg |= (info->fifo_th << 8);
>   	}
> -	reg |= (info->fifo_th << 8);
>   	tilcdc_write(dev, LCDC_DMA_CTRL_REG, reg);
>   
>   	/* Configure timings: */
> @@ -321,8 +309,8 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
>   
>   	/* 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);
> +	/* Use 255 AC Bias Pin Frequency by default */
> +	reg |= LCDC_AC_BIAS_FREQUENCY(255);
>   
>   	/*
>   	 * subtract one from hfp, hbp, hsw because the hardware uses
> @@ -392,20 +380,19 @@ static void tilcdc_crtc_set_mode(struct drm_crtc *crtc)
>   			return;
>   		}
>   	}
> -	reg |= info->fdd << 12;
> +	/* Use 128 FIFO DMA Request Delay by default */
> +	reg |= 128 << 12;
>   	tilcdc_write(dev, LCDC_RASTER_CTRL_REG, reg);
>   
> -	if (info->invert_pxl_clk)
> +	if (info->invert_pxl_clk ||
> +	    mode->flags == DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
>   		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_PIXEL_CLOCK);
>   	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);
> -
> -	if (info->sync_edge)
> +	tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_CTRL);
> +	if (info->sync_edge ||
> +	    mode->flags == DRM_BUS_FLAG_SYNC_DRIVE_NEGEDGE)
>   		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_EDGE);
>   	else
>   		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_EDGE);
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index 3dcbec312bacb..60230fa9cec95 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -31,6 +31,11 @@
>   #include "tilcdc_panel.h"
>   #include "tilcdc_regs.h"
>   
> +enum {
> +	AM33XX_TILCDC,
> +	DA850_TILCDC,
> +};
> +
>   static LIST_HEAD(module_list);
>   
>   static const u32 tilcdc_rev1_formats[] = { DRM_FORMAT_RGB565 };
> @@ -192,11 +197,19 @@ static void tilcdc_fini(struct drm_device *dev)
>   	drm_dev_put(dev);
>   }
>   
> +static const struct of_device_id tilcdc_of_match[] = {
> +		{ .compatible = "ti,am33xx-tilcdc", .data = (void *)AM33XX_TILCDC},
> +		{ .compatible = "ti,da850-tilcdc", .data = (void *)DA850_TILCDC},
> +		{ },
> +};
> +MODULE_DEVICE_TABLE(of, tilcdc_of_match);
> +
>   static int tilcdc_init(const struct drm_driver *ddrv, struct device *dev)
>   {
>   	struct drm_device *ddev;
>   	struct platform_device *pdev = to_platform_device(dev);
>   	struct device_node *node = dev->of_node;
> +	const struct of_device_id *of_id;
>   	struct tilcdc_drm_private *priv;
>   	u32 bpp = 0;
>   	int ret;
> @@ -209,6 +222,10 @@ static int tilcdc_init(const struct drm_driver *ddrv, struct device *dev)
>   	if (IS_ERR(ddev))
>   		return PTR_ERR(ddev);
>   
> +	of_id = of_match_node(tilcdc_of_match, node);

You should be able to use `device_get_match_data()` here, then you would
also be able to keep the tilcdc_of_match table down were it was before.

Andrew

> +	if (!of_id)
> +		return -ENODEV;
> +
>   	ddev->dev_private = priv;
>   	platform_set_drvdata(pdev, ddev);
>   	drm_mode_config_init(ddev);
> @@ -309,6 +326,11 @@ static int tilcdc_init(const struct drm_driver *ddrv, struct device *dev)
>   
>   	DBG("Maximum Pixel Clock Value %dKHz", priv->max_pixelclock);
>   
> +	if ((unsigned int)of_id->data == DA850_TILCDC)
> +		priv->fifo_th = 16;
> +	else
> +		priv->fifo_th = 8;
> +
>   	ret = tilcdc_crtc_create(ddev);
>   	if (ret < 0) {
>   		dev_err(dev, "failed to create crtc\n");
> @@ -597,13 +619,6 @@ static void tilcdc_pdev_shutdown(struct platform_device *pdev)
>   	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
>   }
>   
> -static const struct of_device_id tilcdc_of_match[] = {
> -		{ .compatible = "ti,am33xx-tilcdc", },
> -		{ .compatible = "ti,da850-tilcdc", },
> -		{ },
> -};
> -MODULE_DEVICE_TABLE(of, tilcdc_of_match);
> -
>   static struct platform_driver tilcdc_platform_driver = {
>   	.probe      = tilcdc_pdev_probe,
>   	.remove     = tilcdc_pdev_remove,
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> index 3aba3a1155ba0..79078b4ae7393 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> @@ -61,6 +61,8 @@ struct tilcdc_drm_private {
>   	 */
>   	uint32_t max_width;
>   
> +	u32 fifo_th;
> +
>   	/* Supported pixel formats */
>   	const uint32_t *pixelformats;
>   	uint32_t num_pixelformats;
>