Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 12 Mar 2022 21:12:46 +0100
From:   Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>
To:     Andreas Kemnade <andreas@kemnade.info>
Cc:     p.zabel@pengutronix.de, airlied@linux.ie, daniel@ffwll.ch,
        robh+dt@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de,
        kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com,
        maarten.lankhorst@linux.intel.com, mripard@kernel.org,
        tzimmermann@suse.de, dri-devel@lists.freedesktop.org,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, alistair@alistair23.me,
        samuel@sholland.org, josua.mayer@jm0.eu,
        letux-kernel@openphoenux.org
Subject: Re: [RFC PATCH 3/6] drm: mxc-epdc: Add display and waveform
 initialisation
Message-ID: <Yiz+vvnkfn9hemJg@latitude>
References: <20220206080016.796556-1-andreas@kemnade.info>
 <20220206080016.796556-4-andreas@kemnade.info>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="uD2AzuFRXoVuvUY3"
Content-Disposition: inline
In-Reply-To: <20220206080016.796556-4-andreas@kemnade.info>
Precedence: bulk


--uD2AzuFRXoVuvUY3
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sun, Feb 06, 2022 at 09:00:13AM +0100, Andreas Kemnade wrote:
> Adds display parameter initialisation, display power up/down and
> waveform loading
>=20
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
[...]
> +	/* Enable the v3p3 regulator */
> +	ret =3D 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 =3D 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 =3D regulator_enable(priv->display_regulator);
> +	if (IS_ERR((void *)ret)) {

dito

> +		dev_err(priv->drm.dev,
> +			"Unable to enable DISPLAY regulator. err =3D 0x%x\n",
> +			ret);
> +		mutex_unlock(&priv->power_mutex);
> +		return;
> +	}
> +
> +	ret =3D regulator_enable(priv->vcom_regulator);
> +	if (IS_ERR((void *)ret)) {

dito

> +		dev_err(priv->drm.dev,
> +			"Unable to enable VCOM regulator. err =3D 0x%x\n",
> +			ret);
> +		mutex_unlock(&priv->power_mutex);
> +		return;
> +	}
> +
> +	priv->powered =3D true;
> +
> +	mutex_unlock(&priv->power_mutex);
> +}

[...]
> +	priv->rev =3D ((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 =3D=3D 20) { ... }

we'd have

	if (priv->rev =3D=3D 0x02000000) { ... }

or perhaps something along the lines of

	if (priv->rev =3D=3D EPDC_REV(2, 0, 0)) { ... }

(using a macro that does the proper bitshifts).

> +	dev_dbg(priv->drm.dev, "EPDC version =3D %d\n", priv->rev);
> +
> +	if (priv->rev <=3D 20) {
> +		dev_err(priv->drm.dev, "Unsupported version (%d)\n", priv->rev);
> +		return -ENODEV;
> +	}
> +
> +	/* Initialize EPDC pins */
> +	pinctrl =3D 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/m=
xc-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-ep=
dc/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 =3D {
>  static int mxc_epdc_probe(struct platform_device *pdev)
>  {
>  	struct mxc_epdc *priv;
> +	const struct firmware *firmware;
>  	int ret;
> =20
>  	priv =3D devm_drm_dev_alloc(&pdev->dev, &mxc_epdc_driver, struct mxc_ep=
dc, drm);
> @@ -195,6 +276,19 @@ static int mxc_epdc_probe(struct platform_device *pd=
ev)
> =20
>  	platform_set_drvdata(pdev, priv);
> =20
> +	ret =3D mxc_epdc_init_hw(priv);
> +	if (ret)
> +		return ret;
> +
> +	ret =3D 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 =3D mxc_epdc_prepare_waveform(priv, firmware->data, firmware->size);
> +	release_firmware(firmware);
> +	if (ret)
> +		return ret;
> +
>  	mxc_epdc_setup_mode_config(&priv->drm);
> =20
>  	ret =3D mxc_epdc_output(&priv->drm);


Jonathan

--uD2AzuFRXoVuvUY3
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEvHAHGBBjQPVy+qvDCDBEmo7zX9sFAmIs/p0ACgkQCDBEmo7z
X9svPg/+OL4D1gwnQuMC1jKKa1HHwROE2fd/xV2gC/5F+52t1t+rjYcMcp7UtIJe
X9q0wiuOOYNLa8LW3kX6HFVlRWpsMZ7XI14AeHwGzdPqtACpRwfnmtamSvp4QrG/
XddVB5wPnfmnFzjuHqQXb/ekIDZCYk2V5rNNyi7ZJ2ESpCxxAUVzws4172PIFAJL
h7NquhDpCKFjrxO+7/NDQ7+hRFb0yW6rPx5WnrgHrVIcBSskH5lZxP7XYticAElq
GxHg57wXN3OW9oYmZ9sCQ8GmzVCPeTAprdOc6mqh/ZKA/sTh1IpRnSa0wDSQGo5n
yiFYtYcGJ0VAplDbRk4U4J55om3AJC4z7PB6LFuebSrRAINHUJinwC2qiqfBMzBV
jkxzB6xPaONjAiREDtGziYf6Py1nv3/uaqd2gEORiiHX4DJxZi3aoeEcc3xGieaB
ZfQ5EmLWIbx79ijoX6ZTcl1CXeVJ7d7yOJYTBRlx7f2VYl6gA/P3n9Jzj1E2Lqmk
5K0O3j+CieKPuIroS6nKJtuFgsWb8Hl5sReMbNlJK55KxN0AX90oi6ZEvXHb9GxC
hkqfiuvY1oWmNnLWD3GQXrGaPN8FjMbEL3E5qQbYHR2wyFCjveNZn4cLf64zWdv2
7urZ4VDlg+x8wOK/DWAaZXQQqrkkSQVM2QLRq+ewhRiRRIzstgQ=
=nc/z
-----END PGP SIGNATURE-----

--uD2AzuFRXoVuvUY3--