Add Intel ComboPhy driver
Subsystems (4)
v3 cover Add Intel ComboPhy driver
This patch series adds Intel Combophy driver, respective yaml schemas and a kernel API fwnode_to_regmap().
ComboPhy driver calls it to get regmap handle through firmware node.
Dilip Kota (3):
mfd: syscon: Add fwnode_to_regmap
dt-bindings: phy: Add YAML schemas for Intel Combophy
phy: intel: Add driver support for Combophy
.../devicetree/bindings/phy/intel,combo-phy.yaml | 123 ++++ drivers/mfd/syscon.c | 8 + drivers/phy/intel/Kconfig | 13 + drivers/phy/intel/Makefile | 1 + drivers/phy/intel/phy-intel-combo.c | 672 +++++++++++++++++++++ include/dt-bindings/phy/phy-intel-combophy.h | 10 + include/linux/mfd/syscon.h | 6 + 7 files changed, 833 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml create mode 100644 drivers/phy/intel/phy-intel-combo.c create mode 100644 include/dt-bindings/phy/phy-intel-combophy.h
Signature · 1 line
2.11.0
PATCH v3 1/3 mfd: syscon: Add fwnode_to_regmap
Traverse regmap handle entry from firmware node handle.
--- drivers/mfd/syscon.c | 8 ++++++++ include/linux/mfd/syscon.h | 6 ++++++ 2 files changed, 14 insertions(+)
drivers/mfd/syscon.c +8 -0
@@ -178,6 +178,14 @@ struct regmap *device_node_to_regmap(struct device_node *np) }EXPORT_SYMBOL_GPL (device_node_to_regmap); +struct regmap *fwnode_to_regmap (struct fwnode_handle *fwnode) +{ +struct device_node *np =to_of_node (fwnode); + +return device_node_get_regmap (np, false); +} +EXPORT_SYMBOL_GPL (fwnode_to_regmap); +struct regmap *syscon_node_to_regmap (struct device_node *np) {if (!of_device_is_compatible (np,"syscon" ))
include/linux/mfd/syscon.h +6 -0
@@ -18,6 +18,7 @@ struct device_node;#ifdef CONFIG_MFD_SYSCON extern struct regmap *device_node_to_regmap (struct device_node *np); +extern struct regmap *fwnode_to_regmap (struct fwnode_handle *fwnode);extern struct regmap *syscon_node_to_regmap (struct device_node *np);extern struct regmap *syscon_regmap_lookup_by_compatible (const char *s); extern struct regmap *syscon_regmap_lookup_by_phandle(
@@ -29,6 +30,11 @@ static inline struct regmap *device_node_to_regmap(struct device_node *np)return ERR_PTR (-ENOTSUPP ); } +static inline struct regmap *fwnode_to_regmap (struct fwnode_handle *fwnode) +{ +return ERR_PTR (-ENOTSUPP ); +} +static inline struct regmap *syscon_node_to_regmap (struct device_node *np) {return ERR_PTR (-ENOTSUPP );
Signature · 1 line
2.11.0
PATCH v3 2/3 dt-bindings: phy: Add YAML schemas for Intel Combophy
In reply to Dilip KotaCombophy subsystem provides PHY support to various controllers, viz. PCIe, SATA and EMAC. Adding YAML schemas for the same.
---
Changes on v3:
Add include/dt-bindings/phy/phy-intel-combphy.h for phy modes. Add SoC specific compatible "intel,combophy-lgm".
Correct the nodename pattern.
clocks description removed and add maxItems entry.
Remove "simple-bus" as it expects minimum one address
cell and size cell in the children node. Call devm_of_platform_populate() in the driver to perform "simple-bus" functionality.
Changes on v2:
Add custom 'select'
Pass hardware instance entries with phandles and
remove cell-index and bid entries
Clock, register address space, are same for the children.
So move them to parent node.
Two PHY instances cannot run in different modes,
so move the phy-mode entry to parent node.
Add second child entry in the DT example.
.../devicetree/bindings/phy/intel,combo-phy.yaml | 123 +++++++++++++++++++++ include/dt-bindings/phy/phy-intel-combophy.h | 10 ++ 2 files changed, 133 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml create mode 100644 include/dt-bindings/phy/phy-intel-combophy.h
Documentation/devicetree/bindings/phy/intel,combo-phy.yaml new file +123 -0
@@ -0,0 +1,123 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/phy/intel,combo-phy.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel ComboPhy Subsystem + +maintainers: + - Dilip Kota <[email protected]> + +description: | + Intel Combophy subsystem supports PHYs for PCIe, EMAC and SATA + controllers. A single Combophy provides two PHY instances. + +properties: + $nodename: + pattern: "combophy(@.*|-[0-9a-f])*$" + + compatible: + items: + - const: intel,combophy-lgm + - const: intel,combo-phy + + clocks: + maxItems: 1 + + reg: + items: + - description: ComboPhy core registers + - description: PCIe app core control registers + + reg-names: + items: + - const: core + - const: app + + resets: + maxItems: 2 + + reset-names: + items: + - const: phy + - const: core + + intel,syscfg: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: Chip configuration registers handle and ComboPhy instance id + + intel,hsio: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: HSIO registers handle and ComboPhy instance id on NOC + + intel,aggregation: + type: boolean + description: | + Specify the flag to configure ComboPHY in dual lane mode. + + intel,phy-mode: + $ref: /schemas/types.yaml#/definitions/uint32 + description: | + Mode of the two phys in ComboPhy. + See dt-bindings/phy/phy-intel-combophy.h for values. + +patternProperties: + "^phy@[0-9]+$": + type: object + + properties: + compatible: + const: intel,phydev + + "#phy-cells": + const: 0 + + resets: + description: | + reset handle according to the PHY mode. + See ../reset/reset.txt for details. + + required: + - compatible + - "#phy-cells" + +required: + - compatible + - clocks + - reg + - reg-names + - intel,syscfg + - intel,hsio + - intel,phy-mode + +additionalProperties: false + +examples: + - | + #include <dt-bindings/phy/phy-intel-combophy.h> + combophy@d0a00000 { + compatible = "intel,combophy-lgm", "intel,combo-phy"; + clocks = <&cgu0 1>; + reg = <0xd0a00000 0x40000>, + <0xd0a40000 0x1000>; + reg-names = "core", "app"; + resets = <&rcu0 0x50 6>, + <&rcu0 0x50 17>; + reset-names = "phy", "core"; + intel,syscfg = <&sysconf 0>; + intel,hsio = <&hsiol 0>; + intel,phy-mode = <COMBO_PHY_PCIE>; + + phy@0 { + compatible = "intel,phydev"; + #phy-cells = <0>; + resets = <&rcu0 0x50 23>; + }; + + phy@1 { + compatible = "intel,phydev"; + #phy-cells = <0>; + resets = <&rcu0 0x50 24>; + }; + };
include/dt-bindings/phy/phy-intel-combophy.h new file +10 -0
@@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef _DT_BINDINGS_INTEL_COMBOPHY +#define _DT_BINDINGS_INTEL_COMBOPHY + +#define COMBO_PHY_PCIE 0 +#define COMBO_PHY_XPCS 1 +#define COMBO_PHY_SATA 2 + +#endif /* _DT_BINDINGS_INTEL_COMBOPHY*/
Signature · 1 line
2.11.0
PATCH v3 3/3 phy: intel: Add driver support for Combophy
In reply to Dilip KotaCombophy subsystem provides PHYs for various
controllers like PCIe, SATA and EMAC.
---
Changes on v3:
Use fwnode_to_regmap()
Call devm_of_platform_populate() to populate child nodes Add SoC specific compatible "intel,combophy-lgm" Remove intel_iphy_names
Remove struct phy entry in struct intel_cbphy_iphy Imporve if conditions logic
Add description for enums
Remove default case in switch {} intel_cbphy_set_mode() as it
never happens.
Use mutex_lock to synchronise ComboPhy initialization across
two phys.
Change init_cnt to u32 datatype as it is within mutex lock. Correct error handling of
fwnode_property_read_u32_array(fwnode, "intel,phy-mode", ...)
drivers/phy/intel/Kconfig | 13 + drivers/phy/intel/Makefile | 1 + drivers/phy/intel/phy-intel-combo.c | 672 ++++++++++++++++++++++++++++++++++++ 3 files changed, 686 insertions(+) create mode 100644 drivers/phy/intel/phy-intel-combo.c
drivers/phy/intel/Kconfig +13 -0
@@ -2,6 +2,19 @@ # # Phy drivers for Intel Lightning Mountain(LGM) platform # +config PHY_INTEL_COMBO + bool "Intel Combo PHY driver" + depends on OF && HAS_IOMEM && (X86 || COMPILE_TEST) + select MFD_SYSCON + select GENERIC_PHY + select REGMAP + help + Enable this to support Intel combo phy. + + This driver configures combo phy subsystem on Intel gateway + chipsets which provides PHYs for various controllers, EMAC, + SATA and PCIe. + config PHY_INTEL_EMMC tristate "Intel EMMC PHY driver" select GENERIC_PHY
drivers/phy/intel/Makefile +1 -0
@@ -1,2 +1,3 @@ # SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_PHY_INTEL_COMBO) += phy-intel-combo.o obj-$(CONFIG_PHY_INTEL_EMMC) += phy-intel-emmc.o
drivers/phy/intel/phy-intel-combo.c new file +672 -0
672 lines not on this page · raw message
Signature · 1 line
2.11.0
Re: [PATCH v3 3/3] phy: intel: Add driver support for Combophy
Quoted 2 lines · Dilip Kota
Combophy subsystem provides PHYs for various
controllers like PCIe, SATA and EMAC.
Thanks for an update, my comments below.
...
drivers/phy/intel/Kconfig line 7
Quoted 4 lines
drivers/phy/intel/Kconfig +3
-0+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?
Quoted 3 lines
drivers/phy/intel/Kconfig +3
-0+ select MFD_SYSCON + select GENERIC_PHY + select REGMAP
...
Quoted 1 line
+ * Copyright (C) 2019 Intel Corporation.
2019-2020
...
drivers/phy/intel/phy-intel-combo.c line 42
Quoted 2 lines
drivers/phy/intel/phy-intel-combo.c new file +2
-0+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.
drivers/phy/intel/phy-intel-combo.c line 48
Quoted 7 lines
drivers/phy/intel/phy-intel-combo.c new file +6
-0+}; + +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.
Quoted 1 line
+};
...
Quoted 3 lines
+/*
+ * Clock register bitfields to enable clocks
+ * for ComboPhy according to the mode
For multi-line comments use proper English punctuation.
Quoted 1 line
+ */
...
drivers/phy/intel/phy-intel-combo.c line 123
Quoted 6 lines
drivers/phy/intel/phy-intel-combo.c new file +5
-0+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.
Quoted 7 lines
drivers/phy/intel/phy-intel-combo.c new file +7
-0+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); +}
...
drivers/phy/intel/phy-intel-combo.c line 160
Quoted 3 lines
drivers/phy/intel/phy-intel-combo.c new file +3
-0+ ret =phy_cfg (sphy); + +return ret;
return phy_cfg();
...
drivers/phy/intel/phy-intel-combo.c line 182
Quoted 4 lines
drivers/phy/intel/phy-intel-combo.c new file +4
-0+/* +* 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).
...
drivers/phy/intel/phy-intel-combo.c line 324
Quoted 10 lines
drivers/phy/intel/phy-intel-combo.c new file +10
-0+ 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
Quoted 2 lines
(quoted diff) +2
-0+ return ret; + }
...
drivers/phy/intel/phy-intel-combo.c line 405
Quoted 8 lines
drivers/phy/intel/phy-intel-combo.c new file +7
-0+/* 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().
...
Quoted 1 line
+static int intel_cbphy_iphy_dt_parse(struct
intel_combo_phy*cbphy,
dt -> fwnode
Ditto for other similar function names.
drivers/phy/intel/phy-intel-combo.c line 421
Quoted 4 lines
drivers/phy/intel/phy-intel-combo.c new file +3
-0+ 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.
Quoted 1 line
+}
...
drivers/phy/intel/phy-intel-combo.c line 449
Quoted 1 line
drivers/phy/intel/phy-intel-combo.c new file +1
-0+if (!iphy0->enable && !iphy1->enable) {
if (!(iphy0->enable || iphy1->enable)) {?
drivers/phy/intel/phy-intel-combo.c line 451
Quoted 3 lines
drivers/phy/intel/phy-intel-combo.c new file +2
-0+dev_err (cbphy->dev,"CBPHY%u both PHY disabled!\n" , cbphy->id); +return -EINVAL ;
-ENODEV ?
drivers/phy/intel/phy-intel-combo.c line 455
Quoted 4 lines
drivers/phy/intel/phy-intel-combo.c new file +4
-0+ } + +if (cbphy->aggr_mode ==PHY_DL_MODE && + (!iphy0->enable || !iphy1->enable)) {
if (cbphy->aggr_mode == PHY_DL_MODE && !(iphy0->enable && iphy1->enable)) {?
Quoted 6 lines
drivers/phy/intel/phy-intel-combo.c new file +6
-0+dev_err (cbphy->dev, +"Dual lane mode but lane0: %s, lane1: %s\n" , + iphy0->enable ?"on" :"off" , + iphy1->enable ?"on" :"off" ); +return -EINVAL ; + }
...
drivers/phy/intel/phy-intel-combo.c line 473
Quoted 6 lines
drivers/phy/intel/phy-intel-combo.c new file +6
-0+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;
drivers/phy/intel/phy-intel-combo.c line 499
Quoted 1 line
drivers/phy/intel/phy-intel-combo.c new file +1
-0+ pdev =to_platform_device (dev);
See above.
drivers/phy/intel/phy-intel-combo.c line 508
Quoted 1 line
drivers/phy/intel/phy-intel-combo.c new file +1
-0+ fwnode =dev_fwnode (dev);
See above.
drivers/phy/intel/phy-intel-combo.c line 536
Quoted 10 lines
drivers/phy/intel/phy-intel-combo.c new file +10
-0+ 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?
...
drivers/phy/intel/phy-intel-combo.c line 557
Quoted 9 lines
drivers/phy/intel/phy-intel-combo.c new file +8
-0+ 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...
drivers/phy/intel/phy-intel-combo.c line 565
Quoted 8 lines
drivers/phy/intel/phy-intel-combo.c new file +7
-0+ ret =intel_cbphy_iphy_dt_parse (cbphy, fwnode, i); +if (ret) { +fwnode_handle_put (fwnode); +return ret; + } + + i++;
...here.
Quoted 4 lines
drivers/phy/intel/phy-intel-combo.c new file +4
-0+ } + +return intel_cbphy_dt_sanity_check (cbphy); +}
...
drivers/phy/intel/phy-intel-combo.c line 575
Quoted 4 lines
drivers/phy/intel/phy-intel-combo.c new file +4
-0+ .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,
...
drivers/phy/intel/phy-intel-combo.c line 614
Quoted 4 lines
drivers/phy/intel/phy-intel-combo.c new file +3
-0+for (i =0 ; i <PHY_MAX_NUM ; i++) { + iphy = &cbphy->iphy[i]; +if (iphy->enable) {
if (!iphy->enable) continue;
?
Quoted 5 lines
drivers/phy/intel/phy-intel-combo.c new file +5
-0+ ret =intel_cbphy_iphy_create (iphy); +if (ret) +return ret; + } + }
...
drivers/phy/intel/phy-intel-combo.c line 637
Quoted 1 line
drivers/phy/intel/phy-intel-combo.c new file +1
-0+platform_set_drvdata (pdev, cbphy);
I guess it makes sense a bit later to do...
drivers/phy/intel/phy-intel-combo.c line 640
Quoted 3 lines
drivers/phy/intel/phy-intel-combo.c new file +3
-0+ ret =intel_cbphy_dt_parse (cbphy); +if (ret) +return ret;
...here?
Quoted 2 lines
drivers/phy/intel/phy-intel-combo.c new file +2
-0+ +return intel_cbphy_create (cbphy);
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v3 3/3] phy: intel: Add driver support for Combophy
Quoted 5 lines · Andy Shevchenko
Quoted 2 lines · Dilip Kota
drivers/phy/intel/phy-intel-combo.c new file +2
-0+ i =0 ; +fwnode_for_each_available_child_node (dev_fwnode (dev), fwnode) {
Quoted 8 lines
drivers/phy/intel/phy-intel-combo.c line 557
Quoted 6 lines
drivers/phy/intel/phy-intel-combo.c new file +6
-0+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.
Quoted 15 lines
drivers/phy/intel/phy-intel-combo.c line 565
Quoted 8 lines
drivers/phy/intel/phy-intel-combo.c new file +7
-0+ ret =intel_cbphy_iphy_dt_parse (cbphy, fwnode, i); +if (ret) { +fwnode_handle_put (fwnode); +return ret; + } + + i++;...here.
Quoted 4 lines
drivers/phy/intel/phy-intel-combo.c new file +4
-0+ } + +return intel_cbphy_dt_sanity_check (cbphy); +}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v3 3/3] phy: intel: Add driver support for Combophy
In reply to Andy ShevchenkoThanks 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.
Quoted 12 lines · Andy Shevchenko
Quoted 2 lines · Dilip Kota
Combophy subsystem provides PHYs for various
controllers like PCIe, SATA and EMAC.Thanks for an update, my comments below.
...
drivers/phy/intel/Kconfig line 7
Quoted 3 lines
drivers/phy/intel/Kconfig +3
-0+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:
Quoted 4 lines
depends on X86 || COMPILE_TEST depends on OF && HAS_IOMEMBut do you still have a dependency to OF?
Yes, OF is not required. I will remove it.
Quoted 8 lines
Quoted 4 lines
drivers/phy/intel/Kconfig +3
-0+ select MFD_SYSCON + select GENERIC_PHY + select REGMAP...
Quoted 1 line
+ * Copyright (C) 2019 Intel Corporation.
2019-2020
My bad. I will update it.
Quoted 3 lines
...
...
Quoted 8 lines
drivers/phy/intel/phy-intel-combo.c line 48
Quoted 6 lines
drivers/phy/intel/phy-intel-combo.c new file +6
-0+}; + +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.
...
Quoted 5 lines
...
Quoted 1 line
+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.
Quoted 5 lines
drivers/phy/intel/phy-intel-combo.c line 421
Quoted 4 lines
drivers/phy/intel/phy-intel-combo.c new file +3
-0+ 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.
...
Quoted 9 lines
...
drivers/phy/intel/phy-intel-combo.c line 473
Quoted 6 lines
drivers/phy/intel/phy-intel-combo.c new file +6
-0+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.
Quoted 8 lines
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;
Quoted 7 lines
drivers/phy/intel/phy-intel-combo.c line 499
Quoted 1 line
drivers/phy/intel/phy-intel-combo.c new file +1
-0+ pdev =to_platform_device (dev);See above.
drivers/phy/intel/phy-intel-combo.c line 508
Quoted 1 line
drivers/phy/intel/phy-intel-combo.c new file +1
-0+ fwnode =dev_fwnode (dev);See above.
Regards,
Dilip
Re: [PATCH v3 3/3] phy: intel: Add driver support for Combophy
Quoted 3 lines · Dilip Kota
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.
...
Quoted 11 lines
Quoted 6 lines
drivers/phy/intel/phy-intel-combo.c line 48
Quoted 4 lines
drivers/phy/intel/phy-intel-combo.c new file +4
-0+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.
...
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
+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 forintel_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?
Quoted 1 line
Quoted 1 line
Quoted 1 line
+ struct
fwnode_handle*fwnode, int idx)
...
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/phy/intel/phy-intel-combo.c new file +6
-0+struct fwnode_reference_args ref; +struct device *dev = cbphy->dev; +struct fwnode_handle *fwnode; +struct platform_device *pdev; +int i, ret; +u32 prop;
Quoted 7 lines
Quoted 1 line
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.
Quoted 12 lines
Quoted 12 lines
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;Quoted 1 line
+ pdev =
to_platform_device(dev);See above.
Quoted 1 line
+ fwnode =
dev_fwnode(dev);See above.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v3 3/3] phy: intel: Add driver support for Combophy
Quoted 2 lines · Andy Shevchenko
On Thu, Feb 27, 2020 at 9:54 AM Dilip Kota <[email protected]> wrote:
...
Quoted 9 lines
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
+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 forintel_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 takesof_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() )
Quoted 16 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
+ struct
fwnode_handle*fwnode, int idx)...
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/phy/intel/phy-intel-combo.c new file +6
-0+struct fwnode_reference_args ref; +struct device *dev = cbphy->dev; +struct fwnode_handle *fwnode; +struct platform_device *pdev; +int i, ret; +u32 prop;Quoted 6 lines
Quoted 1 line
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.
Quoted 6 lines
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
Quoted 1 line
Quoted 1 line
.