On Thu, Feb 20, 2020 at 05:58:41PM +0800, Dilip Kota wrote:
> On 2/19/2020 6:14 PM, Andy Shevchenko wrote:
> > On Wed, Feb 19, 2020 at 11:31:30AM +0800, Dilip Kota wrote:
...
> > return regmap_update_bits(..., mask, val);
> >
> > ?
> Still it is taking more than 80 characters with mask, need to be in 2 lines
>
> return regmap_update_bits(...,
> mask, val);
It's up to maintainer, I was talking about use of temporary variable for mask.
> > > +static int intel_cbphy_pcie_refclk_cfg(struct intel_cbphy_iphy *iphy, bool set)
> > > +{
> > > + struct intel_combo_phy *cbphy = iphy->parent;
> > > + const u32 pad_dis_cfg_off = 0x174;
> > > + u32 val, bitn;
> > > +
> > > + bitn = cbphy->id * 2 + iphy->id;
> > > +
> > > + /* Register: 0 is enable, 1 is disable */
> > > + val = set ? 0 : BIT(bitn);
> > > +
> > > + return regmap_update_bits(cbphy->syscfg, pad_dis_cfg_off, BIT(bitn),
> > > + val);
> > Ditto.
> Here it can with go in single line with mask,
Here I meant all changes from previous function, yes, temporary variable mask
in particular.
> > > +}
...
> > > + case PHY_PCIE_MODE:
> > > + cb_mode = (aggr == PHY_DL_MODE) ?
> > > + PCIE_DL_MODE : PCIE0_PCIE1_MODE;
> > I think one line is okay here.
> its taking 82 characters.
Up to maintainer, but I consider the two lines approach is worse to read.
> > > + break;
...
> > > + ret = intel_cbphy_iphy_cfg(iphy,
> > > + intel_cbphy_pcie_en_pad_refclk);
> > One line is fine here.
> It is taking 81 characters, so kept in 2 lines.
Ditto.
> > > + if (ret)
> > > + return ret;
...
> > > + ret = intel_cbphy_iphy_cfg(iphy,
> > > + intel_cbphy_pcie_dis_pad_refclk);
> > Ditto.
> 82 characters here.
Ditto.
> > > + if (ret)
> > > + return ret;
...
> > > + ret = fwnode_property_get_reference_args(dev_fwnode(dev),
> > > + "intel,syscfg", NULL, 1, 0,
> > > + &ref);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + fwnode_handle_put(ref.fwnode);
> > Why here?
>
> Instructed to do:
>
> " Caller is responsible to call fwnode_handle_put() on the returned
> args->fwnode pointer"
Right...
> > > + cbphy->id = ref.args[0];
> > > + cbphy->syscfg = device_node_to_regmap(ref.fwnode->dev->of_node);
...and here you called unreferenced one. Is it okay?
If it's still being referenced, that is fine, but otherwise it may gone already.
> > > + ret = fwnode_property_get_reference_args(dev_fwnode(dev), "intel,hsio",
> > > + NULL, 1, 0, &ref);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + fwnode_handle_put(ref.fwnode);
> > > + cbphy->bid = ref.args[0];
> > > + cbphy->hsiocfg = device_node_to_regmap(ref.fwnode->dev->of_node);
> > Ditto.
> >
> > > + if (!device_property_read_u32(dev, "intel,phy-mode", &prop)) {
> > Hmm... Why to mix device_property_*() vs. fwnode_property_*() ?
> device_property_* are wrapper functions to fwnode_property_*().
> Calling the fwnode_property_*() ending up doing the same work of
> device_property_*().
>
> If the best practice is to maintain symmetry, will call fwnode_property_*().
The best practice to keep consistency as much as possible.
If you call two similar APIs in one scope, it's not okay.
--
With Best Regards,
Andy Shevchenko
