Messages in this thread | | | Subject | Re: [RFC PATCH 12/21] reset: uniphier: add core support for UniPhier reset driver | From | Philipp Zabel <> | Date | Wed, 11 May 2016 12:34:34 +0200 |
| |
Am Mittwoch, den 11.05.2016, 11:46 +0900 schrieb Masahiro Yamada: > Hi Philipp, > > > 2016-05-10 22:54 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>: > > Am Dienstag, den 10.05.2016, 18:50 +0900 schrieb Masahiro Yamada: > > [...] > >> +static int uniphier_reset_update(struct reset_controller_dev *rcdev, > >> + unsigned long id, bool assert) > >> +{ > >> + struct uniphier_reset_priv *priv = to_uniphier_reset_priv(rcdev); > >> + const struct uniphier_reset_data *p; > >> + bool handled = false; > >> + > >> + for (p = priv->data; p->id != UNIPHIER_RESET_ID_END; p++) { > >> + unsigned int val; > >> + int ret; > >> + > >> + if (p->id != id) > >> + continue; > >> + > >> + val = p->deassert_val; > >> + if (assert) > >> + val = ~val; > >> + > >> + ret = regmap_write_bits(priv->regmap, p->reg, p->mask, val); > > > > What is the difference between mask and deassert_val? Couldn't you just > > assign > > val = assert ? 0 : p->mask; > > ? > > > I need to handle both active-high resets and active-low resets.
I see. I hadn't seen any active-high resets in your lists yet. If you need them, you obviously can't simplify this much.
> I thought two ways to do that. > > > [1] Have mask and a flag indicating active-low/active-high, > like follows: > > if (flag & UNIPHIER_RST_ACTIVE_LOW) > assert = !assert; > val = assert ? 0 : p->mask; > > [2] Have mask and deassert_val as in this patch > > [1] cannot manage a case where one register contains > active-low bits and active-high bits mixed in it. > > > > For example, let's say reset bits are BIT(1) and BIT(0). > > [2] can solve this case as follows: > > (a) If both bit1 and bit0 are active-high. > .mask = BIT(1) | BIT(0); > .deassert_val = 0; > > (b) If bit1 is active-high and bit0 is active-low > .mask = BIT(1) | BIT(0); > .deassert_val = BIT(0); > > (c) If bit1 is active-low and bit0 is active-high > .mask = BIT(1) | BIT(0); > .deassert_val = BIT(1); > > (d) If both bit1 and bit0 are active-low > .mask = BIT(1) | BIT(0); > .deassert_val = BIT(1) | BIT(0); > > > I have not been hit by such a complicated case though.
In general it is a good idea not to add complexity for theoretical cases, on the other hand [1] isn't really much less complex. Can I ask you to invert the logic, though, and use assert_val instead?
regards Philipp
| |