Re: [PATCH RESEND 3/7] clk: spacemit: add reset controller support

[Yixun Lan <dlan@gentoo.org>]

Hi Alex:

On 10:18 Fri 21 Mar     , Alex Elder wrote:
> Define ccu_reset_data as a structure that contains the constant
> register offset and bitmasks used to assert and deassert a reset
> control on a SpacemiT K1 CCU. Define ccu_reset_controller_data as
> a structure that contains the address of an array of those structures
> and a count of the number of elements in the array.
> 
> Add a pointer to a ccu_reset_controller_data structure to the
> k1_ccu_data structure.  Reset support is optional for SpacemiT CCUs;
> the new pointer field will be null for CCUs without any resets.
> 
> Finally, define a new ccu_reset_controller structure, which (for
> a CCU with resets) contains a pointer to the constant reset data,
> the regmap to be used for the controller, and an embedded a reset
> controller structure.
> 
> Each reset control is asserted or deasserted by updating bits in
> a register.  The bits used are defined by an assert mask and a
> deassert mask.  In some cases, one (non-zero) mask asserts reset
> and a different (non-zero) mask deasserts it.  Otherwise one mask
> is nonzero, and the other is zero.  Either way, the bits in
> both masks are cleared, then either the assert mask or the deassert
> mask is set in a register to affect the state of a reset control.
> 
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
>  drivers/clk/spacemit/ccu-k1.c | 93 +++++++++++++++++++++++++++++++++++
>  1 file changed, 93 insertions(+)
> 
> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
> index f7367271396a0..6d879411c6c05 100644
> --- a/drivers/clk/spacemit/ccu-k1.c
> +++ b/drivers/clk/spacemit/ccu-k1.c
> @@ -10,6 +10,7 @@
>  #include <linux/minmax.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
>  
>  #include "ccu_common.h"
>  #include "ccu_pll.h"
> @@ -134,8 +135,26 @@ struct spacemit_ccu_clk {
>  	struct clk_hw *hw;
>  };
>  
> +struct ccu_reset_data {
> +	u32 offset;
> +	u32 assert_mask;
> +	u32 deassert_mask;
> +};
> +
> +struct ccu_reset_controller_data {
> +	u32 count;
> +	const struct ccu_reset_data *data;	/* array */
> +};
> +
>  struct k1_ccu_data {
>  	struct spacemit_ccu_clk *clk;		/* array with sentinel */
> +	const struct ccu_reset_controller_data *rst_data;
> +};
> +
> +struct ccu_reset_controller {
> +	struct regmap *regmap;
> +	const struct ccu_reset_controller_data *data;
> +	struct reset_controller_dev rcdev;
>  };
>  
>  /*	APBS clocks start	*/
> @@ -1630,6 +1649,48 @@ static const struct k1_ccu_data k1_ccu_apmu_data = {
>  	.clk		= k1_ccu_apmu_clks,
>  };
>  
> +static struct ccu_reset_controller *
> +rcdev_to_controller(struct reset_controller_dev *rcdev)
I'd suggest to avoid the line break to make it slightly more readable, intuitive
as the 80 column limit isn't hard rule

there are maybe more place similar to this, I won't add more comments
https://github.com/torvalds/linux/commit/bdc48fa11e46f867ea4d75fa59ee87a7f48be144

> +{
> +	return container_of(rcdev, struct ccu_reset_controller, rcdev);
> +}
since this function is only used once, open-code it?
but I'd fine with either way if you prefer to keep it

> +
> +static int
> +k1_rst_update(struct reset_controller_dev *rcdev, unsigned long id, bool assert)
s/k1_rst_update/k1_reset_update/g
this is a taste change, but I found more people follow this when grep driver/reset
> +{
> +	struct ccu_reset_controller *controller = rcdev_to_controller(rcdev);
> +	struct regmap *regmap = controller->regmap;
> +	const struct ccu_reset_data *data;
> +	u32 val;
> +	int ret;
> +
> +	data = &controller->data->data[id];
> +
> +	ret = regmap_read(regmap, data->offset, &val);
> +	if (ret)
> +		return ret;
> +
> +	val &= ~(data->assert_mask | data->deassert_mask);
> +	val |= assert ? data->assert_mask : data->deassert_mask;
> +
> +	return regmap_write(regmap, data->offset, val);
> +}
> +
> +static int k1_rst_assert(struct reset_controller_dev *rcdev, unsigned long id)
same reason, rst -> reset, more below
> +{
> +	return k1_rst_update(rcdev, id, true);
> +}
> +
> +static int k1_rst_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	return k1_rst_update(rcdev, id, false);
> +}
> +
> +static const struct reset_control_ops k1_reset_control_ops = {
> +	.assert		= k1_rst_assert,
> +	.deassert	= k1_rst_deassert,
> +};
> +
>  static int k1_ccu_register(struct device *dev, struct regmap *regmap,
>  			   struct regmap *lock_regmap,
>  			   struct spacemit_ccu_clk *clks)
> @@ -1675,6 +1736,33 @@ static int k1_ccu_register(struct device *dev, struct regmap *regmap,
>  	return ret;
>  }
>  
> +static int
> +k1_reset_controller_register(struct device *dev, struct regmap *regmap,
> +			     const struct ccu_reset_controller_data *data)
> +{
> +	struct ccu_reset_controller *controller;
> +	struct reset_controller_dev *rcdev;
> +
> +	/* Resets are optional */
> +	if (!data)
> +		return 0;
> +
> +	controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL);
> +	if (!controller)
> +		return -ENOMEM;
> +
> +	controller->regmap = regmap;
> +	controller->data = data;
> +
> +	rcdev = &controller->rcdev;
..
> +	rcdev->owner = THIS_MODULE;
move to last?
> +	rcdev->nr_resets = data->count;
> +	rcdev->ops = &k1_reset_control_ops;
> +	rcdev->of_node = dev->of_node;
> +
> +	return devm_reset_controller_register(dev, rcdev);
> +}
> +
>  static int k1_ccu_probe(struct platform_device *pdev)
>  {
>  	struct regmap *base_regmap, *lock_regmap = NULL;
> @@ -1710,6 +1798,11 @@ static int k1_ccu_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to register clocks\n");
>  
> +	ret = k1_reset_controller_register(dev, base_regmap, data->rst_data);
..
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to register reset controller\n");
same 100 column reason
> +
>  	return 0;
>  }
>  
> -- 
> 2.43.0
> 

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55