Hi Philipp,
2016-05-10 22:54 GMT+09:00 Philipp Zabel <p.za...@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 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.
>> +static const struct reset_control_ops uniphier_reset_ops = {
>> + .assert = uniphier_reset_assert,
>> + .deassert = uniphier_reset_deassert,
>> + .status = uniphier_reset_status,
>> +};
>> +
>> +int uniphier_reset_probe(struct platform_device *pdev,
>> + const struct uniphier_reset_data *data)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct uniphier_reset_priv *priv;
>> + const struct uniphier_reset_data *p;
>> + struct regmap *regmap;
>> + unsigned int nr_resets = 0;
>> +
>> + /* parent should be MFD and syscon node */
>> + regmap = syscon_node_to_regmap(dev->parent->of_node);
>> + if (IS_ERR(regmap)) {
>> + dev_err(dev, "failed to get regmap\n");
>
> syscon_node_to_regmap can return different error codes. It might be
> helpful to use
> dev_err(dev, "failed to get regmap: %d\n", PTR_ERR(regmap));
> here.
OK. Will do.
--
Best Regards
Masahiro Yamada
