Re: [Intel-gfx] [PATCH] drm/i915: Move hotplug inversion logic into separate helper

[Gustavo Sousa <gustavo.sousa@intel.com>]

On Thu, Sep 22, 2022 at 11:17:01AM +0300, Jani Nikula wrote:
> On Wed, 21 Sep 2022, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> > On Wed, Sep 21, 2022 at 11:21:00AM +0300, Jani Nikula wrote:
> >> On Tue, 20 Sep 2022, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> >> > Hi, Jani.
> >> >
> >> > On Tue, Sep 20, 2022 at 10:19:53AM +0300, Jani Nikula wrote:
> >> >> On Mon, 19 Sep 2022, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> >> >> > Make the code more readable, which will be more apparent as new
> >> >> > platforms with different hotplug inversion needs are added.
> >> >> >
> >> >> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> >> >> > ---
> >> >> >  drivers/gpu/drm/i915/i915_irq.c | 25 ++++++++++++++++---------
> >> >> >  1 file changed, 16 insertions(+), 9 deletions(-)
> >> >> >
> >> >> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> >> > index de06f293e173..c53d21ae197f 100644
> >> >> > --- a/drivers/gpu/drm/i915/i915_irq.c
> >> >> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> >> > @@ -3263,6 +3263,21 @@ static void cherryview_irq_reset(struct drm_i915_private *dev_priv)
> >> >> >  	spin_unlock_irq(&dev_priv->irq_lock);
> >> >> >  }
> >> >> >  
> >> >> > +static void setup_hotplug_inversion(struct drm_i915_private *dev_priv)
> >> >> > +{
> >> >> > +	u32 invert_bits;
> >> >> > +
> >> >> > +	if (HAS_PCH_DG1(dev_priv))
> >> >> > +		invert_bits = INVERT_DDIA_HPD |
> >> >> > +			      INVERT_DDIB_HPD |
> >> >> > +			      INVERT_DDIC_HPD |
> >> >> > +			      INVERT_DDID_HPD;

Just checked the source and it seems I'm the only oddball aligning stuff like
this :-)

> >> >> 
> >> >> Nitpick, the indentation will be off compared to automated indentation.
> >> >
> >> > What do you mean by automated indentation?
> >> 
> >> For example, hit TAB on the lines using a smart enough editor, which has
> >> been configured to follow kernel coding style. ;)
> >> 
> >
> > I'm not sure I completely understand the issue. Could you provide an example of
> > such a configuration?
> 
> I never indent anything manually. If I hit TAB in emacs (or run M-x
> indent-region), it'll indent the current line (or region) using the
> built-in "linux" style [1].
> 
> [1] https://www.gnu.org/software/emacs/manual/html_node/ccmode/Built_002din-Styles.html
> 
> > Is the problem here the spaces after the tabs to align each INVERT_DDI*? Would
> > you suggest I just use tabs and do not align them to the first one?
> 
> If a parenthesized element in round brackets is split to multiple lines,
> the indent is usually tabs first and then spaces to align everything
> after the open parenthesis. If there are no parenthesis, the indent is
> just tabs.
> 
> Guess why the original dg1_hpd_irq_setup() has round brackets in the
> statement. ;) Though not everyone likes superfluous parenthesis for
> style.
> 
> > E.g.:
> >
> >                 invert_bits = INVERT_DDIA_HPD |
> >                                 INVERT_DDIB_HPD |
> >                                 INVERT_DDIC_HPD |
> >                                 INVERT_DDID_HPD;
> >
> > Another one:
> >
> >                 invert_bits = INVERT_DDIA_HPD |
> >                         INVERT_DDIB_HPD |
> >                         INVERT_DDIC_HPD |
> >                         INVERT_DDID_HPD;
> 
> Your patch applied, re-indented, and tabs converted to spaces for visual
> here:
> 
>         if (HAS_PCH_DG1(dev_priv))
>                 invert_bits = INVERT_DDIA_HPD |
>                         INVERT_DDIB_HPD |
>                         INVERT_DDIC_HPD |
>                         INVERT_DDID_HPD;
> 
> Anyway, indentation isn't a very fruitful conversation in itself. The
> original nitpick was an off the cuff remark. I'm mostly trying to
> suggest to use tools that will do indentation and other things for you,
> so you don't have to worry about getting it right.
> 

Thank you for the tips! I find them very helpful. I'll make sure to use proper
style when sending a new version.

--
Gustavo Sousa