Re: [PATCH v3 10/16] reset: Add a reset controller driver for the Lantiq XWAY based SoCs

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Hauke,

On Sun, 2017-05-28 at 20:40 +0200, Hauke Mehrtens wrote:
[...]
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/lantiq,reset.txt
> @@ -0,0 +1,30 @@
> +Lantiq XWAY SoC RCU reset controller binding
> +============================================
> +
> +This binding describes a reset-controller found on the RCU module on Lantiq
> +XWAY SoCs.
> +
> +
> +-------------------------------------------------------------------------------
> +Required properties:
> +- compatible		: Should be one of
> +				"lantiq,reset-danube"
> +				"lantiq,reset-xrx200"
> +- regmap		: A phandle to the RCU syscon
> +- offset-set		: Offset of the reset set register
> +- offset-status		: Offset of the reset status register
> +- #reset-cells		: Specifies the number of cells needed to encode the
> +			  reset line, should be 2.
> +			  The first cell takes the reset set bit and the
> +			  second cell takes the status bit.
> +
> +-------------------------------------------------------------------------------
> +Example for the reset-controllers on the xRX200 SoCs:
> +	reset0: reset-controller@0 {
> +		compatible = "lantiq,reset-xrx200";
> +
> +		regmap = <&rcu0>;

Why not place these nodes as children of &rcu0 ? This property would be
superfluous then.

> +		offset-set = <0x10>;
> +		offset-status = <0x14>;
> +		#reset-cells = <2>;
> +	};
[...]
> --- /dev/null
> +++ b/drivers/reset/reset-lantiq.c
> @@ -0,0 +1,205 @@
[...]
> +static int lantiq_rcu_reset_update(struct reset_controller_dev *rcdev,
> +				   unsigned long id, bool assert)
> +{
> +	struct lantiq_rcu_reset_priv *priv = to_lantiq_rcu_reset_priv(rcdev);
> +	int ret, retry = LANTIQ_RCU_RESET_TIMEOUT;
> +	unsigned int set = id & 0x1f;
> +	u32 val;
> +
> +	if (assert)
> +		val = BIT(set);
> +	else
> +		val = 0;
> +
> +	ret = regmap_update_bits(priv->regmap, priv->reset_offset, BIT(set),
> +				 val);
> +	if (ret) {
> +		dev_err(priv->dev, "Failed to set reset bit %u\n", set);
> +		return ret;
> +	}
> +
> +	do {
> +		ret = lantiq_rcu_reset_status(rcdev, id);
> +		if (ret < 0)
> +			return ret;
> +		if (ret == assert)
> +			break;
> +		usleep_range(20, 40);
> +	} while (--retry);

How about wrapping this in a lantiq_rcu_poll_status(_timeout) function?
I still think this is a case where regmap_read_poll_timeout could be
used.

[...]
> +static int lantiq_rcu_reset_of_probe(struct platform_device *pdev,
> +			       struct lantiq_rcu_reset_priv *priv)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	priv->regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "regmap");

Why not use syscon_node_to_regmap(dev->of_node->parent) ?

regards
Philipp