Re: [RFC PATCH 3/6] drm: mxc-epdc: Add display and waveform initialisation

[Jonathan Neuschäfer]

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@xxxxxxxxxxxx>
> ---
[...]
> +	/* 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

Attachment: signature.asc
Description: PGP signature