| Message ID | 20240816080933.440594-3-victor.liu@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/bridge: imx: Add i.MX93 parallel display format configuration support | expand |
On 16/08/2024 10:09, Liu Ying wrote: > NXP i.MX93 mediamix blk-ctrl contains one DISPLAY_MUX register which > configures parallel display format by using the "PARALLEL_DISP_FORMAT" > field. Add a DRM bridge driver to support the display format configuration. > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > --- ... > + > +static int imx93_pdfc_bridge_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct imx93_pdfc *pdfc; > + int ret; > + > + pdfc = devm_kzalloc(dev, sizeof(*pdfc), GFP_KERNEL); > + if (!pdfc) > + return -ENOMEM; > + > + pdfc->regmap = syscon_node_to_regmap(dev->of_node->parent); > + if (IS_ERR(pdfc->regmap)) { > + ret = PTR_ERR(pdfc->regmap); > + if (ret != -EPROBE_DEFER) > + DRM_DEV_ERROR(dev, "failed to get regmap: %d\n", ret); > + return ret; Nope, you just open-coded dev_err_probe. Syntax is - return dev_err_probe(). if you need wrapper for DRM, add such. > + } > + > + pdfc->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); > + if (IS_ERR(pdfc->next_bridge)) { > + ret = PTR_ERR(pdfc->next_bridge); > + if (ret != -EPROBE_DEFER) > + DRM_DEV_ERROR(dev, "failed to get next bridge: %d\n", ret); > + return ret; Ditto > + } > + ... > +MODULE_DESCRIPTION("NXP i.MX93 parallel display format configuration driver"); > +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:" DRIVER_NAME); Which other driver needs this platform alias? Best regards, Krzysztof
On 08/16/2024, Krzysztof Kozlowski wrote: > On 16/08/2024 10:09, Liu Ying wrote: >> NXP i.MX93 mediamix blk-ctrl contains one DISPLAY_MUX register which >> configures parallel display format by using the "PARALLEL_DISP_FORMAT" >> field. Add a DRM bridge driver to support the display format configuration. >> >> Signed-off-by: Liu Ying <victor.liu@nxp.com> >> --- > > ... > >> + >> +static int imx93_pdfc_bridge_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct imx93_pdfc *pdfc; >> + int ret; >> + >> + pdfc = devm_kzalloc(dev, sizeof(*pdfc), GFP_KERNEL); >> + if (!pdfc) >> + return -ENOMEM; >> + >> + pdfc->regmap = syscon_node_to_regmap(dev->of_node->parent); >> + if (IS_ERR(pdfc->regmap)) { >> + ret = PTR_ERR(pdfc->regmap); >> + if (ret != -EPROBE_DEFER) >> + DRM_DEV_ERROR(dev, "failed to get regmap: %d\n", ret); >> + return ret; > > Nope, you just open-coded dev_err_probe. Syntax is - return > dev_err_probe(). if you need wrapper for DRM, add such. Will use dev_err_probe(). > >> + } >> + >> + pdfc->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); >> + if (IS_ERR(pdfc->next_bridge)) { >> + ret = PTR_ERR(pdfc->next_bridge); >> + if (ret != -EPROBE_DEFER) >> + DRM_DEV_ERROR(dev, "failed to get next bridge: %d\n", ret); >> + return ret; > > Ditto Will use dev_err_probe(). > > >> + } >> + > > ... > >> +MODULE_DESCRIPTION("NXP i.MX93 parallel display format configuration driver"); >> +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>"); >> +MODULE_LICENSE("GPL v2"); >> +MODULE_ALIAS("platform:" DRIVER_NAME); > > Which other driver needs this platform alias? Quote include/linux/module.h: " /* For userspace: you can also call me... */ #define MODULE_ALIAS(_alias) MODULE_INFO(alias, _alias) " Anything wrong with using MODULE_ALIAS() here? > > Best regards, > Krzysztof >
On 16/08/2024 11:44, Liu Ying wrote: > On 08/16/2024, Krzysztof Kozlowski wrote: >> On 16/08/2024 10:09, Liu Ying wrote: >>> NXP i.MX93 mediamix blk-ctrl contains one DISPLAY_MUX register which >>> configures parallel display format by using the "PARALLEL_DISP_FORMAT" >>> field. Add a DRM bridge driver to support the display format configuration. >>> >>> Signed-off-by: Liu Ying <victor.liu@nxp.com> >>> --- >> >> ... >> >>> + >>> +static int imx93_pdfc_bridge_probe(struct platform_device *pdev) >>> +{ >>> + struct device *dev = &pdev->dev; >>> + struct imx93_pdfc *pdfc; >>> + int ret; >>> + >>> + pdfc = devm_kzalloc(dev, sizeof(*pdfc), GFP_KERNEL); >>> + if (!pdfc) >>> + return -ENOMEM; >>> + >>> + pdfc->regmap = syscon_node_to_regmap(dev->of_node->parent); >>> + if (IS_ERR(pdfc->regmap)) { >>> + ret = PTR_ERR(pdfc->regmap); >>> + if (ret != -EPROBE_DEFER) >>> + DRM_DEV_ERROR(dev, "failed to get regmap: %d\n", ret); >>> + return ret; >> >> Nope, you just open-coded dev_err_probe. Syntax is - return >> dev_err_probe(). if you need wrapper for DRM, add such. > > Will use dev_err_probe(). > >> >>> + } >>> + >>> + pdfc->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); >>> + if (IS_ERR(pdfc->next_bridge)) { >>> + ret = PTR_ERR(pdfc->next_bridge); >>> + if (ret != -EPROBE_DEFER) >>> + DRM_DEV_ERROR(dev, "failed to get next bridge: %d\n", ret); >>> + return ret; >> >> Ditto > > Will use dev_err_probe(). > >> >> >>> + } >>> + >> >> ... >> >>> +MODULE_DESCRIPTION("NXP i.MX93 parallel display format configuration driver"); >>> +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>"); >>> +MODULE_LICENSE("GPL v2"); >>> +MODULE_ALIAS("platform:" DRIVER_NAME); >> >> Which other driver needs this platform alias? > > Quote include/linux/module.h: > > " > /* For userspace: you can also call me... */ > #define MODULE_ALIAS(_alias) MODULE_INFO(alias, _alias) > " > > Anything wrong with using MODULE_ALIAS() here? Yes, it redundant. Do not answer with question to the question. Are you adding random code that you cannot justify? It looks like this if you cannot answer why do you need it. Form letter: You should not need MODULE_ALIAS() in normal cases. If you need it, usually it means your device ID table is wrong (e.g. misses either entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute for incomplete ID table. Best regards, Krzysztof
diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig index 8dd89efa8ea7..088241575857 100644 --- a/drivers/gpu/drm/bridge/imx/Kconfig +++ b/drivers/gpu/drm/bridge/imx/Kconfig @@ -78,4 +78,12 @@ config DRM_IMX93_MIPI_DSI Choose this to enable MIPI DSI controller found in Freescale i.MX93 processor. +config DRM_IMX93_PARALLEL_DISP_FMT_CONFIG + tristate "NXP i.MX93 parallel display format configuration" + depends on OF + select DRM_KMS_HELPER + help + Choose this to enable parallel display format configuration + found in NXP i.MX93 processor. + endif # ARCH_MXC || COMPILE_TEST diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile index edb0a7b71b30..8d3499fb7fba 100644 --- a/drivers/gpu/drm/bridge/imx/Makefile +++ b/drivers/gpu/drm/bridge/imx/Makefile @@ -7,3 +7,4 @@ obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-combiner.o obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK) += imx8qxp-pixel-link.o obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK_TO_DPI) += imx8qxp-pxl2dpi.o obj-$(CONFIG_DRM_IMX93_MIPI_DSI) += imx93-mipi-dsi.o +obj-$(CONFIG_DRM_IMX93_PARALLEL_DISP_FMT_CONFIG) += imx93-pdfc.o diff --git a/drivers/gpu/drm/bridge/imx/imx93-pdfc.c b/drivers/gpu/drm/bridge/imx/imx93-pdfc.c new file mode 100644 index 000000000000..61e01b503be5 --- /dev/null +++ b/drivers/gpu/drm/bridge/imx/imx93-pdfc.c @@ -0,0 +1,209 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/* + * Copyright 2022,2023 NXP + */ + +#include <linux/media-bus-format.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#include <drm/drm_atomic_state_helper.h> +#include <drm/drm_bridge.h> +#include <drm/drm_print.h> + +#define DRIVER_NAME "imx93_pdfc" + +#define DISPLAY_MUX 0x60 +#define PARALLEL_DISP_FORMAT 0x700 + +enum imx93_pdfc_format { + RGB888_TO_RGB888 = 0x0, + RGB888_TO_RGB666 = 0x1 << 8, + RGB565_TO_RGB565 = 0x2 << 8, +}; + +struct imx93_pdfc { + struct drm_bridge bridge; + struct drm_bridge *next_bridge; + struct device *dev; + struct regmap *regmap; + u32 format; +}; + +static int imx93_pdfc_bridge_attach(struct drm_bridge *bridge, + enum drm_bridge_attach_flags flags) +{ + struct imx93_pdfc *pdfc = bridge->driver_private; + + return drm_bridge_attach(bridge->encoder, pdfc->next_bridge, bridge, flags); +} + +static void +imx93_pdfc_bridge_atomic_enable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) +{ + struct imx93_pdfc *pdfc = bridge->driver_private; + + regmap_update_bits(pdfc->regmap, DISPLAY_MUX, PARALLEL_DISP_FORMAT, + pdfc->format); +} + +static const u32 imx93_pdfc_bus_output_fmts[] = { + MEDIA_BUS_FMT_RGB888_1X24, + MEDIA_BUS_FMT_RGB666_1X18, + MEDIA_BUS_FMT_RGB565_1X16, + MEDIA_BUS_FMT_FIXED +}; + +static bool imx93_pdfc_bus_output_fmt_supported(u32 fmt) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(imx93_pdfc_bus_output_fmts); i++) { + if (imx93_pdfc_bus_output_fmts[i] == fmt) + return true; + } + + return false; +} + +static u32 * +imx93_pdfc_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state, + u32 output_fmt, + unsigned int *num_input_fmts) +{ + u32 *input_fmts; + + *num_input_fmts = 0; + + if (!imx93_pdfc_bus_output_fmt_supported(output_fmt)) + return NULL; + + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); + if (!input_fmts) + return NULL; + + switch (output_fmt) { + case MEDIA_BUS_FMT_RGB888_1X24: + case MEDIA_BUS_FMT_RGB565_1X16: + input_fmts[0] = output_fmt; + break; + case MEDIA_BUS_FMT_RGB666_1X18: + case MEDIA_BUS_FMT_FIXED: + input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24; + break; + } + + *num_input_fmts = 1; + + return input_fmts; +} + +static int imx93_pdfc_bridge_atomic_check(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + struct imx93_pdfc *pdfc = bridge->driver_private; + + switch (bridge_state->output_bus_cfg.format) { + case MEDIA_BUS_FMT_RGB888_1X24: + pdfc->format = RGB888_TO_RGB888; + break; + case MEDIA_BUS_FMT_RGB666_1X18: + pdfc->format = RGB888_TO_RGB666; + break; + case MEDIA_BUS_FMT_RGB565_1X16: + pdfc->format = RGB565_TO_RGB565; + break; + default: + DRM_DEV_DEBUG_DRIVER(pdfc->dev, "Unsupported output bus format: 0x%x\n", + bridge_state->output_bus_cfg.format); + return -EINVAL; + } + + return 0; +} + +static const struct drm_bridge_funcs imx93_pdfc_bridge_funcs = { + .attach = imx93_pdfc_bridge_attach, + .atomic_enable = imx93_pdfc_bridge_atomic_enable, + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, + .atomic_get_input_bus_fmts = imx93_pdfc_bridge_atomic_get_input_bus_fmts, + .atomic_check = imx93_pdfc_bridge_atomic_check, + .atomic_reset = drm_atomic_helper_bridge_reset, +}; + +static int imx93_pdfc_bridge_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct imx93_pdfc *pdfc; + int ret; + + pdfc = devm_kzalloc(dev, sizeof(*pdfc), GFP_KERNEL); + if (!pdfc) + return -ENOMEM; + + pdfc->regmap = syscon_node_to_regmap(dev->of_node->parent); + if (IS_ERR(pdfc->regmap)) { + ret = PTR_ERR(pdfc->regmap); + if (ret != -EPROBE_DEFER) + DRM_DEV_ERROR(dev, "failed to get regmap: %d\n", ret); + return ret; + } + + pdfc->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); + if (IS_ERR(pdfc->next_bridge)) { + ret = PTR_ERR(pdfc->next_bridge); + if (ret != -EPROBE_DEFER) + DRM_DEV_ERROR(dev, "failed to get next bridge: %d\n", ret); + return ret; + } + + platform_set_drvdata(pdev, pdfc); + + pdfc->dev = dev; + pdfc->bridge.driver_private = pdfc; + pdfc->bridge.funcs = &imx93_pdfc_bridge_funcs; + pdfc->bridge.of_node = dev->of_node; + + drm_bridge_add(&pdfc->bridge); + + return 0; +} + +static void imx93_pdfc_bridge_remove(struct platform_device *pdev) +{ + struct imx93_pdfc *pdfc = platform_get_drvdata(pdev); + + drm_bridge_remove(&pdfc->bridge); +} + +static const struct of_device_id imx93_pdfc_dt_ids[] = { + { .compatible = "nxp,imx93-pdfc", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, imx93_pdfc_dt_ids); + +static struct platform_driver imx93_pdfc_bridge_driver = { + .probe = imx93_pdfc_bridge_probe, + .remove_new = imx93_pdfc_bridge_remove, + .driver = { + .of_match_table = imx93_pdfc_dt_ids, + .name = DRIVER_NAME, + }, +}; +module_platform_driver(imx93_pdfc_bridge_driver); + +MODULE_DESCRIPTION("NXP i.MX93 parallel display format configuration driver"); +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:" DRIVER_NAME);
NXP i.MX93 mediamix blk-ctrl contains one DISPLAY_MUX register which configures parallel display format by using the "PARALLEL_DISP_FORMAT" field. Add a DRM bridge driver to support the display format configuration. Signed-off-by: Liu Ying <victor.liu@nxp.com> --- v2->v3: * No change. * Resend with the patch rebased upon v6.11-rc1. v1->v2: * Set *num_input_fmts to zero in case imx93_pdfc_bridge_atomic_get_input_bus_fmts() returns NULL. * Replace .remove callback with .remove_new callback in imx93_pdfc_bridge_driver. drivers/gpu/drm/bridge/imx/Kconfig | 8 + drivers/gpu/drm/bridge/imx/Makefile | 1 + drivers/gpu/drm/bridge/imx/imx93-pdfc.c | 209 ++++++++++++++++++++++++ 3 files changed, 218 insertions(+) create mode 100644 drivers/gpu/drm/bridge/imx/imx93-pdfc.c