Re: [PATCH v3 3/3] phy: intel: Add driver support for Combophy

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Feb 26, 2020 at 06:09:53PM +0800, Dilip Kota wrote:
> Combophy subsystem provides PHYs for various
> controllers like PCIe, SATA and EMAC.

Thanks for an update, my comments below.

...

> +config PHY_INTEL_COMBO
> +	bool "Intel Combo PHY driver"

> +	depends on OF && HAS_IOMEM && (X86 || COMPILE_TEST)

I guess it would be better to have like this:

	depends on X86 || COMPILE_TEST
	depends on OF && HAS_IOMEM

But do you still have a dependency to OF?

> +	select MFD_SYSCON
> +	select GENERIC_PHY
> +	select REGMAP

...

> + * Copyright (C) 2019 Intel Corporation.

2019-2020

...

> +static const unsigned long intel_iphy_clk_rates[] = {
> +	CLK_100MHZ, CLK_156_25MHZ, CLK_100MHZ

It's good to have comma at the end since this might potentially be extended in
the future.

> +};
> +
> +enum {
> +	PHY_0,
> +	PHY_1,

> +	PHY_MAX_NUM,

But here we don't need it since it's a terminator line.
Ditto for the rest of enumerators with a terminator / max entry.

> +};

...

> +/*
> + * Clock register bitfields to enable clocks
> + * for ComboPhy according to the mode

For multi-line comments use proper English punctuation.

> + */

...

> +static int intel_cbphy_pcie_refclk_cfg(struct intel_cbphy_iphy *iphy, bool set)
> +{
> +	struct intel_combo_phy *cbphy = iphy->parent;
> +	u32 mask = BIT(cbphy->id * 2 + iphy->id);

> +	const u32 pad_dis_cfg_off = 0x174;

This has to be defined in the top.

> +	u32 val;
> +
> +	/* Register: 0 is enable, 1 is disable */
> +	val = set ? 0 : mask;
> +
> +	return regmap_update_bits(cbphy->syscfg, pad_dis_cfg_off, mask, val);
> +}

...

> +	ret = phy_cfg(sphy);
> +
> +	return ret;

	return phy_cfg();

...

> +	/*
> +	 * No way to identify gen1/2/3/4 for mppla and mpplb, just delay
> +	 * for stable plla(gen1/gen2) or pllb(gen3/gen4)
> +	 */

Use proper abbreviation rules, i.e. capitalize appropriately. (I think at least
PLL is quite common way to do it, the rest depends to the documentation / your
intentions).


...

> +	ret = reset_control_assert(iphy->app_rst);
> +	if (ret) {
> +		dev_err(dev, "PHY(%u:%u) core assert failed!\n",
> +			COMBO_PHY_ID(iphy), PHY_ID(iphy));
> +		return ret;
> +	}
> +
> +	ret = intel_cbphy_iphy_enable(iphy, false);
> +	if (ret) {
> +		dev_err(dev, "Failed disabling Phy core\n");

Phy -> PHY for sake of the consistency

> +		return ret;
> +	}

...

> +	/* Wait RX adaptation to finish */
> +	ret = readl_poll_timeout(cr_base + CR_ADDR(PCS_XF_RX_ADAPT_ACK, id),
> +				 val, val & RX_ADAPT_ACK_BIT, 10, 5000);
> +	if (ret)
> +		dev_err(dev, "RX Adaptation failed!\n");
> +	else

> +		dev_info(dev, "RX Adaptation success!\n");

Sounds more likely like dev_dbg().

...

> +static int intel_cbphy_iphy_dt_parse(struct intel_combo_phy *cbphy,

dt -> fwnode
Ditto for other similar function names.

> +				     struct fwnode_handle *fwnode, int idx)
> +{

> +	dev = get_dev_from_fwnode(fwnode);

I don't see where you drop reference count to the struct device object.

> +}

...

> +	if (!iphy0->enable && !iphy1->enable) {

	if (!(iphy0->enable || iphy1->enable)) {

?

> +		dev_err(cbphy->dev, "CBPHY%u both PHY disabled!\n", cbphy->id);

> +		return -EINVAL;

-ENODEV ?

> +	}
> +
> +	if (cbphy->aggr_mode == PHY_DL_MODE &&
> +	    (!iphy0->enable || !iphy1->enable)) {

	if (cbphy->aggr_mode == PHY_DL_MODE && !(iphy0->enable && iphy1->enable)) {

?

> +		dev_err(cbphy->dev,
> +			"Dual lane mode but lane0: %s, lane1: %s\n",
> +			iphy0->enable ? "on" : "off",
> +			iphy1->enable ? "on" : "off");
> +		return -EINVAL;
> +	}

...

> +	struct fwnode_reference_args ref;
> +	struct device *dev = cbphy->dev;
> +	struct fwnode_handle *fwnode;
> +	struct platform_device *pdev;
> +	int i, ret;
> +	u32 prop;

I guess the following would be better:

	struct device *dev = cbphy->dev;
	struct platform_device *pdev = to_platform_device(dev);
	struct fwnode_handle *fwnode = dev_fwnode(dev);
	struct fwnode_reference_args ref;
	int i, ret;
	u32 prop;

> +	pdev = to_platform_device(dev);

See above.

> +	fwnode = dev_fwnode(dev);

See above.

> +	ret = fwnode_property_read_u32_array(fwnode, "intel,phy-mode", &prop, 1);
> +	if (ret)
> +		return ret;
> +
> +	if (prop >= PHY_MAX_MODE) {
> +		dev_err(dev, "Invalid phy mode: %u\n", prop);
> +		return -EINVAL;
> +	}
> +
> +	cbphy->phy_mode = prop;

I don't see why you need temporary variable at all?


...

> +	i = 0;
> +	fwnode_for_each_available_child_node(dev_fwnode(dev), fwnode) {

> +		if (i >= PHY_MAX_NUM) {
> +			fwnode_handle_put(fwnode);
> +			dev_err(dev, "Error: DT child number larger than %d\n",
> +				PHY_MAX_NUM);
> +			return -EINVAL;
> +		}

Logically this part is better to put after i++; line...

> +		ret = intel_cbphy_iphy_dt_parse(cbphy, fwnode, i);
> +		if (ret) {
> +			fwnode_handle_put(fwnode);
> +			return ret;
> +		}
> +

> +		i++;

...here.

> +	}
> +
> +	return intel_cbphy_dt_sanity_check(cbphy);
> +}

...

> +	.init =		intel_cbphy_init,
> +	.exit =		intel_cbphy_exit,
> +	.calibrate =	intel_cbphy_calibrate,
> +	.owner =	THIS_MODULE,

Usual way is to format by =, i.e.

	.init		= intel_cbphy_init,
	.exit		= intel_cbphy_exit,
	.calibrate	= intel_cbphy_calibrate,
	.owner		= THIS_MODULE,

...

> +	for (i = 0; i < PHY_MAX_NUM; i++) {
> +		iphy = &cbphy->iphy[i];

> +		if (iphy->enable) {

		if (!iphy->enable)
			continue;

?

> +			ret = intel_cbphy_iphy_create(iphy);
> +			if (ret)
> +				return ret;
> +		}
> +	}

...

> +	platform_set_drvdata(pdev, cbphy);

I guess it makes sense a bit later to do...

> +	ret = intel_cbphy_dt_parse(cbphy);
> +	if (ret)
> +		return ret;

...here?

> +
> +	return intel_cbphy_create(cbphy);

-- 
With Best Regards,
Andy Shevchenko