Messages in this thread | | | Subject | Re: [PATCH v3 3/3] phy: intel: Add driver support for Combophy | From | Dilip Kota <> | Date | Thu, 27 Feb 2020 15:52:13 +0800 |
| |
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
| |