Message-ID: <debbde23-d040-46de-9ecc-45e17f537b19@gmail.com>
Date: Tue, 27 Jan 2026 11:39:28 +0800
From: Joey Lu <a0987203069@...il.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: airlied@...il.com, simona@...ll.ch, maarten.lankhorst@...ux.intel.com,
 tzimmermann@...e.de, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, ychuang3@...oton.com, schung@...oton.com,
 yclu4@...oton.com, linux-arm-kernel@...ts.infradead.org,
 dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] drm/nuvoton: add MA35D1 display controller driver


On 1/26/2026 8:50 PM, Maxime Ripard wrote:
> On Mon, Jan 26, 2026 at 04:57:27PM +0800, Joey Lu wrote:
>> +static int ma35_crtc_atomic_set_property(struct drm_crtc *drm_crtc,
>> +					 struct drm_crtc_state *state,
>> +					 struct drm_property *property,
>> +					 uint64_t value)
>> +{
>> +	struct ma35_crtc *crtc = ma35_crtc(drm_crtc);
>> +
>> +	if (property == crtc->dpi_format_prop)
>> +		crtc->dpi_format = value;
>> +	else if (property == crtc->dither_enable_prop)
>> +		crtc->dither_enable = value;
>> +	else if (property == crtc->dither_depth_prop)
>> +		crtc->dither_depth = value;
>> +	else
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35_crtc_atomic_get_property(struct drm_crtc *drm_crtc,
>> +					 const struct drm_crtc_state *state,
>> +					 struct drm_property *property,
>> +					 uint64_t *value)
>> +{
>> +	struct ma35_crtc *crtc = ma35_crtc(drm_crtc);
>> +
>> +	if (property == crtc->dpi_format_prop)
>> +		*value = crtc->dpi_format;
>> +	else if (property == crtc->dither_enable_prop)
>> +		*value = crtc->dither_enable;
>> +	else if (property == crtc->dither_depth_prop)
>> +		*value = crtc->dither_depth;
>> +	else
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
> You shouldn't have any driver specific properties.
>
> The bar to add new ones is pretty high too (see
> https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#requirements and
> https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements)
>
> So I'd suggest you to drop them for now.
Got it. I'll drop them and rely on internal defaults.
>> +int ma35_crtc_init(struct ma35_drm *priv)
>> +{
>> +	struct drm_device *drm_dev = &priv->drm_dev;
>> +	struct device *dev = drm_dev->dev;
>> +	struct ma35_crtc *crtc;
>> +	struct ma35_layer *layer_primary, *layer_cursor;
>> +	struct drm_plane *cursor_plane = NULL;
>> +	int ret;
>> +
>> +	crtc = devm_kzalloc(dev, sizeof(*crtc), GFP_KERNEL);
>> +	if (!crtc)
>> +		return -ENOMEM;
> Allocating a crtc with a devm helper isn't memory safe. You must use a
> drmm helper.
I'll fix it.
>> +static int ma35_drm_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *of_node = pdev->dev.of_node;
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *mem_node;
>> +	struct resource res;
>> +	struct ma35_drm *priv;
>> +	struct drm_device *drm_dev;
>> +	void __iomem *base;
>> +	struct regmap *regmap = NULL;
>> +	int irq;
>> +	int ret;
>> +
>> +	/* Check for reserved memory. Fallback to dynamic allocation if undefined */
>> +	mem_node = of_parse_phandle(of_node, "memory-region", 0);
>> +	if (mem_node) {
>> +		ret = of_address_to_resource(mem_node, 0, &res);
>> +		if (ret) {
>> +			dev_err(dev, "Failed to parse reserved memory resource: %d\n", ret);
>> +			of_node_put(mem_node);
>> +			return ret;
>> +		}
>> +		of_node_put(mem_node);
>> +		dev_info(dev, "registering reserved memory %pR\n", &res);
>> +
>> +		ret = of_reserved_mem_device_init(dev);
>> +		if (ret && ret != -ENODEV) {
>> +			dev_err(dev, "Failed to init memory region\n");
>> +			goto error_early;
>> +		}
>> +	}
> That part doesn't make much sense to me. Can you explain / add a comment
> on why simply calling of_reserved_mem_device_init doesn't work?
You're right. I'll simplify the code to rely solely on that helper and 
remove the misleading comment.
>> +	base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(base)) {
>> +		dev_err(dev, "Failed to map I/O base\n");
>> +		ret = PTR_ERR(base);
>> +		goto error_reserved_mem;
>> +	}
>> +	regmap = devm_regmap_init_mmio(dev, base, &ma35_drm_regmap_config);
>> +	if (IS_ERR(regmap)) {
>> +		dev_err(dev, "Failed to create regmap for I/O\n");
>> +		ret = PTR_ERR(regmap);
>> +		goto error_reserved_mem;
>> +	}
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0) {
>> +		ret = -ENODEV;
>> +		goto error_reserved_mem;
>> +	}
>> +
>> +	priv = devm_drm_dev_alloc(dev, &ma35_drm_driver,
>> +				     struct ma35_drm, drm_dev);
>> +	if (IS_ERR(priv)) {
>> +		ret = PTR_ERR(priv);
>> +		goto error_reserved_mem;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +	drm_dev = &priv->drm_dev;
>> +	priv->regmap = regmap;
>> +	INIT_LIST_HEAD(&priv->layers_list);
>> +
>> +	ret = ma35_clocks_prepare(priv);
>> +	if (ret) {
>> +		drm_err(drm_dev, "Failed to prepare clocks\n");
>> +		goto error_reserved_mem;
>> +	}
>> +
>> +	ret = devm_request_irq(dev, irq, ma35_drm_irq_handler, 0,
>> +			       dev_name(dev), priv);
>> +	if (ret) {
>> +		drm_err(drm_dev, "Failed to request IRQ\n");
>> +		goto error_clocks;
>> +	}
>> +
>> +	/* modeset */
>> +	ret = ma35_mode_init(priv);
>> +	if (ret) {
>> +		drm_err(drm_dev, "Failed to initialize KMS\n");
>> +		goto error_clocks;
>> +	}
>> +
>> +	/* plane */
>> +	ret = ma35_plane_init(priv);
>> +	if (ret) {
>> +		drm_err(drm_dev, "Failed to initialize layers\n");
>> +		goto error_clocks;
>> +	}
>> +
>> +	/* crtc */
>> +	ret = ma35_crtc_init(priv);
>> +	if (ret) {
>> +		drm_err(drm_dev, "Failed to initialize CRTC\n");
>> +		goto error_clocks;
>> +	}
>> +
>> +	/* interface */
>> +	ret = ma35_interface_init(priv);
>> +	if (ret) {
>> +		if (ret != -EPROBE_DEFER)
>> +			drm_err(drm_dev, "Failed to initialize interface\n");
>> +
>> +		goto error_clocks;
>> +	}
>> +
>> +	drm_mode_config_reset(drm_dev);
>> +
>> +	ret = drm_dev_register(drm_dev, 0);
>> +	if (ret) {
>> +		drm_err(drm_dev, "Failed to register DRM device\n");
>> +		goto error_mode;
>> +	}
>> +
>> +	drm_client_setup(drm_dev, NULL);
>> +
>> +	return 0;
>> +
>> +error_mode:
>> +	ma35_mode_fini(priv);
>> +
>> +error_clocks:
>> +	ma35_clocks_unprepare(priv);
>> +
>> +error_reserved_mem:
>> +	of_reserved_mem_device_release(dev);
>> +
>> +error_early:
>> +	return ret;
>> +}
>> +
>> +static void ma35_drm_remove(struct platform_device *pdev)
>> +{
>> +	struct ma35_drm *priv = platform_get_drvdata(pdev);
>> +	struct device *dev = &pdev->dev;
>> +	struct drm_device *drm_dev = &priv->drm_dev;
>> +
>> +	drm_dev_unregister(drm_dev);
>> +	drm_atomic_helper_shutdown(drm_dev);
>> +
>> +	ma35_mode_fini(priv);
>> +
>> +	ma35_clocks_unprepare(priv);
>> +
>> +	of_reserved_mem_device_release(dev);
>> +}
>> +
>> +static void ma35_drm_shutdown(struct platform_device *pdev)
>> +{
>> +	struct ma35_drm *priv = platform_get_drvdata(pdev);
>> +	struct drm_device *drm_dev = &priv->drm_dev;
>> +
>> +	drm_atomic_helper_shutdown(drm_dev);
>> +}
>> +
>> +static __maybe_unused int ma35_drm_suspend(struct device *dev)
>> +{
>> +	struct ma35_drm *priv = dev_get_drvdata(dev);
>> +	struct drm_device *drm_dev = &priv->drm_dev;
>> +
>> +	return drm_mode_config_helper_suspend(drm_dev);
>> +}
>> +
>> +static __maybe_unused int ma35_drm_resume(struct device *dev)
>> +{
>> +	struct ma35_drm *priv = dev_get_drvdata(dev);
>> +	struct drm_device *drm_dev = &priv->drm_dev;
>> +
>> +	return drm_mode_config_helper_resume(drm_dev);
>> +}
>> +
>> +static const struct of_device_id ma35_drm_of_table[] = {
>> +	{ .compatible = "nuvoton,ma35d1-dcu" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ma35_drm_of_table);
>> +
>> +static const struct dev_pm_ops ma35_pm_ops = {
>> +	SET_SYSTEM_SLEEP_PM_OPS(ma35_drm_suspend, ma35_drm_resume)
>> +};
>> +
>> +static struct platform_driver ma35_drm_platform_driver = {
>> +	.probe		= ma35_drm_probe,
>> +	.remove		= ma35_drm_remove,
>> +	.shutdown	= ma35_drm_shutdown,
>> +	.driver		= {
>> +		.name		= "ma35-drm",
>> +		.of_match_table	= ma35_drm_of_table,
>> +		.pm = &ma35_pm_ops,
>> +	},
>> +};
>> +
>> +module_platform_driver(ma35_drm_platform_driver);
>> +
>> +MODULE_AUTHOR("Joey Lu <a0987203069@...il.com>");
>> +MODULE_DESCRIPTION("Nuvoton MA35 series DRM driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/gpu/drm/nuvoton/ma35_drm.h b/drivers/gpu/drm/nuvoton/ma35_drm.h
>> new file mode 100644
>> index 000000000000..68da6b11a323
>> --- /dev/null
>> +++ b/drivers/gpu/drm/nuvoton/ma35_drm.h
>> @@ -0,0 +1,48 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +/*
>> + * Nuvoton DRM driver
>> + *
>> + * Copyright (C) 2026 Nuvoton Technology Corp.
>> + *
>> + * Author: Joey Lu <a0987203069@...il.com>
>> + */
>> +
>> +#ifndef _MA35_DRM_H_
>> +#define _MA35_DRM_H_
>> +
>> +#include <linux/regmap.h>
>> +#include <linux/types.h>
>> +#include <drm/drm_device.h>
>> +
>> +#include "ma35_regs.h"
>> +#include "ma35_plane.h"
>> +#include "ma35_crtc.h"
>> +#include "ma35_interface.h"
>> +
>> +#define DRIVER_MAJOR	1
>> +#define DRIVER_MINOR	0
>> +
>> +#define MA35_INT_STATE_DISP0	BIT(0)
>> +
>> +#define MA35_DISPLAY_ALIGN_PIXELS	32
>> +#define MA35_DISPLAY_PREFER_DEPTH	32
>> +
>> +#define MA35_CURSOR_WIDTH	32
>> +#define MA35_CURSOR_HEIGHT	32
>> +
>> +#define MA35_DISPLAY_MAX_ZPOS	3
>> +
>> +#define ma35_drm(d) \
>> +	container_of(d, struct ma35_drm, drm_dev)
>> +
>> +struct ma35_drm {
>> +	struct drm_device drm_dev;
>> +	struct regmap *regmap;
>> +	struct list_head layers_list;
>> +	struct ma35_crtc *crtc;
>> +	struct ma35_interface *interface;
>> +	struct clk *dcuclk;
>> +	struct clk *dcupclk;
>> +};
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/nuvoton/ma35_interface.c b/drivers/gpu/drm/nuvoton/ma35_interface.c
>> new file mode 100644
>> index 000000000000..48d1535ace2f
>> --- /dev/null
>> +++ b/drivers/gpu/drm/nuvoton/ma35_interface.c
>> @@ -0,0 +1,192 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Nuvoton DRM driver
>> + *
>> + * Copyright (C) 2026 Nuvoton Technology Corp.
>> + *
>> + * Author: Joey Lu <a0987203069@...il.com>
>> + */
>> +
>> +#include <linux/types.h>
>> +#include <linux/clk.h>
>> +
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_bridge.h>
>> +#include <drm/drm_connector.h>
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_edid.h>
>> +#include <drm/drm_encoder.h>
>> +#include <drm/drm_gem_dma_helper.h>
>> +#include <drm/drm_modeset_helper_vtables.h>
>> +#include <drm/drm_of.h>
>> +#include <drm/drm_panel.h>
>> +#include <drm/drm_print.h>
>> +#include <drm/drm_probe_helper.h>
>> +#include <drm/drm_simple_kms_helper.h>
>> +
>> +#include "ma35_drm.h"
>> +
>> +#define ma35_encoder(e) \
>> +	container_of(e, struct ma35_interface, drm_encoder)
>> +#define ma35_connector(c) \
>> +	container_of(c, struct ma35_interface, drm_connector)
>> +
>> +static void ma35_encoder_mode_set(struct drm_encoder *encoder,
>> +	struct drm_crtc_state *crtc_state,
>> +	struct drm_connector_state *conn_state)
>> +{
>> +	struct drm_device *drm_dev = encoder->dev;
>> +	struct ma35_drm *priv = ma35_drm(drm_dev);
>> +	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
>> +	int result;
>> +
>> +	clk_set_rate(priv->dcupclk, adjusted_mode->clock * 1000);
>> +	result = DIV_ROUND_UP(clk_get_rate(priv->dcupclk), 1000);
>> +	drm_dbg(drm_dev, "Pixel clock: %d kHz; request : %d kHz\n", result, adjusted_mode->clock);
>> +}
>> +
>> +static int ma35_encoder_atomic_check(struct drm_encoder *encoder,
>> +					struct drm_crtc_state *crtc_state,
>> +					struct drm_connector_state *conn_state)
>> +{
>> +	struct ma35_interface *interface = ma35_encoder(encoder);
>> +	struct drm_display_info *display_info = &conn_state->connector->display_info;
>> +
>> +	interface->bus_flags = display_info->bus_flags;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct drm_encoder_helper_funcs ma35_encoder_helper_funcs = {
>> +	.atomic_mode_set	= ma35_encoder_mode_set,
>> +	.atomic_check		= ma35_encoder_atomic_check,
>> +};
>> +
>> +static const struct drm_connector_funcs ma35_connector_funcs = {
>> +	.reset			= drm_atomic_helper_connector_reset,
>> +	.fill_modes		= drm_helper_probe_single_connector_modes,
>> +	.destroy		= drm_connector_cleanup,
>> +	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
>> +	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
>> +};
>> +
>> +static int ma35_connector_get_modes(struct drm_connector *drm_connector)
>> +{
>> +	struct ma35_drm *priv = ma35_drm(drm_connector->dev);
>> +	struct drm_device *drm_dev = &priv->drm_dev;
>> +	struct drm_mode_config *mode_config = &drm_dev->mode_config;
>> +	struct ma35_interface *interface = ma35_connector(drm_connector);
>> +	int count;
>> +
>> +	if (!interface->drm_panel) {
>> +		/* Use the default modes */
>> +		count = drm_add_modes_noedid(drm_connector,
>> +				mode_config->max_width, mode_config->max_height);
>> +		drm_set_preferred_mode(drm_connector,
>> +				mode_config->max_width, mode_config->max_height);
>> +
>> +		return count;
>> +	} else {
>> +		return drm_panel_get_modes(interface->drm_panel, drm_connector);
>> +	}
>> +}
>> +
>> +static const struct drm_connector_helper_funcs ma35_connector_helper_funcs = {
>> +	.get_modes		= ma35_connector_get_modes,
>> +};
>> +
>> +static void ma35_encoder_attach_crtc(struct ma35_drm *priv)
>> +{
>> +	uint32_t possible_crtcs = drm_crtc_mask(&priv->crtc->drm_crtc);
>> +
>> +	priv->interface->drm_encoder.possible_crtcs = possible_crtcs;
>> +}
>> +
>> +static int ma35_bridge_try_attach(struct ma35_drm *priv, struct ma35_interface *interface)
>> +{
>> +	struct drm_device *drm_dev = &priv->drm_dev;
>> +	struct device *dev = drm_dev->dev;
>> +	struct device_node *of_node = dev->of_node;
>> +	struct drm_bridge *bridge;
>> +	struct drm_panel *panel;
>> +	int ret;
>> +
>> +	ret = drm_of_find_panel_or_bridge(of_node, 0, 0, &panel, &bridge);
>> +
>> +	if (ret) {
>> +		drm_info(drm_dev, "No panel or bridge found\n");
>> +		return ret;
>> +	}
>> +
>> +	if (panel) {
>> +		bridge = drm_panel_bridge_add_typed(panel, DRM_MODE_CONNECTOR_DPI);
>> +		if (IS_ERR(bridge))
>> +			return PTR_ERR(bridge);
>> +	}
>> +
>> +	interface->drm_panel = panel;
>> +	interface->drm_bridge = bridge;
>> +
>> +	ret = drm_bridge_attach(&interface->drm_encoder, bridge,
>> +				NULL, 0);
>> +	if (ret) {
>> +		drm_err(drm_dev, "Failed to attach bridge to encoder\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int ma35_interface_init(struct ma35_drm *priv)
>> +{
>> +	struct ma35_interface *interface;
>> +	struct drm_device *drm_dev = &priv->drm_dev;
>> +	struct drm_encoder *drm_encoder;
>> +	int ret;
>> +
>> +	/* encoder */
>> +	interface = drmm_simple_encoder_alloc(drm_dev,
>> +					struct ma35_interface, drm_encoder, DRM_MODE_ENCODER_DPI);
>> +	if (!interface) {
>> +		drm_err(drm_dev, "Failed to initialize encoder\n");
>> +		goto error_early;
>> +	}
>> +	priv->interface = interface;
>> +	drm_encoder = &interface->drm_encoder;
>> +	drm_encoder_helper_add(drm_encoder,
>> +			       &ma35_encoder_helper_funcs);
>> +
>> +	/* attach encoder to crtc */
>> +	ma35_encoder_attach_crtc(priv);
>> +
>> +	/* attach bridge to encoder if found one in device tree */
>> +	ret = ma35_bridge_try_attach(priv, interface);
>> +	if (!ret)
>> +		return 0;
>> +
>> +	/* fallback to raw dpi connector */
>> +	ret = drm_connector_init(drm_dev, &interface->drm_connector,
>> +					&ma35_connector_funcs,
>> +					DRM_MODE_CONNECTOR_DPI);
> drmm_connector_init
I'll fix it.
>> +static int ma35_plane_atomic_check(struct drm_plane *drm_plane,
>> +				      struct drm_atomic_state *state)
>> +{
>> +	struct drm_device *drm_dev = drm_plane->dev;
>> +	struct ma35_layer *layer = ma35_layer(drm_plane);
>> +	struct drm_plane_state *new_state =
>> +		drm_atomic_get_new_plane_state(state, drm_plane);
>> +	struct drm_crtc *crtc = new_state->crtc;
>> +	struct drm_framebuffer *fb = new_state->fb;
>> +	struct drm_crtc_state *crtc_state;
>> +	bool can_position;
>> +	u32 format;
>> +
>> +	if (!crtc)
>> +		return 0;
>> +
>> +	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>> +	if (WARN_ON(!crtc_state))
>> +		return -EINVAL;
> You don't necessarily have a crtc state in drm_atomic_state. You should
> use drm_atomic_get_crtc_state.
I'll fix it.
>> +static int ma35_cursor_plane_atomic_check(struct drm_plane *drm_plane,
>> +				      struct drm_atomic_state *state)
>> +{
>> +	struct drm_device *drm_dev = drm_plane->dev;
>> +	struct drm_plane_state *new_state =
>> +		drm_atomic_get_new_plane_state(state, drm_plane);
>> +	struct drm_framebuffer *fb = new_state->fb;
>> +	struct drm_crtc *crtc = new_state->crtc;
>> +	struct drm_crtc_state *crtc_state;
>> +
>> +	if (!fb)
>> +		return 0;
>> +
>> +	if (!crtc)
>> +		return -EINVAL;
>> +
>> +	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>> +	if (WARN_ON(!crtc_state))
>> +		return -EINVAL;
> Ditto
>
I'll fix it.
>> +static int ma35_plane_set_property(struct drm_plane *drm_plane,
>> +	struct drm_plane_state *state, struct drm_property *property,
>> +	uint64_t val)
>> +{
>> +	struct ma35_layer *layer = ma35_layer(drm_plane);
>> +
>> +	if (property == layer->blend_mode_prop)
>> +		layer->config.blend_mode = val;
>> +	else if (property == layer->src_alpha_mode_prop)
>> +		layer->config.alpha_mode[0] = val;
>> +	else if (property == layer->dst_alpha_mode_prop)
>> +		layer->config.alpha_mode[1] = val;
>> +	else if (property == layer->src_color_prop)
>> +		layer->config.src_color = val;
>> +	else if (property == layer->dst_color_prop)
>> +		layer->config.dst_color = val;
>> +	else if (property == layer->swizzle_prop)
>> +		layer->config.swizzle = val;
>> +	else if (property == layer->colorkeylo_prop)
>> +		layer->config.colorkeylo = val;
>> +	else if (property == layer->colorkeyup_prop)
>> +		layer->config.colorkeyup = val;
>> +	else if (property == layer->background_prop)
>> +		layer->config.background = val;
>> +	else if (property == layer->foreground_prop)
>> +		layer->config.foreground = val;
>> +	else if (property == drm_plane->hotspot_x_property)
>> +		state->hotspot_x = val;
>> +	else if (property == drm_plane->hotspot_y_property)
>> +		state->hotspot_y = val;
>> +	else
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35_plane_get_property(struct drm_plane *drm_plane,
>> +	const struct drm_plane_state *state, struct drm_property *property,
>> +	uint64_t *val)
>> +{
>> +	struct ma35_layer *layer = ma35_layer(drm_plane);
>> +
>> +	if (property == layer->blend_mode_prop)
>> +		*val = layer->config.blend_mode;
>> +	else if (property == layer->src_alpha_mode_prop)
>> +		*val = layer->config.alpha_mode[0];
>> +	else if (property == layer->dst_alpha_mode_prop)
>> +		*val = layer->config.alpha_mode[1];
>> +	else if (property == layer->src_color_prop)
>> +		*val = layer->config.src_color;
>> +	else if (property == layer->dst_color_prop)
>> +		*val = layer->config.dst_color;
>> +	else if (property == layer->swizzle_prop)
>> +		*val = layer->config.swizzle;
>> +	else if (property == layer->colorkeylo_prop)
>> +		*val = layer->config.colorkeylo;
>> +	else if (property == layer->colorkeyup_prop)
>> +		*val = layer->config.colorkeyup;
>> +	else if (property == layer->background_prop)
>> +		*val = layer->config.background;
>> +	else if (property == layer->foreground_prop)
>> +		*val = layer->config.foreground;
>> +	else if (property == drm_plane->hotspot_x_property)
>> +		*val = state->hotspot_x;
>> +	else if (property == drm_plane->hotspot_y_property)
>> +		*val = state->hotspot_y;
>> +	else
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
> Same comment for the properties than for the CRTC.
I'll drop them and rely on internal defaults.
>> +static int ma35_layer_create(struct ma35_drm *priv,
>> +			      struct device_node *of_node, u32 index,
>> +			      enum drm_plane_type type)
>> +{
>> +	struct drm_device *drm_dev = &priv->drm_dev;
>> +	struct device *dev = drm_dev->dev;
>> +	struct ma35_layer *layer;
>> +	int ret;
>> +
>> +	layer = devm_kzalloc(dev, sizeof(*layer), GFP_KERNEL);
>> +	if (!layer) {
>> +		ret = -ENOMEM;
>> +		goto error;
>> +	}
> This needs to be allocated using drmm
I'll fix it.
> Maxime

Thank you very much for the detailed and careful review.

Joey

Powered by blists - more mailing lists