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.
