Re: [PATCH v2 2/4] interconnect: qcom: Add QCS404 interconnect provider driver

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Mon 15 Apr 03:43 PDT 2019, Georgi Djakov wrote:
> diff --git a/drivers/interconnect/qcom/qcs404.c b/drivers/interconnect/qcom/qcs404.c
[..]
> +#define QCS404_MASTER_AMPSS_M0			0
> +#define QCS404_MASTER_GRAPHICS_3D		1
> +#define QCS404_MASTER_MDP_PORT0			2
> +#define QCS404_SNOC_BIMC_1_MAS			3
> +#define QCS404_MASTER_TCU_0			4
> +#define QCS404_MASTER_SPDM			5
> +#define QCS404_MASTER_BLSP_1			6
> +#define QCS404_MASTER_BLSP_2			7
> +#define QCS404_MASTER_XM_USB_HS1		8
> +#define QCS404_MASTER_CRYPTO_CORE0		9
> +#define QCS404_MASTER_SDCC_1			10
> +#define QCS404_MASTER_SDCC_2			11
> +#define QCS404_SNOC_PNOC_MAS			12
> +#define QCS404_MASTER_QPIC			13
> +#define QCS404_MASTER_QDSS_BAM			14
> +#define QCS404_BIMC_SNOC_MAS			15
> +#define QCS404_PNOC_SNOC_MAS			16
> +#define QCS404_MASTER_QDSS_ETR			17
> +#define QCS404_MASTER_EMAC			18
> +#define QCS404_MASTER_PCIE			19
> +#define QCS404_MASTER_USB3			20
> +#define QCS404_PNOC_INT_0			21
> +#define QCS404_PNOC_INT_2			22
> +#define QCS404_PNOC_INT_3			23
> +#define QCS404_PNOC_SLV_0			24
> +#define QCS404_PNOC_SLV_1			25
> +#define QCS404_PNOC_SLV_2			26
> +#define QCS404_PNOC_SLV_3			27
> +#define QCS404_PNOC_SLV_4			28
> +#define QCS404_PNOC_SLV_6			29
> +#define QCS404_PNOC_SLV_7			30
> +#define QCS404_PNOC_SLV_8			31
> +#define QCS404_PNOC_SLV_9			32
> +#define QCS404_PNOC_SLV_10			33
> +#define QCS404_PNOC_SLV_11			34
> +#define QCS404_SNOC_QDSS_INT			35
> +#define QCS404_SNOC_INT_0			36
> +#define QCS404_SNOC_INT_1			37
> +#define QCS404_SNOC_INT_2			38
> +#define QCS404_SLAVE_EBI_CH0			39
> +#define QCS404_BIMC_SNOC_SLV			40
> +#define QCS404_SLAVE_SPDM_WRAPPER		41
> +#define QCS404_SLAVE_PDM			42
> +#define QCS404_SLAVE_PRNG			43
> +#define QCS404_SLAVE_TCSR			44
> +#define QCS404_SLAVE_SNOC_CFG			45
> +#define QCS404_SLAVE_MESSAGE_RAM		46
> +#define QCS404_SLAVE_DISPLAY_CFG		47
> +#define QCS404_SLAVE_GRAPHICS_3D_CFG		48
> +#define QCS404_SLAVE_BLSP_1			49
> +#define QCS404_SLAVE_TLMM_NORTH			50
> +#define QCS404_SLAVE_PCIE_1			51
> +#define QCS404_SLAVE_EMAC_CFG			52
> +#define QCS404_SLAVE_BLSP_2			53
> +#define QCS404_SLAVE_TLMM_EAST			54
> +#define QCS404_SLAVE_TCU			55
> +#define QCS404_SLAVE_PMIC_ARB			56
> +#define QCS404_SLAVE_SDCC_1			57
> +#define QCS404_SLAVE_SDCC_2			58
> +#define QCS404_SLAVE_TLMM_SOUTH			59
> +#define QCS404_SLAVE_USB_HS			60
> +#define QCS404_SLAVE_USB3			61
> +#define QCS404_SLAVE_CRYPTO_0_CFG		62
> +#define QCS404_PNOC_SNOC_SLV			63
> +#define QCS404_SLAVE_APPSS			64
> +#define QCS404_SLAVE_WCSS			65
> +#define QCS404_SNOC_BIMC_1_SLV			66
> +#define QCS404_SLAVE_OCIMEM			67
> +#define QCS404_SNOC_PNOC_SLV			68
> +#define QCS404_SLAVE_QDSS_STM			69
> +#define QCS404_SLAVE_CATS_128			70
> +#define QCS404_SLAVE_OCMEM_64			71
> +#define QCS404_SLAVE_LPASS			72

An enum would probably be cleaner, given that the actual values are not
significant.

[..]
> +static int qnoc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct qcom_icc_desc *desc;
> +	struct icc_onecell_data *data;
> +	struct icc_provider *provider;
> +	struct qcom_icc_node **qnodes;
> +	struct qcom_icc_provider *qp;
> +	struct icc_node *node;
> +	size_t num_nodes, i;
> +	int ret;
> +
> +	rpm = dev_get_drvdata(dev->parent);
> +	if (!rpm) {
> +		dev_err(dev, "unable to retrieve handle to RPM\n");
> +		return -ENODEV;
> +	}

In line with my feedback on the DT binding I would prefer if this driver
deals with the devices on the mmio/soc bus and you move the SMD/RPM part
to a separate driver - then perform the SMD/RPM operation by calling
into the other driver.

Given how much of this driver is platforms specific then I think it's a
pretty clean split for adding further (SMD/RPM based) platforms.


Except for the decision on where in the device tree this sits I think
the implementation looks good now!

Regards,
Bjorn