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

["Guido Günther" <agx@sigxcpu.org>]

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@sigxcpu.org> 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.
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel