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: <685783e3-9636-e512-7810-1f87ac5d9328@linaro.org>
Date:   Thu, 18 Aug 2022 11:05:14 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH 2/4] soc: qcom: pmic_glink: Introduce base PMIC GLINK
 driver
Content-Language: en-US
To:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Sebastian Reichel <sre@kernel.org>,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org
References: <20220818031512.319310-1-bjorn.andersson@linaro.org>
 <20220818031512.319310-3-bjorn.andersson@linaro.org>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20220818031512.319310-3-bjorn.andersson@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 18/08/2022 06:15, Bjorn Andersson wrote:
> The PMIC GLINK service runs on one of the co-processors of some modern
> Qualcomm platforms and implements USB-C and battery managements. It uses
> a message based protocol over GLINK for communication with the OS, hence
> the name.
> 
> The driver implemented provides the rpmsg device for communication and
> uses auxilirary bus to spawn off individual devices in respsective

typos:
auxiliary
respective
> subsystem. The auxilirary devices are spawned off from a

auxiliary

> platform_device, so that the drm_bridge is available early, to allow the
> DisplayPort driver to probe even before the remoteproc has spun up.
> 

(...)

> +
> +static int pmic_glink_init(void)
> +{
> +	platform_driver_register(&pmic_glink_driver);
> +	register_rpmsg_driver(&pmic_glink_rpmsg_driver);
> +
> +	return 0;
> +};
> +module_init(pmic_glink_init);
> +
> +static void pmic_glink_exit(void)
> +{
> +	platform_driver_unregister(&pmic_glink_driver);
> +	unregister_rpmsg_driver(&pmic_glink_rpmsg_driver);

Shouldn't this be in reversed order of init()? So first unregister
rpmsg, then platform driver.

> +};
> +module_exit(pmic_glink_exit);
> +
> +MODULE_DESCRIPTION("Qualcomm PMIC GLINK driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/soc/qcom/pmic_glink.h b/include/linux/soc/qcom/pmic_glink.h
> new file mode 100644
> index 000000000000..40470f8dfc1e
> --- /dev/null
> +++ b/include/linux/soc/qcom/pmic_glink.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2022, Linaro Ltd
> + */
> +#ifndef __PMIC_GLINK_H__

I propose more detailed guard:
__SOC_QCOM_PMIC_GLINK_H__

> +#define __PMIC_GLINK_H__


Best regards,
Krzysztof