Re: [PATCH v3 3/3] pinctrl: pinctrl-single: fix pcs_pin_dbg_show() when bits_per_mux is not zero
On Fri, Mar 19, 2021 at 9:53 AM Hawa, Hanna wrote: > On 3/18/2021 2:15 PM, Andy Shevchenko wrote: > > On Wed, Mar 17, 2021 at 11:42 PM Hanna Hawa 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/? Not below, but something which makes clear what SError is. Like "A System Error (SError, followed by 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. > " -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 3/3] pinctrl: pinctrl-single: fix pcs_pin_dbg_show() when bits_per_mux is not zero
On 3/18/2021 2:15 PM, Andy Shevchenko wrote:
On Wed, Mar 17, 2021 at 11:42 PM Hanna Hawa 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
Thanks
Fixes: 4e7e8017a80e ("pinctrl: pinctrl-single: enhance to configure multiple pins of
different modules")
Signed-off-by: Hanna Hawa
---
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
Re: [PATCH v3 3/3] pinctrl: pinctrl-single: fix pcs_pin_dbg_show() when bits_per_mux is not zero
On Wed, Mar 17, 2021 at 11:42 PM Hanna Hawa 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?
> 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
> Fixes: 4e7e8017a80e ("pinctrl: pinctrl-single: enhance to configure multiple
> pins of different modules")
> Signed-off-by: Hanna Hawa
> ---
> 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.
> + 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).
> + }
> +
> + 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)
> {
> struct pcs_device *pcs;
> - unsigned val, mux_bytes;
> + unsigned int val;
> unsigned long offset;
> size_t pa;
>
> pcs = pinctrl_dev_get_drvdata(pctldev);
>
> - mux_bytes = pcs->width / BITS_PER_BYTE;
> - offset = pin * mux_bytes;
> + offset = pcs_pin_reg_offset_get(pcs, pin);
> val = pcs->read(pcs->base + offset);
> +
> + if (pcs->bits_per_mux)
> + val &= pcs->fmask << pcs_pin_shift_reg_get(pcs, pin);
> +
> pa = pcs->res->start + offset;
>
> seq_printf(s, "%zx %08x %s ", pa, val, DRIVER_NAME);
> @@ -384,7 +410,6 @@ 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;
> unsigned data;
>
> /* If function mask is null, return directly. */
> @@ -392,29 +417,27 @@ static int pcs_request_gpio(struct pinctrl_dev *pctldev,
> return -ENOTSUPP;
>
> list_for_each_safe(pos, tmp, >gpiofuncs) {
> + u32 offset;
> +
> frange = list_entry(pos, struct pcs_gpiofunc_range, node);
> if (pin >= frange->offset + frange->npins
> || pin < frange->offset)
> continue;
> - mux_bytes = pcs->width / BITS_PER_BYTE;
>
> - if (pcs->bits_per_mux) {
> - int byte_num, offset, pin_shift;
> + offset = pcs_pin_reg_offset_get(pcs, pin);
>
> - 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;
> + if (pcs->bits_per_mux) {
> + int pin_shift = pcs_pin_shift_reg_get(pcs, 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->read(pcs->base + offset);
> data &= ~pcs->fmask;
>
[PATCH v3 3/3] pinctrl: pinctrl-single: fix pcs_pin_dbg_show() when bits_per_mux is not zero
An SError was detected when trying to print the supported pins in a
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.
Fixes: 4e7e8017a80e ("pinctrl: pinctrl-single: enhance to configure multiple
pins of different modules")
Signed-off-by: Hanna Hawa
---
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;
+
+ 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;
+ }
+
+ 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;
+}
+
static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
struct seq_file *s,
unsigned pin)
{
struct pcs_device *pcs;
- unsigned val, mux_bytes;
+ unsigned int val;
unsigned long offset;
size_t pa;
pcs = pinctrl_dev_get_drvdata(pctldev);
- mux_bytes = pcs->width / BITS_PER_BYTE;
- offset = pin * mux_bytes;
+ offset = pcs_pin_reg_offset_get(pcs, pin);
val = pcs->read(pcs->base + offset);
+
+ if (pcs->bits_per_mux)
+ val &= pcs->fmask << pcs_pin_shift_reg_get(pcs, pin);
+
pa = pcs->res->start + offset;
seq_printf(s, "%zx %08x %s ", pa, val, DRIVER_NAME);
@@ -384,7 +410,6 @@ 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;
unsigned data;
/* If function mask is null, return directly. */
@@ -392,29 +417,27 @@ static int pcs_request_gpio(struct pinctrl_dev *pctldev,
return -ENOTSUPP;
list_for_each_safe(pos, tmp, >gpiofuncs) {
+ u32 offset;
+
frange = list_entry(pos, struct pcs_gpiofunc_range, node);
if (pin >= frange->offset + frange->npins
|| pin < frange->offset)
continue;
- mux_bytes = pcs->width / BITS_PER_BYTE;
- if (pcs->bits_per_mux) {
- int byte_num, offset, pin_shift;
+ offset = pcs_pin_reg_offset_get(pcs, pin);
- 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;
+ if (pcs->bits_per_mux) {
+ int pin_shift = pcs_pin_shift_reg_get(pcs, 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->read(pcs->base + offset);
data &= ~pcs->fmask;
data |= frange->gpiofunc;
- pcs->write(data, pcs->base + pin * mux_bytes);
+ pcs->write(data, pcs->base + offset);
}
break;
}
@@ -724,14 +747,8 @@ static int pcs_allocate_pin_table(struct pcs_device *pcs)
for (i = 0; i < pcs->desc.npins; i++) {
unsigned offset;
int res;
- int byte_num;
- if (pcs->bits_per_mux) {
- byte_num = (pcs->bits_per_pin * i) / BITS_PER_BYTE;
- offset = (byte_num / mux_bytes) * mux_bytes;
- } else {
- offset = i * mux_bytes;
- }
+ offset = pcs_pin_reg_offset_get(pcs, i);
res = pcs_add_pin(pcs, offset);
