lkml.org 
[lkml]   [2019]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] phy: intel: Add driver support for combo phy
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

> +#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

> + 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?
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.

...

> + 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.

> +
> + 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_...;

?

> +}
> +
> +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.

> +}

...

> +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?

...

> +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.

> + 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.

> +{
> + 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?

> + 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?

...

> + " Mode mismatch lane0 : %u, lane1 : %u\n",

Extra leading space.

...

> +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.

> + struct fwnode_handle *fwn;

Better name is fwnode as done in plenty other drivers.

> + int i = 0, ret = 0;

i = 0 better to have near to its user.
ret = 0 is redundant assignment.


> + 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?

> +
> + 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(...);

...

> + 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.

> + 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_...();

...

> +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?

--
With Best Regards,
Andy Shevchenko


\
 
 \ /
  Last update: 2019-12-20 11:46    [W:0.073 / U:2.768 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site