lkml.org 
[lkml]   [2020]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 3/3] phy: intel: Add driver support for Combophy
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


\
 
 \ /
  Last update: 2020-02-26 15:42    [W:1.094 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site