Re: [PATCH v3 3/3] pinctrl: pinctrl-single: fix pcs_pin_dbg_show() when bits_per_mux is not zero

[Hawa, Hanna]

On 3/18/2021 2:15 PM, Andy Shevchenko wrote:
> On Wed, Mar 17, 2021 at 11:42 PM Hanna Hawa<hhhawa@xxxxxxxxxx>  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@xxxxxxxxx>

Thanks

> > Fixes: 4e7e8017a80e ("pinctrl: pinctrl-single: enhance to configure multiple pins of different modules")
> > Signed-off-by: Hanna Hawa<hhhawa@xxxxxxxxxx>
> > ---
> >   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