> -----Original Message-----
> From: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Sent: Friday, February 14, 2025 3:54 AM
> To: Wilson Ding <dingwei@xxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: andrew@xxxxxxx; gregory.clement@xxxxxxxxxxx;
> sebastian.hesselbarth@xxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; Sanghoon Lee <salee@xxxxxxxxxxx>; Geethasowjanya
> Akula <gakula@xxxxxxxxxxx>
> Subject: [EXTERNAL] Re: [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon
> device compatible
>
> 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.
>
If just adding a new struct reset_control_ops for the regmap variant, almost
all the functions will be duplicated for regmap variant.
Besides reset_simple_regmap_assert/deassert(), we also need to have the
regmap version of reset_simple_update(). Since reset_simple_reset() invokes
reset_simple_regmap_assert/deassert(), it also needs to be duplicated.
In this case, there will be too many redundant codes in this file. I doubt if
it is worth to use the reset simple code. Maybe it's better to fork a new file
for the syscon device, such as 'reset-simple-syscon.c'. What do you say?
> > 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.
>
See my reply to the first comment.
> > 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),
> > + ®);
> > + 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 ...
>
See my reply to the first comment.
> > +
> > 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.
>
See my reply to the first comment.
> regards
> Philipp
- Wilson
