RE: [PATCH v2 4/4] pinctrl/lantiq: fix up pinmux

[Martin Schiller <mschiller@tdt.de>]

On 11/26/2015 at 8:20 AM, John Crispin wrote:
>
>
> On 26/11/2015 08:15, Martin Schiller wrote:
> > On 11/26/2015 at 8:04 AM, John Crispin wrote:
> >>
> >>
> >> On 26/11/2015 07:40, Martin Schiller wrote:
> >>> On 11/25/2015 at 11:40 AM, Jonas Gorski wrote:
> >>>> Hi
> >>>>
> >>>> On Wed, Nov 25, 2015 at 11:18 AM, Martin Schiller
> <mschiller@tdt.de>
> >>>> wrote:
> >>>>> From: John Crispin <blogic@openwrt.org>
> >>>>>
> >>>>> This patch is included in the openwrt patchset for several years
> >> now
> >>>> and needs
> >>>>> to go upstream as well. It includes the following changes:
> >>>>> 1. Fix up inline function call to xway_mux_apply
> >>>>
> >>>> This really needs an explanation what is being fixed here.
> >>>
> >>> I hope John - as the original author of this patch - can explain
> >>> why this change is necessary.
> >>
> >> what change? why am I in Cc: and not To: if an action is required ?
> >>
> >> John
> >
> > That change is meant:
> >
> #######################################################################
> #
> >> diff --git a/drivers/pinctrl/pinctrl-xway.c
> b/drivers/pinctrl/pinctrl-
> >> xway.c
> >> index a064962..f0b1b48 100644
> >> --- a/drivers/pinctrl/pinctrl-xway.c
> >> +++ b/drivers/pinctrl/pinctrl-xway.c
> >> @@ -1496,10 +1496,9 @@ static struct pinctrl_desc xway_pctrl_desc =
> {
> >>  .confops= &xway_pinconf_ops,
> >>  };
> >>
> >> -static inline int xway_mux_apply(struct pinctrl_dev *pctrldev,
> >> +static int mux_apply(struct ltq_pinmux_info *info,
> >>  int pin, int mux)
> >>  {
> >> -struct ltq_pinmux_info *info = pinctrl_dev_get_drvdata(pctrldev);
> >>  int port = PORT(pin);
> >>  u32 alt1_reg = GPIO_ALT1(pin);
> >>
> >> @@ -1519,6 +1518,14 @@ static inline int xway_mux_apply(struct
> >> pinctrl_dev *pctrldev,
> >>  return 0;
> >>  }
> >>
> >> +static inline int xway_mux_apply(struct pinctrl_dev *pctrldev,
> >> +int pin, int mux)
> >> +{
> >> +struct ltq_pinmux_info *info = pinctrl_dev_get_drvdata(pctrldev);
> >> +
> >> +return mux_apply(info, pin, mux);
> >> +}
> >> +
> >>  static const struct ltq_cfg_param xway_cfg_params[] = {
> >>  {"lantiq,pull",LTQ_PINCONF_PARAM_PULL},
> >>  {"lantiq,open-drain",LTQ_PINCONF_PARAM_OPEN_DRAIN},
> >
> #######################################################################
> >
>
> ok so you picked up a patch and sent it upstream without looking at
> what
> it really does. the patch is simply not ready for upstream. the problem
> here is copy & paste inconsistency.

Of course I analyzed the whole patch and - as you may have noticed - added
a description for the 3 changes which were made in this patch.

I also know that this first part of the patch changes the scope of the inline
code, but I did not know why this change was done.

>
> however if we want to resolve this we should either keep the inlines
> and
> just stick to the current code pattern used or we could just assume
> that
> the compiler will be smart enough to to know when to inline and remove
> all of them.
>
> i'll leave it up to you to decide and propose your solution as a patch
> with an explanation.
>
> John

So I think it would be the best for now to leave this code as it is and make
two separate patches from the remaining two changes:

- Fix GPIO Setup of GPIO Port3
- Implement gpio_chip.to_irq

Martin

>
>
> >>
> >>>
> >>>>
> >>>>> 2. Fix GPIO Setup of GPIO Port3
> >>>>
> >>>> This change looks fine.
> >>>>
> >>>>> 3. Implement gpio_chip.to_irq
> >>>>
> >>>> These are three different changes (two fixes, one new feature) and
> >>>> therefore should be split up into three patches.
> >>>
> >>> As I'm not the author of this patch, I decided to leave it as it
> is.
> >>> But per se you are right, it would be better to split it up.
> >>>
> >>>>
> >>>>> Signed-off-by: John Crispin <blogic@openwrt.org>
> >>>>> Signed-off-by: Martin Schiller <mschiller@tdt.de>
> >>>>> ---
> >>>>
> >>>> Also please provide a changelog for your patches here.
> >>>
> >>> OK.
> >>>
> >>>>
> >>>>>  drivers/pinctrl/pinctrl-xway.c | 28 ++++++++++++++++++++++++++--
> >>>>>  1 file changed, 26 insertions(+), 2 deletions(-)
> >>>>>
> >>>>
> >>>>
> >>>> Jonas
> >>>
> >>> Martin
> >>>
> >>>
> >
> >