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

[Jani Nikula]

On Wed, 21 Sep 2022, Gustavo Sousa <gustavo.sousa at 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 at 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 at 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 at 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;
>> >> 
>> >> 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.


BR,
Jani.

>
>
> Note: I'm using 8 spaces for instead tabs in the above examples for proper
> visual, but they would be tab characters in the source.
>
> --
> Gustavo Sousa

-- 
Jani Nikula, Intel Open Source Graphics Center