Re: [PATCH v2 2/2] phy: intel: Add driver support for Combophy

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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