Re: [v2,4/6] reset-controller: ti: introduce a new reset handler

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

On Mon, 2020-08-03 at 14:15 +0800, Crystal Guo wrote:
> Add ti_syscon_reset() to integrate assert and deassert together.
> If some modules need do serialized assert and deassert operations
> to reset itself, reset_control_reset can be called for convenience.
> 
> Change-Id: I9046992b115a46f3594de57fa89c6a2de9957d49
> ---
>  drivers/reset/reset-ti-syscon.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> index a2635c21db7f..1c74bcb9a6c3 100644
> --- a/drivers/reset/reset-ti-syscon.c
> +++ b/drivers/reset/reset-ti-syscon.c
> @@ -56,6 +56,7 @@ struct ti_syscon_reset_data {
>  	struct regmap *regmap;
>  	struct ti_syscon_reset_control *controls;
>  	unsigned int nr_controls;
> +	bool assert_deassert_together;
>  };
>  
>  #define to_ti_syscon_reset_data(rcdev)	\
> @@ -158,10 +159,24 @@ static int ti_syscon_reset_status(struct reset_controller_dev *rcdev,
>  		!(control->flags & STATUS_SET);
>  }
>  
> +static int ti_syscon_reset(struct reset_controller_dev *rcdev,
> +			  unsigned long id)
> +{
> +	struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev);
> +
> +	if (data->assert_deassert_together) {
> +		ti_syscon_reset_assert(rcdev, id);
> +		return ti_syscon_reset_deassert(rcdev, id);

Even if your hardware engineers guarantee that no delays are necessary
between assert and deassert for any peripheral used with your reset
controller that may use this function, this is not generic.

I wonder if it would be better to define a minimum delay similarly to
reset-simple [1].

[1] https://lore.kernel.org/lkml/be2cecb2654e68385561a15df7967c7723d5531d.1590594512.git-series.maxime@cerno.tech/

> +	} else {
> +		return -ENOTSUPP;
> +	}
> +}
> +
>  static const struct reset_control_ops ti_syscon_reset_ops = {
>  	.assert		= ti_syscon_reset_assert,
>  	.deassert	= ti_syscon_reset_deassert,
>  	.status		= ti_syscon_reset_status,
> +	.reset		= ti_syscon_reset,
>  };
>  
>  static int ti_syscon_reset_probe(struct platform_device *pdev)
> @@ -204,6 +219,11 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
>  		controls[i].flags = be32_to_cpup(list++);
>  	}
>  
> +	if (of_property_read_bool(np, "assert-deassert-together"))
> +		data->assert_deassert_together = true;
> +	else
> +		data->assert_deassert_together = false;
> +

This could just be assigned directly, but as said above, I think it
might be better to have a reset-duration-us property or similar.

regards
Philipp