| Message ID | 20190404035140.3721-5-david.wu@rock-chips.com |
|---|---|
| State | Superseded |
| Delegated to: | Philipp Tomsich |
| Headers | show |
| Series | pinctrl: Split the common mux/drive/pull/schmitt func into per Soc | expand |
> On 04.04.2019, at 05:51, David Wu <david.wu@rock-chips.com> wrote: > > RK3288 pmu_gpio0 iomux setting have no higher 16 writing corresponding > bits, need to read before write the register. > > Signed-off-by: David Wu <david.wu@rock-chips.com> > --- > > drivers/pinctrl/rockchip/pinctrl-rk3288.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/rockchip/pinctrl-rk3288.c b/drivers/pinctrl/rockchip/pinctrl-rk3288.c > index 1fa601d954..d66ffdf24b 100644 > --- a/drivers/pinctrl/rockchip/pinctrl-rk3288.c > +++ b/drivers/pinctrl/rockchip/pinctrl-rk3288.c > @@ -54,7 +54,13 @@ static int rk3288_set_mux(struct rockchip_pin_bank *bank, int pin, int mux) > } > } > > - data = (mask << (bit + 16)); > + if (bank->bank_num == 0) { > + regmap_read(regmap, reg, &data); Could you pull the regmap_read out of the if and make it common for all cases, so the differences between the paths are in data-manipulation only? > + data &= ~(mask << bit); > + } else { > + data = (mask << (bit + 16)); > + } > + Please add a comment, so readers will be able to understand what is happening (and why) without referring to the TRM. > data |= (mux & mask) << bit; > ret = regmap_write(regmap, reg, data); > > -- > 2.19.1 > > >
Hi Philipp, 在 2019/4/4 下午3:19, Philipp Tomsich 写道: > > >> On 04.04.2019, at 05:51, David Wu <david.wu@rock-chips.com> wrote: >> >> RK3288 pmu_gpio0 iomux setting have no higher 16 writing corresponding >> bits, need to read before write the register. >> >> Signed-off-by: David Wu <david.wu@rock-chips.com> >> --- >> >> drivers/pinctrl/rockchip/pinctrl-rk3288.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pinctrl/rockchip/pinctrl-rk3288.c b/drivers/pinctrl/rockchip/pinctrl-rk3288.c >> index 1fa601d954..d66ffdf24b 100644 >> --- a/drivers/pinctrl/rockchip/pinctrl-rk3288.c >> +++ b/drivers/pinctrl/rockchip/pinctrl-rk3288.c >> @@ -54,7 +54,13 @@ static int rk3288_set_mux(struct rockchip_pin_bank *bank, int pin, int mux) >> } >> } >> >> - data = (mask << (bit + 16)); >> + if (bank->bank_num == 0) { >> + regmap_read(regmap, reg, &data); > > Could you pull the regmap_read out of the if and make it common for all cases, so the differences between the paths are in data-manipulation only? Yes, the difference between the gpio0 and other pins is the data-manipulation, and i think the others don't need the regmap_read, so it is not a common case. > >> + data &= ~(mask << bit); >> + } else { >> + data = (mask << (bit + 16)); >> + } >> + > > Please add a comment, so readers will be able to understand what is happening (and why) without referring to the TRM > >> data |= (mux & mask) << bit; >> ret = regmap_write(regmap, reg, data); >> >> -- >> 2.19.1 >> >> >> > > >
Am Donnerstag, 4. April 2019, 10:16:02 CEST schrieb David Wu: > Hi Philipp, > > 在 2019/4/4 下午3:19, Philipp Tomsich 写道: > > > > > >> On 04.04.2019, at 05:51, David Wu <david.wu@rock-chips.com> wrote: > >> > >> RK3288 pmu_gpio0 iomux setting have no higher 16 writing corresponding > >> bits, need to read before write the register. > >> > >> Signed-off-by: David Wu <david.wu@rock-chips.com> > >> --- > >> > >> drivers/pinctrl/rockchip/pinctrl-rk3288.c | 8 +++++++- > >> 1 file changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/pinctrl/rockchip/pinctrl-rk3288.c b/drivers/pinctrl/rockchip/pinctrl-rk3288.c > >> index 1fa601d954..d66ffdf24b 100644 > >> --- a/drivers/pinctrl/rockchip/pinctrl-rk3288.c > >> +++ b/drivers/pinctrl/rockchip/pinctrl-rk3288.c > >> @@ -54,7 +54,13 @@ static int rk3288_set_mux(struct rockchip_pin_bank *bank, int pin, int mux) > >> } > >> } > >> > >> - data = (mask << (bit + 16)); > >> + if (bank->bank_num == 0) { > >> + regmap_read(regmap, reg, &data); > > > > Could you pull the regmap_read out of the if and make it common for all cases, so the differences between the paths are in data-manipulation only? > > Yes, the difference between the gpio0 and other pins is the > data-manipulation, and i think the others don't need the regmap_read, > so it is not a common case. yep ... the other pinmuxes are using hiword-mask registers while only gpio0 (in the pmu-area) needs the to get the read-modify-write scheme.
diff --git a/drivers/pinctrl/rockchip/pinctrl-rk3288.c b/drivers/pinctrl/rockchip/pinctrl-rk3288.c index 1fa601d954..d66ffdf24b 100644 --- a/drivers/pinctrl/rockchip/pinctrl-rk3288.c +++ b/drivers/pinctrl/rockchip/pinctrl-rk3288.c @@ -54,7 +54,13 @@ static int rk3288_set_mux(struct rockchip_pin_bank *bank, int pin, int mux) } } - data = (mask << (bit + 16)); + if (bank->bank_num == 0) { + regmap_read(regmap, reg, &data); + data &= ~(mask << bit); + } else { + data = (mask << (bit + 16)); + } + data |= (mux & mask) << bit; ret = regmap_write(regmap, reg, data);
RK3288 pmu_gpio0 iomux setting have no higher 16 writing corresponding bits, need to read before write the register. Signed-off-by: David Wu <david.wu@rock-chips.com> --- drivers/pinctrl/rockchip/pinctrl-rk3288.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)