Re: [PATCH v2 3/3] phy: Add driver for mixel dphy found on imx8

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux