Hi Guido,
Thanks for the respin. It looks better :-)
On Fri, Feb 1, 2019 at 6:50 AM Guido Günther <agx@xxxxxxxxxxx> wrote:
> +config PHY_MIXEL_MIPI_DPHY
> + tristate "Mixel MIPI DSI PHY support"
> + depends on OF
> + select GENERIC_PHY
> + select GENERIC_PHY_MIPI_DPHY
Since you converted to regmap, I guess you need:
select REGMAP_MMIO now?
> + help
> + Enable this to add support for the Mixel DSI PHY as found
> + on NXP's i.MX8 family of SOCs.
> diff --git a/drivers/phy/freescale/Makefile b/drivers/phy/freescale/Makefile
> index dc2b3f1f2f80..07491c926a2c 100644
> --- a/drivers/phy/freescale/Makefile
> +++ b/drivers/phy/freescale/Makefile
> @@ -1 +1,2 @@
> obj-$(CONFIG_PHY_FSL_IMX8MQ_USB) += phy-fsl-imx8mq-usb.o
> +obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY) += phy-fsl-imx8-mipi-dphy.o
> diff --git a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> new file mode 100644
> index 000000000000..4b182f2eaa6e
> --- /dev/null
> +++ b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> @@ -0,0 +1,494 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2017,2018 NXP
> + * Copyright 2019 Purism SPC
> + */
> +
> +#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/regmap.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_REG_BYPASS_PLL 0x4C
> +
> +#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) : \
Doesn't checkpatch complain about the need of space between operators?
> + ((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 */
active low is probably a better term.
> +#define PWR_ON 0
> +#define PWR_OFF 1
> +static inline u32 phy_read(struct phy *phy, unsigned int reg)
After the conversion to regmap this function is unused now and you
should remove it.
> +static inline void phy_write(struct phy *phy, u32 value, unsigned int reg)
No need for "inline".
Make it static int instead.
> +{
> + struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> + int ret;
> +
> + ret = regmap_write(priv->regs, reg, value);
Or maybe get rid of this function and use regmap_write() instead.
> + frequency = ref_clk * numerator / (2 * denominator);
> + dev_info(&phy->dev, "freq=%ld, hs_clk/ref_clk=%ld/%ld ⩰ %ld/%ld\n",
dev_dbg() would be better?
> + frequency, dphy_opts->hs_clk_rate, ref_clk,
> + numerator, denominator);
> +
> + /* LP clock period */
> + lp_t = 1000000000000L / dphy_opts->lp_clk_rate; /* ps */
> + dev_dbg(&phy->dev, "LP clock %lu, period: %lu ps\n",
> + dphy_opts->lp_clk_rate, lp_t);
> + /*
> + * hs_prepare: in lp clock periods
> + */
Please use single line comment style instead.
> + if (2 * dphy_opts->hs_prepare > 5 * lp_t) {
> + dev_err(&phy->dev,
> + "hs_prepare (%u) > 2.5 * lp clock period (%lu)",
> + dphy_opts->hs_prepare, lp_t);
> + return -EINVAL;
> + }
> + /* 00: lp_t, 01: 1.5 * lp_t, 10: 2 * lp_t, 11: 2.5 * lp_t */
> + if (dphy_opts->hs_prepare < lp_t)
> + n = 0;
> + else
> + n = 2 * (dphy_opts->hs_prepare - lp_t) / lp_t;
> + cfg->m_prg_hs_prepare = n;
> +
> + /*
> + * clk_prepare: in lp clock periods
> + */
Same here.
> + if (2 * dphy_opts->clk_prepare > 3 * lp_t) {
> + dev_err(&phy->dev,
> + "clk_prepare (%u) > 1.5 * lp clock period (%lu)",
> + dphy_opts->clk_prepare, lp_t);
> + return -EINVAL;
> + }
> + /* 00: lp_t, 01: 1.5 * lp_t */
> + cfg->mc_prg_hs_prepare = dphy_opts->clk_prepare > lp_t ? 1 : 0;
> +
> + /*
> + * hs_zero: forumula from NXP BSP
Typo: formula
> + */
> + n = (144 * (dphy_opts->hs_clk_rate / 1000000) - 47500) / 10000;
> + cfg->m_prg_hs_zero = n < 1 ? 1 : n;
> +
> + /*
> + * clk_zero: forumula from NXP BSP
Typo: formula
> +static int mixel_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> +{
> + struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> + struct mixel_dphy_cfg cfg = { 0 };
> + int ret;
> +
> + ret = mixel_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
> + if (ret)
> + return ret;
> +
> + /* Update the configuration */
> + memcpy(&priv->cfg, &cfg, sizeof(struct mixel_dphy_cfg));
> +
> + dev_dbg(&phy->dev, "Using CM:%u CN:%u CO:%u\n",
You have already printed CM, CN, CO above. No need to printed again.
> +static int mixel_dphy_power_on(struct phy *phy)
> +{
> + struct mixel_dphy_priv *priv = phy_get_drvdata(phy);
> + u32 locked;
> +
> + clk_prepare_enable(priv->phy_ref_clk);
clk_prepare_enable() may fail. Better do:
ret = clk_prepare_enable(priv->phy_ref_clk);
if (ret < 0)
return ret;
> +
> + phy_write(phy, PWR_ON, DPHY_PD_DPHY);
> + phy_write(phy, PWR_ON, DPHY_PD_PLL);
> +
> + if (regmap_read_poll_timeout(priv->regs, DPHY_LOCK, locked,
> + locked, 10, 1000) < 0) {
> + dev_err(&phy->dev, "Could not get DPHY lock!\n");
> + return -EINVAL;
Please add defines for 10 and 1000.
Better propagate the real error value here:
ret = regmap_read_poll_timeout(...)
if (ret) {
dev_err();
goto clock_disable
}
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENODEV;
You can remove the NULL check ...
> +
> + regs = devm_ioremap(dev, res->start, resource_size(res));
If you use devm_ioremap_resource() instead.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
