Re: [PATCH v3 3/3] phy: intel: Add driver support for Combophy

[Andy Shevchenko]

On Thu, Feb 27, 2020 at 9:54 AM Dilip Kota <eswara.kota@xxxxxxxxxxxxxxx> 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.

-- 
With Best Regards,
Andy Shevchenko