Date:   Sun, 12 Mar 2023 18:01:03 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Khadija Kamran <kamrankhadijadj@gmail.com>,
        <outreachy@lists.linux.dev>
CC:     Vaibhav Hiremath <hvaibhav.linux@gmail.com>,
        Johan Hovold <johan@kernel.org>, Alex Elder <elder@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        <greybus-dev@lists.linaro.org>, <linux-staging@lists.linux.dev>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] staging: greybus: remove tabs to fix line length and
 merge lines
Message-ID: <640e75cfd8fc_229a89294a3@iweiny-mobl.notmuch>
References: <ZAusnKYVTGvO5zoi@khadija-virtual-machine>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZAusnKYVTGvO5zoi@khadija-virtual-machine>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WE82aXlBeVQ3ODlpNTNRcExHN2ordlFVUStxSHJialhTS05Qc25QZnVXYXR1?=
 =?utf-8?B?UUdJYXJWamQ1SzhsZjlVYUlzQ1JFWWtiRUUvMnFyZ3BIVm1PY3gweHZLNGor?=
 =?utf-8?B?RTNzU3lIODhSMXdZSDRma0x1YXRQZWJCZ2JMVzZxM25INUw3SEdvYitadGN4?=
 =?utf-8?B?YTNPVFJ2enJMSU1Ia3hZRUJuV2Jkb0lZSldLT2NqR3FRSjFqVVF0dGpjeGdi?=
 =?utf-8?B?aThUNjBhRExyenNBVHFRYXVJb3pRMFpNWnZML0RhN1N4dFBXQTRZYXc0Z0s3?=
 =?utf-8?B?UFgrWW51bDM5RmVHY2RBK1VEMHNaVE1WMWZiS2dES3NiV3lwbGg1d2czUlZa?=
 =?utf-8?B?NmEwcmVyQkNTS2YvdDJ6dHMwRll2YUYrdGhoL1dVZ2pUUTd6Y1FFZmJrQ2Nu?=
 =?utf-8?B?aEM5NmUzVG5OWitmamM4WXRYQVFrVkhJU2hrVVZOeExudTkyVFdjdU56YTRs?=
 =?utf-8?B?RTJVSGJZMzM4Z1pxTnl1N0MzRmlBVUZvNVFIZjY3Uy8xWTU1Nk1SL2czeWN4?=
 =?utf-8?B?TTN3L0dVbjh4dEdRMGN2WDFocEdYTXlrelBhQVhERVhHZ0FGcVQ3Ykh2aTkx?=
 =?utf-8?B?aHFjcVdxcnkwaTZhNGNUT0NreHlNMzdzMklzMXlmN05oZEtGMjVNR29iOHBo?=
 =?utf-8?B?MFJDSE1FVXZyb0xSNlBUYW9CclhZQjNsSWVRUnNoSTdKV1NPOGlSWHQ3UTRu?=
 =?utf-8?B?dlFYaUowajBKTjdMV2VvSkVPZ0dDNGMva0RhRXNIaVJoRE1aVDNsWE44K1hJ?=
 =?utf-8?B?NEpLczhSL2g4b25QbWpRc3BqMGJYL2FyZzlERGdpQjNVeC9pL3lhakRrTFBP?=
 =?utf-8?B?L2pFYlZaY0t0azhxRUVjZGxDR2FVK2kzMFp2U2xHWXIvWGoxRFZTcThuK2R4?=
 =?utf-8?B?NW12akJKTHJXcmMrNWsrcUJJTXlNMHFCb1ltWS93OEZsc1RqWlM2cWo5S3ZC?=
 =?utf-8?B?VFdUbUFRUE8wOW9yVTA1SUJFMC9qbDZaNXR3SGxyUStvV2s1N1VnMWlLUUs1?=
 =?utf-8?B?RkVKWTcwVGpjb3VjaTY5TVZxNU9PM2s4Sk92U0V4TnJvektaZ3dKVkhKRDMv?=
 =?utf-8?B?Nk0wZmM4NHphK0lwZmQ1Z29wMXo0dVVySFJ4b2trSDl2Sm9VRUJ4L0c2OFIy?=
 =?utf-8?B?dVlYRmZ0a1RmWmpxWmxnNTVXL1pqQndMU1ZpVElRdXA5MGh2Qkd4bmEvRW0v?=
 =?utf-8?B?eGQ5eXYvcGROVGNESjZtSTdZUWFReFhyN1NYcytiZ042cnA1aTRnbnhlVHN2?=
 =?utf-8?B?MUxlUkhxcVVNNHU3YTFxSHJHSkVSQjhON0EyWDF2V0FvSVVzTDQzRlQ4YzhQ?=
 =?utf-8?B?ZEJZTmtPRUVFMzVJVnFRd0x2cUdJbTg0SzZsSFgyZDJKSUczWEY3M284Z0do?=
 =?utf-8?B?dkVJM2ZvSlZYWUtzN2txTDJyUzhURERsc1QxTlFUam9aMHdMMXpkdWdLVzFU?=
 =?utf-8?B?V0gxSVRReXRVdGROVjBZYk4zYVYwUWtnSlJySWdEM2RMUGo0RHVMQlBKcnFu?=
 =?utf-8?B?enA1SjVkVDJ3M1h6cVBianZJbmFJY3g1bmloU3hucTJpTzBwZEYrQUhkbWxE?=
 =?utf-8?B?NXJDR3h4RktHQXIyb2VCRHZmbDlQckNwVlptcjZtNkhCbzFwbnhtbm5vMUNK?=
 =?utf-8?B?RE1UU2g3RnNPWGFKSnFXaEpKWW5pQ3Q2dkU2S2tGMWhXUWVyYXlwSG9lbUhJ?=
 =?utf-8?B?MFVvQVRwaVRJajJtSmZiRW02WFA0aU9iRDBxZWJkZDdWL2h6UzBGTWZrby9F?=
 =?utf-8?B?WlBqQS9xL2pObGRQSVBPSWlCVXVaOU1VUmtHYTlkYUxUTVNIditVZTcvZ0hl?=
 =?utf-8?B?K29EejlDV0dCeTlwOXNodEN4V3ZNWVpVSnNIMVhEejJmMkJCbXUwVU93ZThN?=
 =?utf-8?B?bUJEZWE0M01YclpxUGtsU3NHU3BKOWcvc3k0enVIR203YW9HVXV3QTdxVjhs?=
 =?utf-8?B?dHFTbFRybm95TlEzSTN5SGgzY2tOMStJM0VsOTRGcVV4VnVzK1VaTEZvNFRr?=
 =?utf-8?B?bGFzenFFSlNScm4zRVlMLzE4K25YU3V3YmwrbGlaNUhLZDRsRjF3dENHbzZZ?=
 =?utf-8?B?MXh4NnlWTVlEY2NEeEdWK1ZJTXEyYWhVT29pR3RuY0cxNlJ0ZG5BN2N3R3dF?=
 =?utf-8?Q?3+DACBNe1k2Pj2ltBe18iSQmY?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b83f0385-de1f-4e83-1253-08db235e6cce
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Mar 2023 01:01:06.4442
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: xhifhUhpqJ01djh3+A+DCj1J6LiMWlT0oG8vRW9hw43X/QMy61lgCajCQ78kwObH6JebMYetcnq4X75KO4oHpg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4716
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Khadija Kamran wrote:
> In file drivers/staging/greybus/arche-platform.c,
> - Length of line 181 exceeds 100 columns, fix by removing tabs from the
>   line.
> - If condition and spin_unlock_...() call is split into two lines, join
> them to form a single line.
> 
> Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>

Fundamentally the problem with arche_platform_wd_irq() is that the
indentation is too great.

"... if you need more than 3 levels of indentation, you’re screwed anyway,
and should fix your program."

	-- https://www.kernel.org/doc/html/v4.10/process/coding-style.html#indentation

I think a better solution would be to refactor the entire function.  This
would make the logic of the function more clear as well IMHO.

Here is another tip to help:

https://www.kernel.org/doc/html/v4.10/process/coding-style.html#centralized-exiting-of-functions

Do you think you could try that?
Ira

> ---
> Changes in v2:
>  - Change the subject and log message
>  - Merge if condition and spin_unlock...() from two lines to one 
>  drivers/staging/greybus/arche-platform.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index fcbd5f71eff2..00ed5dfd7915 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -176,12 +176,10 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>  				 * Check we are not in middle of irq thread
>  				 * already
>  				 */
> -				if (arche_pdata->wake_detect_state !=
> -						WD_STATE_COLDBOOT_START) {
> +				if (arche_pdata->wake_detect_state != WD_STATE_COLDBOOT_START) {
>  					arche_platform_set_wake_detect_state(arche_pdata,
> -									     WD_STATE_COLDBOOT_TRIG);
> -					spin_unlock_irqrestore(&arche_pdata->wake_lock,
> -							       flags);
> +						WD_STATE_COLDBOOT_TRIG);
> +					spin_unlock_irqrestore(&arche_pdata->wake_lock, flags);
>  					return IRQ_WAKE_THREAD;
>  				}
>  			}
> -- 
> 2.34.1
> 
>