Subject: Re: [PATCH 2/2] phy: Add driver for mixel dphy

From: Sam Ravnborg <[email-masked]>
Date: Fri, 25 Jan 2019 17:53:55 +0100

Hi Guido.

Patch looks good but a few comments below.

	Sam

On Fri, Jan 25, 2019 at 11:14:46AM +0100, Guido Günther wrote:
> This adds support for the Mixel DPHY as found on i.MX8 CPUs but since
> this is an IP core it will likely be found on others in the future. So
> instead of adding this to the nwl host driver make it a generic PHY
> driver.
> 
> The driver supports the i.MX8MQ. Support for i.MX8QM and i.MX8QXP can be
> added once the necessary system controller bits are in via
> mixel_dpy_ops.
> 
> Signed-off-by: Guido Günther <[email-masked]>
> ---
>  drivers/phy/Kconfig               |   7 +
>  drivers/phy/Makefile              |   1 +
>  drivers/phy/phy-mixel-mipi-dphy.c | 449 ++++++++++++++++++++++++++++++
>  3 files changed, 457 insertions(+)
>  create mode 100644 drivers/phy/phy-mixel-mipi-dphy.c
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 250abe290ca1..9195b5876bcc 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -48,6 +48,13 @@ config PHY_XGENE
>  	help
>  	  This option enables support for APM X-Gene SoC multi-purpose PHY.
>  
> +config PHY_MIXEL_MIPI_DPHY
> +	bool
> +	depends on OF
> +	select GENERIC_PHY
> +	select GENERIC_PHY_MIPI_DPHY
> +	default ARCH_MXC && ARM64

Is it correct that driver is mandatory if ARCH_MXC is y?
There is no prompt to allow the user to select it.
Or in other words - will all i.MX8 user need it?

> +
>  source "drivers/phy/allwinner/Kconfig"
>  source "drivers/phy/amlogic/Kconfig"
>  source "drivers/phy/broadcom/Kconfig"
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 0d9fddc498a6..264f570b67bf 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_ARCH_MEDIATEK)		+= mediatek/
>  obj-$(CONFIG_ARCH_RENESAS)		+= renesas/
>  obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/
>  obj-$(CONFIG_ARCH_TEGRA)		+= tegra/
> +obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)	+= phy-mixel-mipi-dphy.o
>  obj-y					+= broadcom/	\
>  					   cadence/	\
>  					   freescale/	\
> diff --git a/drivers/phy/phy-mixel-mipi-dphy.c b/drivers/phy/phy-mixel-mipi-dphy.c
> new file mode 100644
> index 000000000000..8a43dab79cee
> --- /dev/null
> +++ b/drivers/phy/phy-mixel-mipi-dphy.c

There is already a PHY named phy-fsl-imx8mq-usb, located in the
freescale subdirectory.
Why locate another imx8 PHY in the top level directory with
another naming convention?

> @@ -0,0 +1,449 @@
> +/*
> + * Copyright 2017 NXP
> + * Copyright 2019 Purism SPC
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
SPDX-License-Identifier goes in at first line with //.
It is documented somewhere.
Also, did you double check that GPL 2.0 is correct?

> +
> +/* #define DEBUG 1 */
There is no reference to DEBUG in this file - delete?

> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +
> +/* DPHY registers */
> +#define DPHY_PD_DPHY			0x00
> +#define DPHY_M_PRG_HS_PREPARE		0x04
> +#define DPHY_MC_PRG_HS_PREPARE		0x08
> +#define DPHY_M_PRG_HS_ZERO		0x0c
> +#define DPHY_MC_PRG_HS_ZERO		0x10
> +#define DPHY_M_PRG_HS_TRAIL		0x14
> +#define DPHY_MC_PRG_HS_TRAIL		0x18
> +#define DPHY_PD_PLL			0x1c
> +#define DPHY_TST			0x20
> +#define DPHY_CN				0x24
> +#define DPHY_CM				0x28
> +#define DPHY_CO				0x2c
> +#define DPHY_LOCK			0x30
> +#define DPHY_LOCK_BYP			0x34
> +#define DPHY_TX_RCAL			0x38
> +#define DPHY_AUTO_PD_EN			0x3c
> +#define DPHY_RXLPRP			0x40
> +#define DPHY_RXCDRP			0x44
> +
> +#define MBPS(x) ((x) * 1000000)
> +
> +#define DATA_RATE_MAX_SPEED MBPS(1500)
> +#define DATA_RATE_MIN_SPEED MBPS(80)
> +
> +#define CN_BUF	0xcb7a89c0
> +#define CO_BUF	0x63
> +#define CM(x)	(				\
> +		((x) <  32)?0xe0|((x)-16) :	\
> +		((x) <  64)?0xc0|((x)-32) :	\
> +		((x) < 128)?0x80|((x)-64) :	\
> +		((x) - 128))
> +#define CN(x)	(((x) == 1)?0x1f : (((CN_BUF)>>((x)-1))&0x1f))
> +#define CO(x)	((CO_BUF)>>(8-(x))&0x3)
> +
> +/* PHY power on is LOW_ENABLE */
> +#define PWR_ON	0
> +#define PWR_OFF	1
> +
> +struct mixel_dphy_cfg {
> +	u32 cm;
> +	u32 cn;
> +	u32 co;
> +	unsigned long hs_clk_rate;
> +	u8 mc_prg_hs_prepare;
> +	u8 m_prg_hs_prepare;
> +	u8 mc_prg_hs_zero;
> +	u8 m_prg_hs_zero;
> +	u8 mc_prg_hs_trail;
> +	u8 m_prg_hs_trail;
> +};

For the naive reader it would be helpful to spell out the names in a comment.
As I assume the names comes from the data sheet the short names are OK - but
let others know the purpose.

> +
> +struct mixel_dphy_priv;
> +struct mixel_dphy_ops {
> +	int (*probe)(struct mixel_dphy_priv *priv);
> +	int (*power_on)(struct phy *phy);
> +	int (*power_off)(struct phy *phy);
> +};
Consider same argument for all three ops, less suprises.
But then probe() is called before we have a phy, so this may be
the best option.
> +
> +struct mixel_dphy_priv {
> +	struct mixel_dphy_cfg cfg;
> +	void __iomem *regs;
> +	struct clk *phy_ref_clk;
> +	struct mutex lock;
> +	const struct mixel_dphy_ops *ops;
> +};
Document what the lock protects, or find a better name for the lock to document it

> +
> +/* Find a ratio close to the desired one using continued fraction
> +   approximation ending either at exact match or maximum allowed
> +   nominator, denominator. */

Use kernel style comments
/*
 * Bla bla
 * more bla
 */

> +static void get_best_ratio(unsigned long *pnum, unsigned long *pdenom, unsigned max_n, unsigned max_d)
Wrap line to stay below 80 chars.
Use checkpatch to help you sport things like this.

> +{
> +	unsigned long a = *pnum;
> +	unsigned long b = *pdenom;
> +	unsigned long c;
> +	unsigned n[] = {0, 1};
> +	unsigned d[] = {1, 0};
> +	unsigned whole;
> +	unsigned i = 1;
> +	while (b) {
Add empty line after last local variable.

> +		i ^= 1;
> +		whole = a / b;
> +		n[i] += (n[i ^ 1] * whole);
> +		d[i] += (d[i ^ 1] * whole);
> +		if ((n[i] > max_n) || (d[i] > max_d)) {
> +			i ^= 1;
> +			break;
> +		}
> +		c = a - (b * whole);
> +		a = b;
> +		b = c;
> +	}
> +	*pnum = n[i];
> +	*pdenom = d[i];
> +}
> +
> +static int mixel_dphy_config_from_opts(struct phy *phy,
> +	       struct phy_configure_opts_mipi_dphy *dphy_opts,
> +	       struct mixel_dphy_cfg *cfg)
> +{
Align extra paratmers below the first parameter using tabs and add necessary
spaces.

> +	struct mixel_dphy_priv *priv = dev_get_drvdata(phy->dev.parent);
> +	unsigned long ref_clk = clk_get_rate(priv->phy_ref_clk);
> +	int i;
> +	unsigned long numerator, denominator, frequency;
> +	unsigned step;
> +
> +static int mixel_dphy_ref_power_on(struct phy *phy)
> +{
> +	struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> +	u32 lock, timeout;
> +	int ret = 0;
> +
> +	mutex_lock(&priv->lock);
> +	clk_prepare_enable(priv->phy_ref_clk);
> +
> +	phy_write(phy, PWR_ON, DPHY_PD_DPHY);
> +	phy_write(phy, PWR_ON, DPHY_PD_PLL);
> +
> +	timeout = 100;
> +	while (!(lock = phy_read(phy, DPHY_LOCK))) {
> +		udelay(10);
> +		if (--timeout == 0) {
> +			dev_err(&phy->dev, "Could not get DPHY lock!\n");
> +			mutex_unlock(&priv->lock);
> +			return -EINVAL;
> +		}
USe goto to have a single exit path where you do mutex_unlock()

> +	}
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
> +
> +	mutex_lock(&priv->lock);
> +
> +	phy_write(phy, 0x00, DPHY_LOCK_BYP);
> +	phy_write(phy, 0x01, DPHY_TX_RCAL);
> +	phy_write(phy, 0x00, DPHY_AUTO_PD_EN);
> +	phy_write(phy, 0x01, DPHY_RXLPRP);
> +	phy_write(phy, 0x01, DPHY_RXCDRP);
> +	phy_write(phy, 0x25, DPHY_TST);
> +
> +	mixel_phy_set_hs_timings(phy);
> +	ret = mixel_dphy_set_pll_params(phy);
> +	if (ret < 0) {
> +		mutex_unlock(&priv->lock);
> +		return ret;
> +	}
USe goto to have a single exit path where you do mutex_unlock()
> +
> +	mutex_unlock(&priv->lock);
> +
> +	return 0;
> +}
> +
> +
> +/*
> + * This is the reference implementation of DPHY hooks. Specific integration of
> + * this IP may have to re-implement some of them depending on how they decided
> + * to wire things in the SoC.
> + */
> +static const struct mixel_dphy_ops mixel_dphy_ref_ops = {
> +	.power_on = mixel_dphy_ref_power_on,
> +	.power_off = mixel_dphy_ref_power_off,
> +};
> +
> +static const struct phy_ops mixel_dphy_ops = {
> +	.power_on = mixel_dphy_power_on,
> +	.power_off = mixel_dphy_power_off,
> +	.configure = mixel_dphy_configure,
> +	.validate = mixel_dphy_validate,
> +	.owner = THIS_MODULE,
> +};
This is confusing.
We have struct mixel_dphy_ops => mixel_dphy_ref_ops
And then struct phy_ops => mixel_dphy_ops

So reading this there are to uses of mixel_dphy_ops,
one is a struct, and another is an instance of another type.
Try to find a niming scheme that is less confusing.

> +
> +static const struct of_device_id mixel_dphy_of_match[] = {
> +	{ .compatible = "mixel,imx8mq-mipi-dphy", .data = &mixel_dphy_ref_ops },
> +	{ /* sentinel */ },
> +};
Multi-line to keep line shorter?

> +MODULE_DEVICE_TABLE(of, mixel_dphy_of_match);
> +
> +static int mixel_dphy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct phy_provider *phy_provider;
> +	struct mixel_dphy_priv *priv;
> +	struct resource *res;
> +	struct phy *phy;
> +	int ret;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->ops = of_device_get_match_data(&pdev->dev);
> +	if (!priv->ops)
> +		return -EINVAL;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	priv->regs = devm_ioremap(dev, res->start, SZ_256);
> +	if (IS_ERR(priv->regs))
> +		return PTR_ERR(priv->regs);
> +
> +	priv->phy_ref_clk = devm_clk_get(&pdev->dev, "phy_ref");
> +	if (IS_ERR(priv->phy_ref_clk)) {
> +		dev_err(dev, "No phy_ref clock found");
> +		return PTR_ERR(priv->phy_ref_clk);
> +	}
> +	dev_dbg(dev, "phy_ref clock rate: %lu", clk_get_rate(priv->phy_ref_clk));
> +
> +	mutex_init(&priv->lock);
> +	dev_set_drvdata(dev, priv);
> +
> +	if (priv->ops->probe) {
> +		ret = priv->ops->probe(priv);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	phy = devm_phy_create(dev, np, &mixel_dphy_ops);
> +	if (IS_ERR(phy)) {
> +		dev_err(dev, "Failed to create phy %ld\n", PTR_ERR(phy));
> +		return PTR_ERR(phy);
> +	}
> +	phy_set_drvdata(phy, priv);
> +
> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +
> +	return PTR_ERR_OR_ZERO(phy_provider);
> +}
> +
> +static struct platform_driver mixel_dphy_driver = {
> +	.probe	= mixel_dphy_probe,
> +	.driver = {
> +		.name = "mixel-mipi-dphy",
> +		.of_match_table	= mixel_dphy_of_match,
> +	}
> +};
> +module_platform_driver(mixel_dphy_driver);
> +
> +MODULE_AUTHOR("NXP Semiconductor");
> +MODULE_DESCRIPTION("Mixel MIPI-DSI PHY driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> [email-masked]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
linux-arm-kernel mailing list
[email-masked]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel