Re: [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon device compatible

[Philipp Zabel]

On Do, 2025-02-13 at 22:58 -0800, Wilson Ding wrote:
> Introduce the new ops for updating reset line and getting status. Thus,
> the reset controller can be accessed through either direct I/O or regmap
> interfaces.

Please don't add a new layer of function pointer indirection, just add
a new struct reset_control_ops for the regmap variant.

> It enables the support of the syscon devices with the simple
> reset code. To adapt the DT binding of the syscon device, the number of
> reset lines must be specified in device data.

If the DT node had a reg property, number of reset lines could be
determined from its size, like for MMIO.

> Signed-off-by: Wilson Ding <dingwei@xxxxxxxxxxx>
> ---
>  drivers/reset/reset-simple.c       | 117 +++++++++++++++++++++++------
>  include/linux/reset/reset-simple.h |  11 +++
>  2 files changed, 107 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
> index 276067839830..e4e777d40a79 100644
> --- a/drivers/reset/reset-simple.c
> +++ b/drivers/reset/reset-simple.c
> @@ -15,8 +15,10 @@
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/regmap.h>
>  #include <linux/reset-controller.h>
>  #include <linux/reset/reset-simple.h>
>  #include <linux/spinlock.h>
> @@ -27,10 +29,9 @@ to_reset_simple_data(struct reset_controller_dev *rcdev)
>  	return container_of(rcdev, struct reset_simple_data, rcdev);
>  }
>  
> -static int reset_simple_update(struct reset_controller_dev *rcdev,
> +static int reset_simple_update_mmio(struct reset_simple_data *data,
>  			       unsigned long id, bool assert)

No need to rename or change the function prototype.

>  {
> -	struct reset_simple_data *data = to_reset_simple_data(rcdev);
>  	int reg_width = sizeof(u32);
>  	int bank = id / (reg_width * BITS_PER_BYTE);
>  	int offset = id % (reg_width * BITS_PER_BYTE);
> @@ -51,16 +52,40 @@ static int reset_simple_update(struct reset_controller_dev *rcdev,
>  	return 0;
>  }
>  
> +static int reset_simple_update_regmap(struct reset_simple_data *data,
> +				      unsigned long id, bool assert)

I'd call this reset_simple_regmap_update().

> +{
> +	int reg_width = sizeof(u32);
> +	int bank = id / (reg_width * BITS_PER_BYTE);
> +	int offset = id % (reg_width * BITS_PER_BYTE);
> +	u32 mask, val;
> +
> +	mask = BIT(offset);
> +
> +	if (assert ^ data->active_low)
> +		val = mask;
> +	else
> +		val = 0;
> +
> +	return regmap_write_bits(data->regmap,
> +				 data->reg_offset + (bank * reg_width),
> +				 mask, val);
> +}
> +
>  static int reset_simple_assert(struct reset_controller_dev *rcdev,
>  			       unsigned long id)
>  {
> -	return reset_simple_update(rcdev, id, true);
> +	struct reset_simple_data *data = to_reset_simple_data(rcdev);
> +
> +	return data->ops.update(data, id, true);
>  }
>  
>  static int reset_simple_deassert(struct reset_controller_dev *rcdev,
>  				 unsigned long id)
>  {
> -	return reset_simple_update(rcdev, id, false);
> +	struct reset_simple_data *data = to_reset_simple_data(rcdev);
> +
> +	return data->ops.update(data, id, false);
>  }

No need for indirection. Better to just add separate
reset_simple_regmap_assert/deassert() functions.

>  static int reset_simple_reset(struct reset_controller_dev *rcdev,
> @@ -81,10 +106,9 @@ static int reset_simple_reset(struct reset_controller_dev *rcdev,
>  	return reset_simple_deassert(rcdev, id);
>  }
>  
> -static int reset_simple_status(struct reset_controller_dev *rcdev,
> -			       unsigned long id)
> +static int reset_simple_status_mmio(struct reset_simple_data *data,
> +			     unsigned long id)
>  {
> -	struct reset_simple_data *data = to_reset_simple_data(rcdev);
>  	int reg_width = sizeof(u32);
>  	int bank = id / (reg_width * BITS_PER_BYTE);
>  	int offset = id % (reg_width * BITS_PER_BYTE);
> @@ -95,6 +119,31 @@ static int reset_simple_status(struct reset_controller_dev *rcdev,
>  	return !(reg & BIT(offset)) ^ !data->status_active_low;
>  }
>  
> +static int reset_simple_status_regmap(struct reset_simple_data *data,
> +				    unsigned long id)
> +{
> +	int reg_width = sizeof(u32);
> +	int bank = id / (reg_width * BITS_PER_BYTE);
> +	int offset = id % (reg_width * BITS_PER_BYTE);
> +	u32 reg;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, data->reg_offset + (bank * reg_width),
> +			  &reg);
> +	if (ret)
> +		return ret;
> +
> +	return !(reg & BIT(offset)) ^ !data->status_active_low;
> +}
> +
> +static int reset_simple_status(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct reset_simple_data *data = to_reset_simple_data(rcdev);
> +
> +	return data->ops.status(data, id);
> +}

Same as above, no need for indirection.
Just add separate reset_simple_regmap_assert/deassert() functions ...

> +
>  const struct reset_control_ops reset_simple_ops = {
>  	.assert		= reset_simple_assert,
>  	.deassert	= reset_simple_deassert,

... and add a const struct reset_control_ops reset_simple_regmap_ops.

regards
Philipp