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