| Message ID | 1434506172-4401-1-git-send-email-jun.nie@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 17 June 2015 at 15:17, Tony Lindgren <tony@atomide.com> wrote: > * Jun Nie <jun.nie@linaro.org> [150616 18:58]: >> Support GPIO for one register control multiple pins case >> with calculating register offset first, then bit offset. >> >> Signed-off-by: Jun Nie <jun.nie@linaro.org> >> --- >> drivers/pinctrl/pinctrl-single.c | 22 ++++++++++++++++++---- >> 1 file changed, 18 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c >> index 13b45f2..bd69d9a 100644 >> --- a/drivers/pinctrl/pinctrl-single.c >> +++ b/drivers/pinctrl/pinctrl-single.c >> @@ -494,7 +494,7 @@ static int pcs_request_gpio(struct pinctrl_dev *pctldev, >> struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev); >> struct pcs_gpiofunc_range *frange = NULL; >> struct list_head *pos, *tmp; >> - int mux_bytes = 0; >> + int offset, mux_bytes = 0; >> unsigned data; >> >> /* If function mask is null, return directly. */ >> @@ -507,9 +507,23 @@ static int pcs_request_gpio(struct pinctrl_dev *pctldev, >> || pin < frange->offset) >> continue; >> mux_bytes = pcs->width / BITS_PER_BYTE; >> - data = pcs->read(pcs->base + pin * mux_bytes) & ~pcs->fmask; >> - data |= frange->gpiofunc; >> - pcs->write(data, pcs->base + pin * mux_bytes); >> + if (pcs->bits_per_mux) { >> + int pin_pos, byte_num, num_pins_in_register; >> + >> + num_pins_in_register = pcs->width / pcs->bits_per_pin; >> + byte_num = (pcs->bits_per_pin * pin) / BITS_PER_BYTE; >> + offset = (byte_num / mux_bytes) * mux_bytes; >> + pin_pos = pin % num_pins_in_register; >> + pin_pos *= pcs->bits_per_pin; >> + data = pcs->read(pcs->base + offset) & >> + ~(pcs->fmask << pin_pos); > > Should you check the pcs->fmask here too in case some bits are reserved? > >> + data |= frange->gpiofunc << pin_pos; >> + } else { >> + offset = pin * mux_bytes; >> + data = pcs->read(pcs->base + offset) & ~pcs->fmask; >> + data |= frange->gpiofunc; >> + } >> + pcs->write(data, pcs->base + offset); >> break; >> } >> return 0; > > Other than that looks OK to me, would be good to also wait for Haojian's > comments here. > I'm fine on this. Reviewed-by: Haojian Zhuang <haojian.zhuang@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
2015-06-17 15:17 GMT+08:00 Tony Lindgren <tony@atomide.com>: > * Jun Nie <jun.nie@linaro.org> [150616 18:58]: >> Support GPIO for one register control multiple pins case >> with calculating register offset first, then bit offset. >> >> Signed-off-by: Jun Nie <jun.nie@linaro.org> >> --- >> drivers/pinctrl/pinctrl-single.c | 22 ++++++++++++++++++---- >> 1 file changed, 18 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c >> index 13b45f2..bd69d9a 100644 >> --- a/drivers/pinctrl/pinctrl-single.c >> +++ b/drivers/pinctrl/pinctrl-single.c >> @@ -494,7 +494,7 @@ static int pcs_request_gpio(struct pinctrl_dev *pctldev, >> struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev); >> struct pcs_gpiofunc_range *frange = NULL; >> struct list_head *pos, *tmp; >> - int mux_bytes = 0; >> + int offset, mux_bytes = 0; >> unsigned data; >> >> /* If function mask is null, return directly. */ >> @@ -507,9 +507,23 @@ static int pcs_request_gpio(struct pinctrl_dev *pctldev, >> || pin < frange->offset) >> continue; >> mux_bytes = pcs->width / BITS_PER_BYTE; >> - data = pcs->read(pcs->base + pin * mux_bytes) & ~pcs->fmask; >> - data |= frange->gpiofunc; >> - pcs->write(data, pcs->base + pin * mux_bytes); >> + if (pcs->bits_per_mux) { >> + int pin_pos, byte_num, num_pins_in_register; >> + >> + num_pins_in_register = pcs->width / pcs->bits_per_pin; >> + byte_num = (pcs->bits_per_pin * pin) / BITS_PER_BYTE; >> + offset = (byte_num / mux_bytes) * mux_bytes; >> + pin_pos = pin % num_pins_in_register; >> + pin_pos *= pcs->bits_per_pin; >> + data = pcs->read(pcs->base + offset) & >> + ~(pcs->fmask << pin_pos); > > Should you check the pcs->fmask here too in case some bits are reserved? > Did not catch your idea? Those bits set in fmask are dedicated for one pin mux control and should be clear before set as desired value per my understanding. Do you mean some bits may be reserved and not for any function? >> + data |= frange->gpiofunc << pin_pos; >> + } else { >> + offset = pin * mux_bytes; >> + data = pcs->read(pcs->base + offset) & ~pcs->fmask; >> + data |= frange->gpiofunc; >> + } >> + pcs->write(data, pcs->base + offset); >> break; >> } >> return 0; > > Other than that looks OK to me, would be good to also wait for Haojian's > comments here. > > Regards, > > Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-06-23 18:14 GMT+08:00 Tony Lindgren <tony@atomide.com>: > * Jun Nie <jun.nie@linaro.org> [150623 02:56]: >> 2015-06-17 15:17 GMT+08:00 Tony Lindgren <tony@atomide.com>: >> > * Jun Nie <jun.nie@linaro.org> [150616 18:58]: >> >> + if (pcs->bits_per_mux) { >> >> + int pin_pos, byte_num, num_pins_in_register; >> >> + >> >> + num_pins_in_register = pcs->width / pcs->bits_per_pin; >> >> + byte_num = (pcs->bits_per_pin * pin) / BITS_PER_BYTE; >> >> + offset = (byte_num / mux_bytes) * mux_bytes; >> >> + pin_pos = pin % num_pins_in_register; >> >> + pin_pos *= pcs->bits_per_pin; >> >> + data = pcs->read(pcs->base + offset) & >> >> + ~(pcs->fmask << pin_pos); >> > >> > Should you check the pcs->fmask here too in case some bits are reserved? >> > >> Did not catch your idea? Those bits set in fmask are dedicated for one >> pin mux control and should be clear before set as desired value per my >> understanding. Do you mean some bits may be reserved and not for any >> function? > > Right, can you please check that we don't try to write to reserved > bits in the hardawre if the mask is set? Then I have question that how can I know what bits is for function mask, what bits are for reserved? Do we have any other value to indicate it? I did not find it in one register for one pin mux case. > > Regards, > > Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-06-23 18:18 GMT+08:00 Jun Nie <jun.nie@linaro.org>: > 2015-06-23 18:14 GMT+08:00 Tony Lindgren <tony@atomide.com>: >> * Jun Nie <jun.nie@linaro.org> [150623 02:56]: >>> 2015-06-17 15:17 GMT+08:00 Tony Lindgren <tony@atomide.com>: >>> > * Jun Nie <jun.nie@linaro.org> [150616 18:58]: >>> >> + if (pcs->bits_per_mux) { >>> >> + int pin_pos, byte_num, num_pins_in_register; >>> >> + >>> >> + num_pins_in_register = pcs->width / pcs->bits_per_pin; >>> >> + byte_num = (pcs->bits_per_pin * pin) / BITS_PER_BYTE; >>> >> + offset = (byte_num / mux_bytes) * mux_bytes; >>> >> + pin_pos = pin % num_pins_in_register; >>> >> + pin_pos *= pcs->bits_per_pin; >>> >> + data = pcs->read(pcs->base + offset) & >>> >> + ~(pcs->fmask << pin_pos); >>> > >>> > Should you check the pcs->fmask here too in case some bits are reserved? >>> > >>> Did not catch your idea? Those bits set in fmask are dedicated for one >>> pin mux control and should be clear before set as desired value per my >>> understanding. Do you mean some bits may be reserved and not for any >>> function? >> >> Right, can you please check that we don't try to write to reserved >> bits in the hardawre if the mask is set? > Then I have question that how can I know what bits is for function > mask, what bits are for reserved? Do we have any other value to > indicate it? I did not find it in one register for one pin mux case. Tony, Could you help elaborate this? Thanks! > >> >> Regards, >> >> Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-07-06 17:03 GMT+08:00 Tony Lindgren <tony@atomide.com>: > * Jun Nie <jun.nie@linaro.org> [150706 01:43]: >> 2015-06-23 18:18 GMT+08:00 Jun Nie <jun.nie@linaro.org>: >> > 2015-06-23 18:14 GMT+08:00 Tony Lindgren <tony@atomide.com>: >> >> * Jun Nie <jun.nie@linaro.org> [150623 02:56]: >> >>> 2015-06-17 15:17 GMT+08:00 Tony Lindgren <tony@atomide.com>: >> >>> > * Jun Nie <jun.nie@linaro.org> [150616 18:58]: >> >>> >> + if (pcs->bits_per_mux) { >> >>> >> + int pin_pos, byte_num, num_pins_in_register; >> >>> >> + >> >>> >> + num_pins_in_register = pcs->width / pcs->bits_per_pin; >> >>> >> + byte_num = (pcs->bits_per_pin * pin) / BITS_PER_BYTE; >> >>> >> + offset = (byte_num / mux_bytes) * mux_bytes; >> >>> >> + pin_pos = pin % num_pins_in_register; >> >>> >> + pin_pos *= pcs->bits_per_pin; >> >>> >> + data = pcs->read(pcs->base + offset) & >> >>> >> + ~(pcs->fmask << pin_pos); >> >>> > >> >>> > Should you check the pcs->fmask here too in case some bits are reserved? >> >>> > >> >>> Did not catch your idea? Those bits set in fmask are dedicated for one >> >>> pin mux control and should be clear before set as desired value per my >> >>> understanding. Do you mean some bits may be reserved and not for any >> >>> function? >> >> >> >> Right, can you please check that we don't try to write to reserved >> >> bits in the hardawre if the mask is set? >> >> > Then I have question that how can I know what bits is for function >> > mask, what bits are for reserved? Do we have any other value to >> > indicate it? I did not find it in one register for one pin mux case. >> >> Could you help elaborate this? Thanks! > > We can only write to the bits specified in pinctrl-single,function-mask. > I see, you want below mask to make sure gpiofunc value does not exceed expected bits though it should be safe if dts data is correct. Right? + data = pcs->read(pcs->base + offset) & + ~(pcs->fmask << pin_pos); + data |= (pcs->fmask & frange->gpiofunc) << pin_pos; > Regards, > > Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 13b45f2..bd69d9a 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -494,7 +494,7 @@ static int pcs_request_gpio(struct pinctrl_dev *pctldev, struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev); struct pcs_gpiofunc_range *frange = NULL; struct list_head *pos, *tmp; - int mux_bytes = 0; + int offset, mux_bytes = 0; unsigned data; /* If function mask is null, return directly. */ @@ -507,9 +507,23 @@ static int pcs_request_gpio(struct pinctrl_dev *pctldev, || pin < frange->offset) continue; mux_bytes = pcs->width / BITS_PER_BYTE; - data = pcs->read(pcs->base + pin * mux_bytes) & ~pcs->fmask; - data |= frange->gpiofunc; - pcs->write(data, pcs->base + pin * mux_bytes); + if (pcs->bits_per_mux) { + int pin_pos, byte_num, num_pins_in_register; + + num_pins_in_register = pcs->width / pcs->bits_per_pin; + byte_num = (pcs->bits_per_pin * pin) / BITS_PER_BYTE; + offset = (byte_num / mux_bytes) * mux_bytes; + pin_pos = pin % num_pins_in_register; + pin_pos *= pcs->bits_per_pin; + data = pcs->read(pcs->base + offset) & + ~(pcs->fmask << pin_pos); + data |= frange->gpiofunc << pin_pos; + } else { + offset = pin * mux_bytes; + data = pcs->read(pcs->base + offset) & ~pcs->fmask; + data |= frange->gpiofunc; + } + pcs->write(data, pcs->base + offset); break; } return 0;
Support GPIO for one register control multiple pins case with calculating register offset first, then bit offset. Signed-off-by: Jun Nie <jun.nie@linaro.org> --- drivers/pinctrl/pinctrl-single.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)