Hi Sibi,

On Fri, Aug 24, 2018 at 06:48:56PM +0530, Sibi Sankar wrote:

What exactly does APPS mean? The AP cores, the entire SoC, something
else?

Headers should be sorted in alphabetical order.

+
+#define RPMH_PDC_SYNC_RESET	0x100
+
+struct qcom_pdc_reset_map {
+	u8 bit;
+};
+
+struct qcom_pdc_desc {
+	const struct regmap_config *config;
+	const struct qcom_pdc_reset_map *resets;
+	size_t num_resets;
+};

Not sure if this structure adds much value or just a layer of
indirection:

- .config is only accessed in _probe(), sdm845_pdc_regmap_config could
  be used directly
- .resets is used in _(de)assert(), sdm845_pdc_resets could be used
  directly
- .num_resets is only accessed in _probe(),
  ARRAY_SIZE(sdm845_pdc_resets) could be used instead

It probably makes sense if it is planned to support reset controllers
of other SoCs with this driver.

+struct qcom_pdc_reset_data {
+	struct reset_controller_dev rcdev;
+	struct regmap *regmap;
+	const struct qcom_pdc_desc *desc;
+};
+
+static const struct regmap_config sdm845_pdc_regmap_config = {
+	.name		= "pdc-reset",
+	.reg_bits	= 32,
+	.reg_stride	= 4,
+	.val_bits	= 32,
+	.max_register	= 0x20000,
+	.fast_io	= true,
+};
+
+static const struct qcom_pdc_reset_map sdm845_pdc_resets[] = {
+	[PDC_APPS_SYNC_RESET] = {0},
+	[PDC_SP_SYNC_RESET] = {1},
+	[PDC_AUDIO_SYNC_RESET] = {2},
+	[PDC_SENSORS_SYNC_RESET] = {3},
+	[PDC_AOP_SYNC_RESET] = {4},
+	[PDC_DEBUG_SYNC_RESET] = {5},
+	[PDC_GPU_SYNC_RESET] = {6},
+	[PDC_DISPLAY_SYNC_RESET] = {7},
+	[PDC_COMPUTE_SYNC_RESET] = {8},
+	[PDC_MODEM_SYNC_RESET] = {9},
+};
+
+static const struct qcom_pdc_desc sdm845_pdc_desc = {
+	.config = &sdm845_pdc_regmap_config,
+	.resets = sdm845_pdc_resets,
+	.num_resets = ARRAY_SIZE(sdm845_pdc_resets),
+};
+
+static inline struct qcom_pdc_reset_data *to_qcom_pdc_reset_data(
+				struct reset_controller_dev *rcdev)
+{
+	return container_of(rcdev, struct qcom_pdc_reset_data, rcdev);
+}
+
+static int qcom_pdc_control_assert(struct reset_controller_dev *rcdev,
+					unsigned long idx)
+{
+	struct qcom_pdc_reset_data *data = to_qcom_pdc_reset_data(rcdev);
+	const struct qcom_pdc_reset_map *map = &data->desc->resets[idx];
+
+	return regmap_update_bits(data->regmap, RPMH_PDC_SYNC_RESET,
+					BIT(map->bit), BIT(map->bit));
+}
+
+static int qcom_pdc_control_deassert(struct reset_controller_dev *rcdev,
+					unsigned long idx)
+{
+	struct qcom_pdc_reset_data *data = to_qcom_pdc_reset_data(rcdev);
+	const struct qcom_pdc_reset_map *map = &data->desc->resets[idx];
+
+	return regmap_update_bits(data->regmap, RPMH_PDC_SYNC_RESET,
+					BIT(map->bit), 0);
+}
+
+static const struct reset_control_ops qcom_pdc_reset_ops = {
+	.assert = qcom_pdc_control_assert,
+	.deassert = qcom_pdc_control_deassert,
+};
+
+static int qcom_pdc_reset_probe(struct platform_device *pdev)
+{
+	struct qcom_pdc_reset_data *data;
+	struct device *dev = &pdev->dev;
+	const struct qcom_pdc_desc *desc;
+	void __iomem *base;
+	struct resource *res;
+
+	desc = of_device_get_match_data(dev);
+	if (!desc)
+		return -EINVAL;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->desc = desc;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	data->regmap = devm_regmap_init_mmio(dev, base, desc->config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(dev, "Unable to get pdc-global regmap");

Add missing '\n'

Say 'pdc-reset' instead of 'pdc-global'? (see also my other comment
below).

+		return PTR_ERR(data->regmap);
+	}
+
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.ops = &qcom_pdc_reset_ops;
+	data->rcdev.nr_resets = desc->num_resets;
+	data->rcdev.of_node = dev->of_node;
+
+	return devm_reset_controller_register(dev, &data->rcdev);
+}
+
+static const struct of_device_id qcom_pdc_reset_of_match[] = {
+	{ .compatible = "qcom,sdm845-pdc-global", .data = &sdm845_pdc_desc },

Should this be 'qcom,sdm845-pdc-reset' which is more specific than
'global' and in line with the name and purpose of the driver?
Obviously this would require to adjust the bindings doc too.

Cheers

Matthias