| Message ID | 1516575793-30526-1-git-send-email-david@lechnology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 22, 2018 at 1:03 AM, David Lechner <david@lechnology.com> wrote: > This fixes pcs_request_gpio() in the pinctrl-single driver when > bits_per_mux != 0. It appears this was overlooked when the multiple > pins per register feature was added. > > Fixes: 4e7e8017a80e ("pinctrl: pinctrl-single: enhance to configure > multiple pins of different modules") One line? > + byte_num = (pcs->bits_per_pin * pin) / BITS_PER_BYTE; > + offset = (byte_num / mux_bytes) * mux_bytes; > + pin_shift = pin % (pcs->width / pcs->bits_per_pin) * > + pcs->bits_per_pin; Sounds like playing around pretty well defined macro and functions, e.g. DIV_ROUND_UP(), round_up().
On 01/22/2018 08:49 AM, Andy Shevchenko wrote: > On Mon, Jan 22, 2018 at 1:03 AM, David Lechner <david@lechnology.com> wrote: >> This fixes pcs_request_gpio() in the pinctrl-single driver when >> bits_per_mux != 0. It appears this was overlooked when the multiple >> pins per register feature was added. >> >> Fixes: 4e7e8017a80e ("pinctrl: pinctrl-single: enhance to configure >> multiple pins of different modules") > > One line? One line is more important that wrapping to 75 chars? > >> + byte_num = (pcs->bits_per_pin * pin) / BITS_PER_BYTE; >> + offset = (byte_num / mux_bytes) * mux_bytes; >> + pin_shift = pin % (pcs->width / pcs->bits_per_pin) * >> + pcs->bits_per_pin; > > Sounds like playing around pretty well defined macro and functions, > e.g. DIV_ROUND_UP(), round_up(). > I admit, I just copied existing code (which may be a reason to leave this the way it is). But, I only see once place to do this: offset = round_down(byte_num, mux_bytes); Did I miss another?
On Mon, Jan 22, 2018 at 7:06 PM, David Lechner <david@lechnology.com> wrote: > On 01/22/2018 08:49 AM, Andy Shevchenko wrote: >> On Mon, Jan 22, 2018 at 1:03 AM, David Lechner <david@lechnology.com> >> wrote: >>> Fixes: 4e7e8017a80e ("pinctrl: pinctrl-single: enhance to configure >>> multiple pins of different modules") >> One line? > One line is more important that wrapping to 75 chars? It's *special* line, i.e. tag. Tag per line is a rule I know, did I miss any new change to that? >>> + byte_num = (pcs->bits_per_pin * pin) / >>> BITS_PER_BYTE; >>> + offset = (byte_num / mux_bytes) * mux_bytes; >>> + pin_shift = pin % (pcs->width / >>> pcs->bits_per_pin) * >>> + pcs->bits_per_pin; >> Sounds like playing around pretty well defined macro and functions, >> e.g. DIV_ROUND_UP(), round_up(). > I admit, I just copied existing code (which may be a reason to leave this > the > way it is). Ah, fair enough. > But, I only see once place to do this: > > offset = round_down(byte_num, mux_bytes); > > Did I miss another? I meant that you may use macros to make code cleaner. Though, taking above into consideration, it would be done as a separate patch later.
* Andy Shevchenko <andy.shevchenko@gmail.com> [180123 09:25]: > On Mon, Jan 22, 2018 at 7:06 PM, David Lechner <david@lechnology.com> wrote: > > On 01/22/2018 08:49 AM, Andy Shevchenko wrote: > >> On Mon, Jan 22, 2018 at 1:03 AM, David Lechner <david@lechnology.com> > >> wrote: > > >>> Fixes: 4e7e8017a80e ("pinctrl: pinctrl-single: enhance to configure > >>> multiple pins of different modules") > > >> One line? > > > One line is more important that wrapping to 75 chars? > > It's *special* line, i.e. tag. Tag per line is a rule I know, did I > miss any new change to that? I think the rule for merge commits is to try to limit them to 75 characters for git log. > >>> + byte_num = (pcs->bits_per_pin * pin) / > >>> BITS_PER_BYTE; > >>> + offset = (byte_num / mux_bytes) * mux_bytes; > >>> + pin_shift = pin % (pcs->width / > >>> pcs->bits_per_pin) * > >>> + pcs->bits_per_pin; > > >> Sounds like playing around pretty well defined macro and functions, > >> e.g. DIV_ROUND_UP(), round_up(). > > > I admit, I just copied existing code (which may be a reason to leave this > > the > > way it is). > > Ah, fair enough. > > > But, I only see once place to do this: > > > > offset = round_down(byte_num, mux_bytes); > > > > Did I miss another? > > I meant that you may use macros to make code cleaner. Though, taking > above into consideration, it would be done as a separate patch later. Yeah makes sense to me to do them separately. It would be good to get Haojian's ack or tested-by on this one, I don't think I have hardware where I can test the GPIO features with this driver. Regards, Tony
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 3501491..7a5e2f1 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -391,9 +391,25 @@ 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 byte_num, offset, pin_shift; + + byte_num = (pcs->bits_per_pin * pin) / BITS_PER_BYTE; + offset = (byte_num / mux_bytes) * mux_bytes; + pin_shift = pin % (pcs->width / pcs->bits_per_pin) * + pcs->bits_per_pin; + + data = pcs->read(pcs->base + offset); + data &= ~(pcs->fmask << pin_shift); + data |= frange->gpiofunc << pin_shift; + pcs->write(data, pcs->base + offset); + } else { + data = pcs->read(pcs->base + pin * mux_bytes); + data &= ~pcs->fmask; + data |= frange->gpiofunc; + pcs->write(data, pcs->base + pin * mux_bytes); + } break; } return 0;
This fixes pcs_request_gpio() in the pinctrl-single driver when bits_per_mux != 0. It appears this was overlooked when the multiple pins per register feature was added. Fixes: 4e7e8017a80e ("pinctrl: pinctrl-single: enhance to configure multiple pins of different modules") Cc: Tony Lindgren <tony@atomide.com> Cc: Haojian Zhuang <haojian.zhuang@linaro.org> Cc: Linus Walleij <linus.walleij@linaro.org> Signed-off-by: David Lechner <david@lechnology.com> --- drivers/pinctrl/pinctrl-single.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)