Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <dee69294-e743-e249-3075-e83f5c668c88@linaro.org>
Date:   Thu, 25 May 2023 10:21:34 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH 1/5] dt-bindings: clock: Add YAML schemas for LPASSCC and
 reset on SC8280XP
To:     Johan Hovold <johan@kernel.org>
Cc:     andersson@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, johan+linaro@kernel.org,
        agross@kernel.org, konrad.dybcio@linaro.org,
        mturquette@baylibre.com, sboyd@kernel.org, conor+dt@kernel.org,
        linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230518113800.339158-1-srinivas.kandagatla@linaro.org>
 <20230518113800.339158-2-srinivas.kandagatla@linaro.org>
 <ZGsmCmEdoWIkcVDd@hovoldconsulting.com>
Content-Language: en-US
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
In-Reply-To: <ZGsmCmEdoWIkcVDd@hovoldconsulting.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

thanks Johan for review,

On 22/05/2023 09:21, Johan Hovold wrote:
> On Thu, May 18, 2023 at 12:37:56PM +0100, Srinivas Kandagatla wrote:
>> The LPASS(Low Power Audio Subsystem) clock provider provides reset
> 
> Missing space after LPASS acronym.
> 
> s/provider/controller/?
> 
>> controller support when is driven by the Q6DSP.
> 
> s/controller//?
> 
> "when is driven by": sounds like there are some words missing here.
> 
>> This patch adds support for those resets and adds IDs for clients
> 
> There is never any need to say "this patch" in a commit message. Just say
> 
> 	Add support for...
> 
>> to request the reset.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   .../bindings/clock/qcom,sc8280xp-lpasscc.yaml | 57 +++++++++++++++++++
>>   .../dt-bindings/clock/qcom,lpasscc-sc8280xp.h | 12 ++++
>>   2 files changed, 69 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/clock/qcom,sc8280xp-lpasscc.yaml
>>   create mode 100644 include/dt-bindings/clock/qcom,lpasscc-sc8280xp.h
>>
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,sc8280xp-lpasscc.yaml b/Documentation/devicetree/bindings/clock/qcom,sc8280xp-lpasscc.yaml
>> new file mode 100644
>> index 000000000000..7c30614a0af9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/qcom,sc8280xp-lpasscc.yaml
>> @@ -0,0 +1,57 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/qcom,sc8280xp-lpasscc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm LPASS Core & Audio Clock Controller on SC8280XP
>> +
>> +maintainers:
>> +  - Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> +
>> +description: |
>> +  Qualcomm LPASS core and audio clock control module provides the clocks,
>> +  reset and power domains on SC8280XP.
> 
> "power domains"? copy-paste error?
> 
>> +
>> +  See also::
>> +    include/dt-bindings/clock/qcom,lpasscc-sc8280xp.h
>> +
>> +properties:
>> +  reg: true
>> +
>> +  compatible:
>> +    enum:
>> +      - qcom,sc8280xp-lpasscc
>> +
>> +  '#reset-cells':
>> +    const: 1
>> +
>> +  '#clock-cells':
>> +    const: 1
> 
> Move above #reset-cells for some sorting of related attributes. Same
> below (in two places).
> 
>> +
>> +  qcom,adsp-pil-mode:
>> +    description:
>> +      Indicates if the LPASS would be brought out of reset using
>> +      peripheral loader.
>> +    type: boolean
> 
> Move above the provider cells properties?
> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - qcom,adsp-pil-mode
> 
> If this boolean property is always needed, shouldn't that simply be
> handled by the driver based on the compatible?

Traditionally in Qcom SoCs LPASS is under the control of ADSP, there 
have been some other variants specially chrome platforms that have moved 
this control to APPs processor.

Having this property at Device tree level provides more flexibility, 
given that both the cases use same compatible strings.


Am okay with reset of the comments, Will fix them in v2.
thanks,
Srini

> 
>> +  - '#reset-cells'
>> +  - '#clock-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/qcom,lpasscc-sc8280xp.h>
>> +    lpasscc: clock-controller@3900000 {
>> +      compatible = "qcom,sc8280xp-lpasscc";
> 
> binding examples use 4-space indentation.
> 
>> +      reg = <0x033e0000 0x12000>;
> 
> Does not match the node unit address.
> 
>> +      #reset-cells = <1>;
>> +      #clock-cells = <1>;
>> +      qcom,adsp-pil-mode;
>> +    };
>> +...
>> diff --git a/include/dt-bindings/clock/qcom,lpasscc-sc8280xp.h b/include/dt-bindings/clock/qcom,lpasscc-sc8280xp.h
>> new file mode 100644
>> index 000000000000..df800ea2741c
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/qcom,lpasscc-sc8280xp.h
>> @@ -0,0 +1,12 @@
>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>> +/*
>> + * Copyright (c) 2023, Linaro Ltd.
>> + */
>> +
>> +#ifndef _DT_BINDINGS_CLK_QCOM_LPASSCC_SC8280XP_H
>> +#define _DT_BINDINGS_CLK_QCOM_LPASSCC_SC8280XP_H
>> +
>> +/* LPASS TCSR */
>> +#define LPASS_AUDIO_SWR_TX_CGCR				0
>> +
>> +#endif