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

[Maxime Ripard]

Hi Kishon,

On Wed, Nov 21, 2018 at 03:59:43PM +0530, Kishon Vijay Abraham I wrote:
> On 21/11/18 3:41 PM, Maxime Ripard wrote:
> > Hi Kishon,
> > 
> > On Tue, Nov 20, 2018 at 11:02:34AM +0530, Kishon Vijay Abraham I wrote:
> >>>> +static int cdns_dphy_config_from_opts(struct phy *phy,
> >>>> +				      struct phy_configure_opts_mipi_dphy *opts,
> >>>> +				      struct cdns_dphy_cfg *cfg)
> >>>> +{
> >>>> +	struct cdns_dphy *dphy = phy_get_drvdata(phy);
> >>>> +	unsigned int dsi_hfp_ext = 0;
> >>>> +	int ret;
> >>>> +
> >>>> +	ret = phy_mipi_dphy_config_validate(opts);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	ret = cdns_dsi_get_dphy_pll_cfg(dphy, cfg,
> >>>> +					opts, &dsi_hfp_ext);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	opts->wakeup = cdns_dphy_get_wakeup_time_ns(dphy);
> >>
> >> Is the wakeup populated here used by the consumer driver?
> > 
> > It's supposed to, if it can yes.
> 
> But I guess right now it's not using. I'm thinking the usefulness of validate
> callback (only from this series point of view). Why should a consumer driver
> invoke validate if it doesn't intend to configure the PHY?
> 
> The 3 steps required are
> 	* The consumer driver gets the default config
> 	* The consumer driver changes some of the configuration and
> 	* The consumer driver invokes phy configure callback.
> 
> phy_configure anyways can validate the config before actually configuring the phy.

If you only want to configure the PHY, then yes, sure.

However, the point here is that validate helps negotiating what the
source device (DSI controller for example) and what the sink device
(say a panel) are capable of.

A panel for example might very well have multiple resolutions that it
supports, and the DSI controller will have some as well. And the PHY
will only be able to operate within certain boundaries too. However,
they don't necessarily match, since there's so many panels, and so
many SoCs.

Let's say our (very weird) panel is able to do 640x480, 1024x768 and
1280x800. Our DSI driver can only operate with 1024 pixels in both
dimensions, and the PHY can only reach the bus frequency needed for
around 800x600.

We'll want to filter out 1280x800 (because the DSI controller can't
provide that) and 1024x768 (because the PHY can go fast enough), to
only provide the 640x480 option to the user.

That's what validate bring you. The option to test whether a given
configuration *would* work, without actually wanting to apply it right
now.

Does that make sense?

Maxime

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

Attachment: signature.asc
Description: PGP signature