On Mon 27 Aug 17:22 PDT 2018, Matthias Kaehlcke wrote:

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

quoted

diff --git a/drivers/reset/reset-qcom-pdc.c b/drivers/reset/reset-qcom-pdc.c

[..]

quoted

+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.

I like this suggestion, once we need the configurability we can add the
type for it. It also shows that only .resets would need to be referenced
by qcom_pdc_reset_data.

quoted

+struct qcom_pdc_reset_data {
+	struct reset_controller_dev rcdev;
+	struct regmap *regmap;
+	const struct qcom_pdc_desc *desc;
+};

[..]

quoted

+static int qcom_pdc_reset_probe(struct platform_device *pdev)
+{

[..]

quoted

+	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).

This regmap is created out of the single anonymous "reg", so I think the
error should be reduced to "Unable to initialize regmap\n".

[..]

quoted

+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.

No, the binding describes the hardware block named "PDC Global",
currently implemented as a reset controller. The reason for doing this
is so that we one day can expose additional interfaces in a backwards
compatible fashion.

Regards,
Bjorn