On Wed, 2017-10-18 at 14:00 +0100, Andre Przywara wrote:
> Hi,
Thank you for the review.
> On 17/10/17 14:03, Philipp Zabel wrote:
> > Add reset line status readback, inverted status support, and socfpga
> > device tree quirks to the simple reset driver, and use it to replace
> > the socfpga driver.
> >
> > Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
> > ---
> > Changes since v3:
> > - Rebased onto reset/next
> > - Only warn about missing altr,modrst-offset property on socfpga
> > ---
> > drivers/reset/Kconfig | 10 +--
> > drivers/reset/Makefile | 1 -
> > drivers/reset/reset-simple.c | 51 +++++++++++++-
> > drivers/reset/reset-simple.h | 4 ++
> > drivers/reset/reset-socfpga.c | 157
> > ------------------------------------------
> > 5 files changed, 56 insertions(+), 167 deletions(-)
> > delete mode 100644 drivers/reset/reset-socfpga.c
> >
[...]
> > diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
> > index a5119457cec61..98ff0f924948e 100644
> > --- a/drivers/reset/reset-simple.c
> > +++ b/drivers/reset/reset-simple.c
> > @@ -68,25 +68,58 @@ static int reset_simple_deassert(struct
> > reset_controller_dev *rcdev,
> > return reset_simple_update(rcdev, id, false);
> > }
> >
> > +static int reset_simple_status(struct reset_controller_dev *rcdev,
> > + 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);
> > + u32 reg;
> > +
> > + reg = readl(data->membase + (bank * reg_width));
> > +
> > + return !(reg & BIT(offset)) ^ !data->status_active_low;
> > +}
> > +
> > const struct reset_control_ops reset_simple_ops = {
> > .assert = reset_simple_assert,
> > .deassert = reset_simple_deassert,
> > + .status = reset_simple_status,
> > };
> >
> > /**
> > * struct reset_simple_devdata - simple reset controller properties
> > + * @reg_offset: offset between base address and first reset register.
> > + * @nr_resets: number of resets. If not set, default to resource size in
> > bits.
> > * @active_low: if true, bits are cleared to assert the reset. Otherwise,
> > bits
> > * are set to assert the reset.
> > + * @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.
> > */
> > struct reset_simple_devdata {
> > + u32 reg_offset;
> > + u32 nr_resets;
> > bool active_low;
> > + bool status_active_low;
> > +};
> > +
> > +#define SOCFPGA_NR_BANKS 8
> > +
> > +static const struct reset_simple_devdata reset_simple_socfpga = {
> > + .reg_offset = 0x10,
Here reset_simple_socfpga.reg_offset is set to the default of 0x10.
> > + .nr_resets = SOCFPGA_NR_BANKS * 32,
> > + .status_active_low = true,
> > };
> >
> > static const struct reset_simple_devdata reset_simple_active_low = {
> > .active_low = true,
> > + .status_active_low = true,
> > };
> >
> > static const struct of_device_id reset_simple_dt_ids[] = {
> > + { .compatible = "altr,rst-mgr", .data = &reset_simple_socfpga },
> > { .compatible = "allwinner,sun6i-a31-clock-reset",
> > .data = &reset_simple_active_low },
> > { /* sentinel */ },
> > @@ -99,6 +132,7 @@ static int reset_simple_probe(struct platform_device
> > *pdev)
> > struct reset_simple_data *data;
> > void __iomem *membase;
> > struct resource *res;
> > + u32 reg_offset = 0;
> >
> > devdata = of_device_get_match_data(dev);
> >
> > @@ -118,8 +152,23 @@ static int reset_simple_probe(struct platform_device
> > *pdev)
> > data->rcdev.ops = &reset_simple_ops;
> > data->rcdev.of_node = dev->of_node;
> >
> > - if (devdata)
> > + if (devdata) {
> > + reg_offset = devdata->reg_offset;
And here reg_offset is set to the default of 0x10 on socfpga.
> > + if (devdata->nr_resets)
> > + data->rcdev.nr_resets = devdata->nr_resets;
> > data->active_low = devdata->active_low;
> > + data->status_active_low = devdata->status_active_low;
> > + }
> > +
> > + if (devdata == &reset_simple_socfpga &&
>
> Mmh, this pointer comparison looks a bit dodgy. Isn't
> of_device_is_compatible() the right solution here?
My thinking was, the of_device_is_compatible is already called inside
of_device_get_match_data, so why call it again and not reuse the result?
> Also semantically, as the property is tied to a certain compatible
> string (and not to our data structure)?
I agree with this, though. I'll change this line to say:
+ if (of_device_is_compatible(dev->of_node, "altr,rst-mgr") &&
instead.
> > + of_property_read_u32(dev->of_node, "altr,modrst-offset",
> > + dev_warn(dev,
> > + "missing altr,modrst-offset property, assuming
> > 0x%x!\n",
> > + reg_offset);
>
> The existing driver falls back to 0x10 if no explicit property is found.
> Should we copy this here for compatibility reasons? We could simply use:
> "reg_offset = 0x10;" before the dev_warn().
See above.
regards
Philipp
