Re: [PATCH 25/89] reset: simple: Add reset callback

[Philipp Zabel]

Hi Maxime,

On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote:
> The reset-simple code lacks a reset callback that is still pretty easy to
> implement. The only real thing to consider is the delay needed for a device
> to be reset, so let's expose that as part of the reset-simple driver data.
>
> Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>

This shoulod be done in such a way that simple reset drivers which do
not set the reset delay continue to return -ENOTSUPP from
reset_control_reset().

> ---
>  drivers/reset/reset-simple.c       | 21 +++++++++++++++++++++
>  include/linux/reset/reset-simple.h |  4 ++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
> index c854aa351640..7a8c56512ae9 100644
> --- a/drivers/reset/reset-simple.c
> +++ b/drivers/reset/reset-simple.c
> @@ -11,6 +11,7 @@
>   * Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
> @@ -63,6 +64,25 @@ static int reset_simple_deassert(struct reset_controller_dev *rcdev,
>  	return reset_simple_update(rcdev, id, false);
>  }
>  
> +static int reset_simple_reset(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct reset_simple_data *data = to_reset_simple_data(rcdev);
> +	int ret;

You could just return -ENOTSUPP here if data->reset_ms == 0.

> +	ret = reset_simple_assert(rcdev, id);
> +	if (ret)
> +		return ret;
> +
> +	mdelay(data->reset_ms);

Have you considered specifying the delay in microseconds instead?
That would allow to use usleep_range() for shorter delays.

> +	ret = reset_simple_deassert(rcdev, id);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static int reset_simple_status(struct reset_controller_dev *rcdev,
>  			       unsigned long id)
>  {
> @@ -80,6 +100,7 @@ static int reset_simple_status(struct reset_controller_dev *rcdev,
>  const struct reset_control_ops reset_simple_ops = {
>  	.assert		= reset_simple_assert,
>  	.deassert	= reset_simple_deassert,
> +	.reset		= reset_simple_reset,
>  	.status		= reset_simple_status,
>  };
>  EXPORT_SYMBOL_GPL(reset_simple_ops);
> diff --git a/include/linux/reset/reset-simple.h b/include/linux/reset/reset-simple.h
> index 08ccb25a55e6..a5887f6cbe50 100644
> --- a/include/linux/reset/reset-simple.h
> +++ b/include/linux/reset/reset-simple.h
> @@ -27,6 +27,9 @@
>   * @status_active_low: if true, bits read back as cleared while the reset is
>   *                     asserted. Otherwise, bits read back as set while the
>   *                     reset is asserted.
> + * @reset_ms: Minimum delay in milliseconds needed that needs to be
> + *            waited for between an assert and a deassert to reset the
> + *            device.

If multiple consumers with different delay requirements are connected to
this reset controllers, this must the largest minimum delay. Could you
add mention for this in the comment?

regards
Philipp