Hi Sam,
On Fri, Jan 25, 2019 at 05:53:55PM +0100, Sam Ravnborg wrote:
[..snip..]
> > +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.
These are actual register names so I added comment just saying that.
[..snip..]
> > +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.
Due to the long phy_configure_opts_mipi_dpy this whole hit the 80 char
limit so I left it like that to make checkpatch happy.
>
> > + 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()
Using regmap I could drop the lock entirely.
[..snip..]
> > +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.
Yeah, that's true. I found in another driver other imx8 variants have
different register offsets so I went for register offsets in devdata
rather than function pointer table which make this ambiguity go away too.
I hope I have tackled all your other comments.
Thanks!
-- Guido
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
