Re: [PATCH v3 1/7] dt-bindings: reset: Add AOSS reset bindings for SDM845 SoCs

[Sibi S]

Hi Bjorn,
Thanks for the review.

On 03/19/2018 04:14 AM, Bjorn Andersson wrote:
> On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote:
> > +- reg:
> > +	Usage: required
> > +	Value type: <prop-encoded-array>
> > +	Definition: must specify the base address and size of the register
> > +	            space.
> > +
> > +
>
> Double empty lines.

will remove them

> > +- #reset-cells:
> > +	Usage: required
> > +	Value type: <uint>
> > +	Definition: must be 1; cell entry represents the reset index.
> > +
> > +example:
>
> Please capitalize the initial char.

sure

> > +
> > +aoss_reset: qcom,reset-controller@b2e0100 {
> > +	compatible = "qcom,sdm845-aoss-reset";
> > +	reg = <0xc2b0000 0x20004>;
>
> 0x20004 does seem very even, please verify this size.

even though the reg space after that range is unused, the AOSS reset
driver is supposed to control only those listed reset lines

> > +	#reset-cells = <1>;
> > +};
> > +
> > +
> > +Specifying reset lines connected to IP modules
>
> "AOSS reset clients"

yep this heading makes much more sense

> Although you could probably get a way with just referring to reset.txt
> and the header file.
>
> > +==============================================
> > +
> > +Device nodes that need access to reset lines should
> > +specify them as a reset phandle in their corresponding node as
> > +specified in reset.txt.
> > +
> > +For list of all valid reset indicies see
> > +<dt-bindings/reset/qcom,sdm845-aoss.h>
> > +
> > +Example:
> > +
> > +modem-pil@4080000 {
> > +	...
> > +
> > +	resets = <&aoss_reset AOSS_CC_MSS_RESTART>;
> > +	reset-names = "mss_restart";
> > +
> > +	...
> > +};
> > diff --git a/include/dt-bindings/reset/qcom,sdm845-aoss.h b/include/dt-bindings/reset/qcom,sdm845-aoss.h
> > new file mode 100644
> > index 0000000..e9b38fc
> > --- /dev/null
> > +++ b/include/dt-bindings/reset/qcom,sdm845-aoss.h
> > @@ -0,0 +1,17 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
> For tooling reasons header files should have their SPDX License tag
> wrapped in /* */

Sure will replace it

> > +/*
> > + * Copyright (C) 2018 The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#ifndef _DT_BINDINGS_RESET_AOSS_SDM_845_H
> > +#define _DT_BINDINGS_RESET_AOSS_SDM_845_H
> > +
> > +#define AOSS_CC_MSS_RESTART	0
> > +#define AOSS_CC_CAMSS_RESTART	1
> > +#define AOSS_CC_VENUS_RESTART	2
> > +#define AOSS_CC_GPU_RESTART	3
> > +#define AOSS_CC_DISPSS_RESTART	4
> > +#define AOSS_CC_WCSS_RESTART	5
> > +#define AOSS_CC_LPASS_RESTART	6
> > +
> > +#endif
>
> Apart from these nits this looks reasonable.
>
> Regards,
> Bjorn

--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project