Date:   Fri, 27 Dec 2019 15:56:35 +0800
From:   Dilip Kota <eswara.kota@...ux.intel.com>
To:     Andy Shevchenko <andriy.shevchenko@...el.com>
Cc:     linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        kishon@...com, robh@...nel.org, cheol.yong.kim@...el.com,
        chuanhua.lei@...ux.intel.com, qi-ming.wu@...el.com,
        yixin.zhu@...el.com
Subject: Re: [PATCH 2/2] phy: intel: Add driver support for combo phy


On 12/20/2019 6:45 PM, Andy Shevchenko wrote:
> On Fri, Dec 20, 2019 at 03:28:28PM +0800, Dilip Kota wrote:
>> Combo phy subsystem provides PHYs for various
>> controllers like PCIe, SATA and EMAC.
> ...
>
>> +#define REG_COMBO_MODE(x)	((x) * 0x200)
> Perhaps  + 0x000
Yes, but i think not required to add explicitly.
>
>> +#define REG_CLK_DISABLE(x)	((x) * 0x200 + 0x124)
> ...
>
>> +static const char *const intel_iphy_names[] = {"pcie", "xpcs", "sata"};
>> +static const unsigned long intel_iphy_clk_rate[] = {
> names (note S)
> rate -> rates
Ok, will correct it to rates.
>
>> +	CLK_100MHZ, CLK_156_25MHZ, CLK_100MHZ
>> +};
> ...
>
>> +static ssize_t intel_cbphy_info_show(struct device *dev,
>> +				     struct device_attribute *attr, char *buf)
>> +{
>> +	struct intel_combo_phy *cbphy;
>> +	int i, off;
>> +
>> +	cbphy = dev_get_drvdata(dev);
>> +
>> +	off = sprintf(buf, "mode: %u\n", cbphy->mode);
>> +
>> +	off += sprintf(buf + off, "aggr mode: %s\n",
>> +		      cbphy->aggr_mode == PHY_DL_MODE ? "Yes" : "No");
> Can't you do
>
> static inline const char *yesno(bool choice)
> {
> 	return choice ? "Yes" : "No";
> }
>
> and use it here and below?
>
> Somebody already shared the idea that the above helper should be available
> globally.
>
>> +
>> +	off += sprintf(buf + off, "capability: ");
>> +	for (i = PHY_PCIE_MODE; i < PHY_MAX_MODE; i++) {
>> +		if (BIT(i) & cbphy->phy_cap)
>> +			off += sprintf(buf + off, "%s ", intel_iphy_names[i]);
>> +	}
>> +
>> +	off += sprintf(buf + off, "\n");
>> +
>> +	for (i = 0; i < PHY_MAX_NUM; i++) {
>> +		off += sprintf(buf + off, "PHY%d mode: %s, enable: %s\n",
>> +			       i, intel_iphy_names[cbphy->iphy[i].phy_mode],
>> +			       cbphy->iphy[i].enable ? "Yes" : "No");
>> +	}
>> +
>> +	return off;
>> +}
> ...
>
>> +static struct attribute *intel_cbphy_attrs[] = {
>> +	&dev_attr_intel_cbphy_info.attr,
>> +	NULL,
> Comma is redundant for terminator lines.
>
>> +};
>
>> +static int intel_cbphy_sysfs_init(struct intel_combo_phy *cbphy)
>> +{
>> +	return devm_device_add_groups(cbphy->dev, intel_cbphy_groups);
>> +}
> What the point?
For debug purpose only... can be removed in upstream. I will clean it up 
in next patch version.
> Moreover, can't you use .dev_groups member of struct device_driver?
>
> ...
>
>> +		ret =  phy_cfg(sphy);
> In several places you have extra unneeded white spaces.
>
> ...
>
>> +	combo_phy_w32_off_mask(iphy->app_base, PCIE_PHY_CLK_PAD,
>> +			       0, PCIE_PHY_GEN_CTRL);
> Configure your editor properly! There is plenty of room on the previous line.
Sure, will fix at all the places.
>
> ...
>
>> +	combo_phy_w32_off_mask(iphy->app_base, PCIE_PHY_CLK_PAD,
>> +			       1, PCIE_PHY_GEN_CTRL);
> Ditto.
>
> ...
>
>> +static int intel_cbphy_init(struct phy *phy)
>> +{
>> +	struct intel_cbphy_iphy *iphy;
>
>> +	int ret = 0;
> Redundant assignment. See below.
Sure, will fix it.
>
>> +
>> +	iphy = phy_get_drvdata(phy);
>> +
>> +	if (iphy->phy_mode == PHY_PCIE_MODE) {
>> +		ret = intel_cbphy_iphy_cfg(iphy,
>> +					   intel_cbphy_pcie_en_pad_refclk);
>> +	}
>> +
>> +	if (!ret)
>> +		ret = intel_cbphy_iphy_cfg(iphy, intel_cbphy_iphy_power_on);
>> +
>> +	return ret;
> Why not to simple do
>
> 	if (A) {
> 		ret = ...;
> 		if (ret)
> 			return ret;
> 	}
>
> 	return intel_...;
Looks good, i will update.
>
> ?
>
>> +}
>> +
>> +static int intel_cbphy_exit(struct phy *phy)
>> +{
>> +	struct intel_cbphy_iphy *iphy;
>> +	int ret = 0;
>> +
>> +	iphy = phy_get_drvdata(phy);
>> +
>> +	if (iphy->power_en)
>> +		ret = intel_cbphy_iphy_cfg(iphy, intel_cbphy_iphy_power_off);
>> +
>> +	if (!ret && iphy->phy_mode == PHY_PCIE_MODE)
>> +		ret = intel_cbphy_iphy_cfg(iphy,
>> +					   intel_cbphy_pcie_dis_pad_refclk);
>> +
>> +	return ret;
> Ditto.
Ok
>
>> +}
> ...
>
>> +static int intel_cbphy_iphy_mem_resource(struct intel_cbphy_iphy *iphy)
>> +{
>> +	void __iomem *base;
>> +
>> +	base = devm_platform_ioremap_resource(iphy->pdev, 0);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	iphy->app_base = base;
>> +
>> +	return 0;
>> +}
> What's the point of this helper?
Defined as separate function for traversing memory entry from DT.
Similarly get_clks() and get_reset() functions.
>
> ...
>
>> +static int intel_cbphy_iphy_get_clks(struct intel_cbphy_iphy *iphy)
>> +{
>> +	enum intel_phy_mode mode = iphy->phy_mode;
>> +	struct device *dev = iphy->dev;
>> +	int ret = 0;
> Redundant. Simple return 0 explicitly at the end.
> Ditto for other places in this patch.
Ok, i will fix at all the places.
>
>> +	if (IS_ERR(iphy->freq_clk)) {
>> +		ret = PTR_ERR(iphy->freq_clk);
>> +		if (ret != -EPROBE_DEFER) {
>> +			dev_err(dev, "PHY[%u:%u] No %s freq clock\n",
>> +				COMBO_PHY_ID(iphy), PHY_ID(iphy),
>> +				intel_iphy_names[mode]);
>> +		}
>> +
>> +		return ret;
>> +	}
>> +
>> +	iphy->clk_rate = intel_iphy_clk_rate[mode];
>> +
>> +	return ret;
>> +}
> ...
>
>> +static int intel_cbphy_iphy_dt_parse(struct intel_combo_phy *cbphy,
>> +				     struct fwnode_handle *fwn, int idx)
> fwn -> fwnode.
Sure, i will replace it.
>
>> +{
>> +	struct intel_cbphy_iphy *iphy = &cbphy->iphy[idx];
>> +	struct platform_device *pdev;
>> +	struct device *dev;
>> +	int ret = 0;
>> +	u32 prop;
>> +
>> +	iphy->id = idx;
>> +	iphy->enable = false;
>> +	iphy->power_en = false;
>> +	iphy->parent = cbphy;
>> +	iphy->np = to_of_node(fwn);
>> +	pdev = of_find_device_by_node(iphy->np);
> Why? Can't it be done simpler?
There is no direct helper function to get platform device from fwnode,
I will simplify it to get  fwnode->device-> platform device. However 
iphy->np is no longer required.

>
>> +	if (!pdev) {
>> +		dev_warn(cbphy->dev, "Combo-PHY%u: PHY device: %d disabled!\n",
>> +			 cbphy->id, idx);
>> +		return 0;
>> +	}
>> +	if (!(BIT(iphy->phy_mode) & cbphy->phy_cap)) {
> Yoda style?

I will correct it to ...

if (!(cbphy->phy_cap & BIT(iphy->phy_node)))

>
> ...
>
>> +				" Mode mismatch lane0 : %u, lane1 : %u\n",
> Extra leading space.
Sure, i will fix it.
>
> ...
>
>> +static int intel_cbphy_dt_parse(struct intel_combo_phy *cbphy)
>> +{
>> +	struct device *dev = cbphy->dev;
>> +	struct device_node *np = dev->of_node;
> Why do you need this one? You have to device if it's OF centric driver or not.
Used during syscon_regmap call.
>
>> +	struct fwnode_handle *fwn;
> Better name is fwnode as done in plenty other drivers.
Sure will fix it.
>
>> +	int i = 0, ret = 0;
> i = 0 better to have near to its user.
> ret = 0 is redundant assignment.
Sure, will fix it.
>
>
>> +	ret = device_property_read_u32(dev, "intel,bid", &cbphy->bid);
>> +	if (ret) {
>> +		dev_err(dev, "NO intel,bid provided!\n");
>> +		return ret;
>> +	}
>> +
>> +	device_for_each_child_node(dev, fwn) {
>> +		if (i >= PHY_MAX_NUM) {
>> +			fwnode_handle_put(fwn);
>> +			dev_err(dev, "Error: DT child number larger than %d\n",
>> +				PHY_MAX_NUM);
>> +			return -EINVAL;
>> +		}
>> +
>> +		ret = intel_cbphy_iphy_dt_parse(cbphy, fwn, i);
>> +		if (ret) {
>> +			fwnode_handle_put(fwn);
>> +			return ret;
>> +		}
>> +
>> +		i++;
>> +	}
>> +
>> +	return intel_cbphy_dt_sanity_check(cbphy);
>> +}
> ...
>
>> +	regmap_write(cbphy->hsiocfg, REG_COMBO_MODE(cbphy->bid), cb_mode);
> No error check?
Sure, will add it.
>
>> +
>> +	return 0;
> ...
>
>> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>> +	if (IS_ERR(phy_provider)) {
>> +		dev_err(dev, "PHY[%u:%u]: register phy provider failed!\n",
>> +			COMBO_PHY_ID(iphy), PHY_ID(iphy));
>> +		return PTR_ERR(phy_provider);
>> +	}
>> +
>> +	return 0;
> return PTR_ERR_OR_ZERO(...);
>
> ...
I will update it.
>
>> +	ret = of_property_read_u32(dev->of_node, "cell-index", &id);
> You should decide either you go with OF centric API(s) or with device property
> one as below.

I missed to change to device property.

I will correct it.

>
>> +	if (!device_property_read_bool(dev, "intel,cap-pcie-only"))
>> +		cbphy->phy_cap |= PHY_XPCS_CAP | PHY_SATA_CAP;
> ...
>
>> +	ret = intel_cbphy_sysfs_init(cbphy);
>> +
>> +	return ret;
> return intel_...();
Sure, will update it.
>
> ...
>
>> +static struct platform_driver intel_cbphy_driver = {
>> +	.probe = intel_cbphy_probe,
>> +	.driver = {
>> +		.name = "intel-combo-phy",
>> +		.of_match_table = of_intel_cbphy_match,
>> +	}
>> +};
>> +
>> +builtin_platform_driver(intel_cbphy_driver);
> Can we unbound it? Is it okay to do unbind/bind cycle? Had it been tested for
> that?

Unbound can be done here, i will add the respective code.

Thanks a lot for reviewing and providing the inputs.
Regards,
Dilip

>

Powered by blists - more mailing lists