| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Yiz+vvnkfn9hemJg@latitude>
Date: Sat, 12 Mar 2022 21:12:46 +0100
From: Jonathan Neuschäfer <j.neuschaefer@....net>
To: Andreas Kemnade <andreas@...nade.info>
Cc: p.zabel@...gutronix.de, airlied@...ux.ie, daniel@...ll.ch,
robh+dt@...nel.org, shawnguo@...nel.org, s.hauer@...gutronix.de,
kernel@...gutronix.de, festevam@...il.com, linux-imx@....com,
maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
tzimmermann@...e.de, dri-devel@...ts.freedesktop.org,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, alistair@...stair23.me,
samuel@...lland.org, josua.mayer@....eu,
letux-kernel@...nphoenux.org
Subject: Re: [RFC PATCH 3/6] drm: mxc-epdc: Add display and waveform
initialisation
On Sun, Feb 06, 2022 at 09:00:13AM +0100, Andreas Kemnade wrote:
> Adds display parameter initialisation, display power up/down and
> waveform loading
>
> Signed-off-by: Andreas Kemnade <andreas@...nade.info>
> ---
[...]
> + /* Enable the v3p3 regulator */
> + ret = regulator_enable(priv->v3p3_regulator);
> + if (IS_ERR((void *)ret)) {
if (ret < 0) is common enough to be understood.
> + dev_err(priv->drm.dev,
> + "Unable to enable V3P3 regulator. err = 0x%x\n",
> + ret);
> + mutex_unlock(&priv->power_mutex);
> + return;
> + }
> +
> + usleep_range(1000, 2000);
> +
> + pm_runtime_get_sync(priv->drm.dev);
> +
> + /* Enable clocks to EPDC */
> + clk_prepare_enable(priv->epdc_clk_axi);
> + clk_prepare_enable(priv->epdc_clk_pix);
> +
> + epdc_write(priv, EPDC_CTRL_CLEAR, EPDC_CTRL_CLKGATE);
> +
> + /* Enable power to the EPD panel */
> + ret = regulator_enable(priv->display_regulator);
> + if (IS_ERR((void *)ret)) {
dito
> + dev_err(priv->drm.dev,
> + "Unable to enable DISPLAY regulator. err = 0x%x\n",
> + ret);
> + mutex_unlock(&priv->power_mutex);
> + return;
> + }
> +
> + ret = regulator_enable(priv->vcom_regulator);
> + if (IS_ERR((void *)ret)) {
dito
> + dev_err(priv->drm.dev,
> + "Unable to enable VCOM regulator. err = 0x%x\n",
> + ret);
> + mutex_unlock(&priv->power_mutex);
> + return;
> + }
> +
> + priv->powered = true;
> +
> + mutex_unlock(&priv->power_mutex);
> +}
[...]
> + priv->rev = ((val & EPDC_VERSION_MAJOR_MASK) >>
> + EPDC_VERSION_MAJOR_OFFSET) * 10
> + + ((val & EPDC_VERSION_MINOR_MASK) >>
> + EPDC_VERSION_MINOR_OFFSET);
Instead of this transformation it might be (1) safer against unexpected
versions and (2) simpler, to store the EPDC_VERSION register content
directly.
Instead of
if (priv->rev == 20) { ... }
we'd have
if (priv->rev == 0x02000000) { ... }
or perhaps something along the lines of
if (priv->rev == EPDC_REV(2, 0, 0)) { ... }
(using a macro that does the proper bitshifts).
> + dev_dbg(priv->drm.dev, "EPDC version = %d\n", priv->rev);
> +
> + if (priv->rev <= 20) {
> + dev_err(priv->drm.dev, "Unsupported version (%d)\n", priv->rev);
> + return -ENODEV;
> + }
> +
> + /* Initialize EPDC pins */
> + pinctrl = devm_pinctrl_get_select_default(priv->drm.dev);
> + if (IS_ERR(pinctrl)) {
> + dev_err(priv->drm.dev, "can't get/select pinctrl\n");
> + return PTR_ERR(pinctrl);
> + }
> +
> + mutex_init(&priv->power_mutex);
> +
> + return 0;
> +}
[...]
> diff --git a/drivers/gpu/drm/mxc-epdc/epdc_waveform.h b/drivers/gpu/drm/mxc-epdc/epdc_waveform.h
> new file mode 100644
> index 000000000000..c5c461b975cb
> --- /dev/null
> +++ b/drivers/gpu/drm/mxc-epdc/epdc_waveform.h
> @@ -0,0 +1,7 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/* Copyright (C) 2022 Andreas Kemnade */
> +int mxc_epdc_fb_get_temp_index(struct mxc_epdc *priv, int temp);
> +int mxc_epdc_prepare_waveform(struct mxc_epdc *priv,
> + const u8 *waveform, size_t size);
> +void mxc_epdc_set_update_waveform(struct mxc_epdc *priv,
> + struct mxcfb_waveform_modes *wv_modes);
> diff --git a/drivers/gpu/drm/mxc-epdc/mxc_epdc.h b/drivers/gpu/drm/mxc-epdc/mxc_epdc.h
> index c5f5280b574f..f7b1cbc4cc4e 100644
> --- a/drivers/gpu/drm/mxc-epdc/mxc_epdc.h
> +++ b/drivers/gpu/drm/mxc-epdc/mxc_epdc.h
> @@ -8,6 +8,32 @@
> #include <drm/drm_drv.h>
> #include <drm/drm_connector.h>
> #include <drm/drm_simple_kms_helper.h>
> +#include <linux/thermal.h>
> +#include "epdc_regs.h"
> +
> +#define TEMP_USE_AMBIENT 0x1000
What's the significance of 0x1000 here? Is it a register value?
> static void mxc_epdc_pipe_update(struct drm_simple_display_pipe *pipe,
> @@ -187,6 +267,7 @@ static struct drm_driver mxc_epdc_driver = {
> static int mxc_epdc_probe(struct platform_device *pdev)
> {
> struct mxc_epdc *priv;
> + const struct firmware *firmware;
> int ret;
>
> priv = devm_drm_dev_alloc(&pdev->dev, &mxc_epdc_driver, struct mxc_epdc, drm);
> @@ -195,6 +276,19 @@ static int mxc_epdc_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, priv);
>
> + ret = mxc_epdc_init_hw(priv);
> + if (ret)
> + return ret;
> +
> + ret = request_firmware(&firmware, "imx/epdc/epdc.fw", priv->drm.dev);
Thinking ahead to the point when we'll have multiple waveforms for
different modes... What's your idea for a naming scheme to distinguish
the different waveform files, and should the default name be epdc.fw, or
perhaps something more specific?
> + if (ret)
> + return ret;
> +
> + ret = mxc_epdc_prepare_waveform(priv, firmware->data, firmware->size);
> + release_firmware(firmware);
> + if (ret)
> + return ret;
> +
> mxc_epdc_setup_mode_config(&priv->drm);
>
> ret = mxc_epdc_output(&priv->drm);
Jonathan
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists