| Message ID | cover.1582709320.git.eswara.kota@linux.intel.com |
|---|---|
| Headers | show |
| Series | Add Intel ComboPhy driver | expand |
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);
On Wed, Feb 26, 2020 at 04:41:47PM +0200, Andy Shevchenko wrote: > On Wed, Feb 26, 2020 at 06:09:53PM +0800, Dilip Kota wrote: > > + 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... Ah, dismiss this, I forgot the fwnode_handle_put() part along with amount of accessible children. > > + 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); > > +}
Thanks Andy for reviewing and giving the inputs. I will update them as per your comments, but for couple of cases of i have a different opinion. Please check and give your inputs. On 2/26/2020 10:41 PM, Andy Shevchenko wrote: > 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? Yes, OF is not required. I will remove it. > >> + select MFD_SYSCON >> + select GENERIC_PHY >> + select REGMAP > ... > >> + * Copyright (C) 2019 Intel Corporation. > 2019-2020 My bad. I will update it. > > ... > ... >> +}; >> + >> +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. Sure i will remove them. To be meaningful, i will remove the max entry for the enums representing the value of register bitfields. ... > ... > >> +static int intel_cbphy_iphy_dt_parse(struct intel_combo_phy *cbphy, > dt -> fwnode > Ditto for other similar function names. Sure, it looks appropriate for intel_cbphy_iphy_dt_parse() -> intel_cbphy_iphy_fwnode_parse(). Whereas for intel_cbphy_dt_parse() i will keep it unchanged, because it is calling devm_*, devm_platform_*, fwnode_* APIs to traverse dt node. > >> + 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. I will add it. Thanks for pointing it. ... > ... > >> + 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: In the v2 patch, for int i = 0 you mentioned to do initialization at the user, instead of doing at declaration. So i followed the same for "pdev" and "fwnode" which are being used after few lines of the code . It looked good in the perspective of code readability. > > 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. > > Regards, Dilip
On Thu, Feb 27, 2020 at 9:54 AM Dilip Kota <eswara.kota@linux.intel.com> wrote: > Thanks Andy for reviewing and giving the inputs. > I will update them as per your comments, but for couple of cases of i > have a different opinion. Please check and give your inputs. ... > >> +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. > > Sure i will remove them. > > To be meaningful, i will remove the max entry for the enums representing > the value of register bitfields. It will work. ... > >> +static int intel_cbphy_iphy_dt_parse(struct intel_combo_phy *cbphy, > > dt -> fwnode > > Ditto for other similar function names. > Sure, it looks appropriate for intel_cbphy_iphy_dt_parse() -> > intel_cbphy_iphy_fwnode_parse(). > Whereas for intel_cbphy_dt_parse() i will keep it unchanged, because it > is calling devm_*, devm_platform_*, fwnode_* APIs to traverse dt node. How do you know that it will be DT node? I can't say it from the function parameters: Is any of them takes of_node? > >> + struct fwnode_handle *fwnode, int idx) ... > >> + 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: > In the v2 patch, for int i = 0 you mentioned to do initialization at the > user, instead of doing at declaration. > So i followed the same for "pdev" and "fwnode" which are being used > after few lines of the code . It looked good in the perspective of code > readability. No, it is different. For the loop counter is better to have closer to the loop, for the more global thingy like platform device it makes it actually harder to find. When you do assignments you have to think about the variable meaning and scope. Scope is different for loop counter versus the mentioned rest. > > 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.
On 2/27/2020 5:43 PM, Andy Shevchenko wrote: > On Thu, Feb 27, 2020 at 9:54 AM Dilip Kota <eswara.kota@linux.intel.com> wrote: > ... >>>> +static int intel_cbphy_iphy_dt_parse(struct intel_combo_phy *cbphy, >>> dt -> fwnode >>> Ditto for other similar function names. >> Sure, it looks appropriate for intel_cbphy_iphy_dt_parse() -> >> intel_cbphy_iphy_fwnode_parse(). >> Whereas for intel_cbphy_dt_parse() i will keep it unchanged, because it >> is calling devm_*, devm_platform_*, fwnode_* APIs to traverse dt node. > How do you know that it will be DT node? > I can't say it from the function parameters: Is any of them takes of_node? Got it, All the functions are traversing through device only. I will change intel_cbphy_dt_parse() to intel_cbphy_fwnode_parse(). (PS: My intention is something different. As the function is fetching device tree node entries so kept is as *_dt_parse() ) > >>>> + struct fwnode_handle *fwnode, int idx) > ... > >>>> + 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: >> In the v2 patch, for int i = 0 you mentioned to do initialization at the >> user, instead of doing at declaration. >> So i followed the same for "pdev" and "fwnode" which are being used >> after few lines of the code . It looked good in the perspective of code >> readability. > No, it is different. For the loop counter is better to have closer to > the loop, for the more global thingy like platform device it makes it > actually harder to find. > When you do assignments you have to think about the variable meaning > and scope. Scope is different for loop counter versus the mentioned > rest. Understand. I will follow the same and keep a note for future drivers too. Thanks for detail explanation. Regards, Dilip >> .