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

From: "Hawa, Hanna" <hhhawa@amazon.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>,
	Haojian Zhuang <haojian.zhuang@linaro.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	"David Woodhouse" <dwmw@amazon.co.uk>, <benh@amazon.com>,
	<ronenk@amazon.com>, <talel@amazon.com>, <jonnyc@amazon.com>,
	<hanochu@amazon.com>, <tgershi@amazon.com>,
	linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
	Linux OMAP Mailing List <linux-omap@vger.kernel.org>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 3/3] pinctrl: pinctrl-single: fix pcs_pin_dbg_show() when bits_per_mux is not zero
Date: Fri, 19 Mar 2021 09:52:55 +0200	[thread overview]
Message-ID: <cd589749-7f37-9f7f-9d36-42032c724506@amazon.com> (raw)
In-Reply-To: <CAHp75VdYeVOHu5T37EBEjL5xfgjevzb-ErZb2QMy7defXDS5fg@mail.gmail.com>

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