Hi,
On Fri, Feb 01, 2019 at 09:26:50AM -0200, Fabio Estevam wrote:
> Hi Guido,
>
> Thanks for the respin. It looks better :-)
Thanks for having a look! All your comments made sense to me and I've
folded them into v3.
Cheers,
-- Guido
>
> 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
