Re: [PATCH v2 8/9] phy: Add Cadence D-PHY support

[Maxime Ripard]

Hi!

On Mon, Nov 12, 2018 at 03:21:56PM +0530, Kishon Vijay Abraham I wrote:
> > +static int cdns_dphy_validate(struct phy *phy, enum phy_mode mode,
> > +			      union phy_configure_opts *opts)
> > +{
> > +	struct cdns_dphy_cfg cfg = { 0 };
> > +
> > +	if (mode != PHY_MODE_MIPI_DPHY)
> > +		return -EINVAL;
> > +
> > +	return cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
> > +}
> > +
> > +static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> > +{
> > +	struct cdns_dphy *dphy = phy_get_drvdata(phy);
> > +	struct cdns_dphy_cfg cfg = { 0 };
> > +	int ret;
> > +
> > +	ret = cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
> > +	if (ret)
> > +		return ret;
> 
> Can you explain why you need the same function to be invoked from both validate
> and configure callback? I see this to be redundant.

Sure.

Validate and configure serve two rather different purposes. validate
is here to make sure that a configuration can work with the PHY, and
to let the phy adjust the configuration to find a more optimal one.

configure, on the other hand, apply a configuration. We still have to
make sure that the configuration can work, since:
  - We might have called validate any number of times, with any number
    of configurations before calling configure, so we don't know which
    configuration we validate is actually going to be applied later on
    (if it's even applied)
  - If we don't care about the validation at all, we might just call
    configure directly

Does that make sense?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachment: signature.asc
Description: PGP signature