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
