Re: [PATCH 2/2] phy: Add driver for mixel dphy

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

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

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