Messages in this thread | | | Subject | Re: [PATCH v3 3/3] pinctrl: pinctrl-single: fix pcs_pin_dbg_show() when bits_per_mux is not zero | From | "Hawa, Hanna" <> | Date | Fri, 19 Mar 2021 09:52:55 +0200 |
| |
On 3/18/2021 2:15 PM, Andy Shevchenko wrote: > > > On Wed, Mar 17, 2021 at 11:42 PM Hanna Hawa<hhhawa@amazon.com> wrote: >> An SError was detected when trying to print the supported pins in a > What is SError? Yes, I have read a discussion, but here is the hint: > if a person sees this as a first text due to, for example, bisecting > an issue, what she/he can get from this cryptic name?
What you suggest? s/An SError/A kernel-panic/? Or remove the sentence and keep the below: " This change fixes the pcs_pin_dbg_show() in pinctrl-single driver when bits_per_mux is not zero. In addition move offset calculation and pin offset in register to common function. "
> >> pinctrl device which supports multiple pins per register. This change >> fixes the pcs_pin_dbg_show() in pinctrl-single driver when bits_per_mux >> is not zero. In addition move offset calculation and pin offset in >> register to common function. > Reviewed-by: Andy Shevchenko<andy.shevchenko@gmail.com>
Thanks
> >> Fixes: 4e7e8017a80e ("pinctrl: pinctrl-single: enhance to configure multiple pins of different modules") >> Signed-off-by: Hanna Hawa<hhhawa@amazon.com> >> --- >> drivers/pinctrl/pinctrl-single.c | 57 +++++++++++++++++++++----------- >> 1 file changed, 37 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c >> index f3394517cb2e..4595acf6545e 100644 >> --- a/drivers/pinctrl/pinctrl-single.c >> +++ b/drivers/pinctrl/pinctrl-single.c >> @@ -270,20 +270,46 @@ static void __maybe_unused pcs_writel(unsigned val, void __iomem *reg) >> writel(val, reg); >> } >> >> +static unsigned int pcs_pin_reg_offset_get(struct pcs_device *pcs, >> + unsigned int pin) >> +{ >> + unsigned int mux_bytes; >> + >> + mux_bytes = pcs->width / BITS_PER_BYTE; > Can be folded to one line.
Ack
> >> + if (pcs->bits_per_mux) { >> + unsigned int pin_offset_bytes; >> + >> + pin_offset_bytes = (pcs->bits_per_pin * pin) / BITS_PER_BYTE; >> + return (pin_offset_bytes / mux_bytes) * mux_bytes; > Side note for the further improvements (in a separate change, because > I see that you just copied an original code, and after all this is > just a fix patch): this can be replaced by round down APIs (one which > works for arbitrary divisors).
Agree, didn't want to change the formula as it's fix patch. (here and below), this can be taken for further improvements.
> >> + } >> + >> + return pin * mux_bytes; >> +} >> + >> +static unsigned int pcs_pin_shift_reg_get(struct pcs_device *pcs, >> + unsigned int pin) >> +{ >> + return (pin % (pcs->width / pcs->bits_per_pin)) * pcs->bits_per_pin; > Also a side note: I'm wondering if this can be optimized to have less divisions. > >> +} >> + >> static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev, >> struct seq_file *s, >> unsigned pin) >> {
Thanks, Hanna
| |