Re: [PATCH v3 2/2] PCI: j721e: Enable ACSPCIE Refclk if "ti,syscon-acspcie-proxy-ctrl" exists

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Aug 28, 2024 at 04:19:06PM -0500, Bjorn Helgaas wrote:

Hello Bjorn,

> On Tue, Aug 27, 2024 at 11:25:48AM +0530, Siddharth Vadapalli wrote:
> > The ACSPCIE module is capable of driving the reference clock required by
> > the PCIe Endpoint device. It is an alternative to on-board and external
> > reference clock generators. Enabling the output from the ACSPCIE module's
> > PAD IO Buffers requires clearing the "PAD IO disable" bits of the
> > ACSPCIE_PROXY_CTRL register in the CTRL_MMR register space.
> > 
> > Add support to enable the ACSPCIE reference clock output using the optional
> > device-tree property "ti,syscon-acspcie-proxy-ctrl".
> > 
> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@xxxxxx>
> > ---
> > 

[...]

> > +
> > +	ret = of_parse_phandle_with_fixed_args(node,
> > +					       "ti,syscon-acspcie-proxy-ctrl",
> > +					       1, 0, &args);
> > +	if (!ret) {
> > +		/* Clear PAD IO disable bits to enable refclk output */
> > +		val = ~(args.args[0]);
> > +		ret = regmap_update_bits(syscon, 0, mask, val);
> > +		if (ret)
> > +			dev_err(dev, "failed to enable ACSPCIE refclk: %d\n",
> > +				ret);
> > +	} else {
> > +		dev_err(dev,
> > +			"ti,syscon-acspcie-proxy-ctrl has invalid arguments\n");
> > +	}
> 
> I should have mentioned this the first time, but this would be easier
> to read if structured as:
> 
>   ret = of_parse_phandle_with_fixed_args(...);
>   if (ret) {
>     dev_err(...);
>     return ret;
>   }
> 
>   /* Clear PAD IO disable bits to enable refclk output */
>   val = ~(args.args[0]);
>   ret = regmap_update_bits(syscon, 0, mask, val);
>   if (ret) {
>     dev_err(...);
>     return ret;
>   }
> 
>   return 0;

Yes, this removes the nested IF conditions and is definitely a cleaner
approach. I will update this in the next version of the patch.

> 
> > +	return ret;
> > +}
> > +
> >  static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
> >  {
> >  	struct device *dev = pcie->cdns_pcie->dev;
> > @@ -259,6 +288,15 @@ static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
> >  		return ret;
> >  	}
> >  
> > +	/* Enable ACSPCIE refclk output if the optional property exists */
> > +	syscon = syscon_regmap_lookup_by_phandle_optional(node,
> > +						"ti,syscon-acspcie-proxy-ctrl");
> > +	if (syscon) {
> > +		ret = j721e_enable_acspcie_refclk(pcie, syscon);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	return 0;
> 
> Not as dramatic here, but I think the following would be a little
> simpler since the final "return" isn't used for two purposes
> ((1) syscon property absent, (2) syscon present and refclk
> successfully enabled):
> 
>   syscon = syscon_regmap_lookup_by_phandle_optional(...);
>   if (!syscon)
>     return 0;
> 
>   return j721e_enable_acspcie_refclk(...);

Sure. I will implement the above in the next patch. Thank you for
reviewing this patch and sharing your feedback.

Regards,
Siddharth.




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux