Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   andy.shevchenko@gmail.com
Date:   Sat, 27 May 2023 01:14:50 +0300
To:     Mukesh Ojha <quic_mojha@quicinc.com>
Cc:     agross@kernel.org, andersson@kernel.org, konrad.dybcio@linaro.org,
        linus.walleij@linaro.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
Subject: Re: [PATCH v6 5/5] firmware: qcom_scm: Add multiple download mode
 support
Message-ID: <ZHEvWpCqg_oyWyZW@surfacebook>
References: <1680076012-10785-1-git-send-email-quic_mojha@quicinc.com>
 <1680076012-10785-6-git-send-email-quic_mojha@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1680076012-10785-6-git-send-email-quic_mojha@quicinc.com>
Precedence: bulk

Wed, Mar 29, 2023 at 01:16:52PM +0530, Mukesh Ojha kirjoitti:
> Currently, scm driver only supports full dump when download
> mode is selected. Add support to enable minidump as well as
> enable it along with fulldump.

...

>  #define QCOM_DOWNLOAD_MODE_MASK 0x30
>  #define QCOM_DOWNLOAD_FULLDUMP	0x1
> +#define QCOM_DOWNLOAD_MINIDUMP  0x2
> +#define QCOM_DOWNLOAD_BOTHDUMP	(QCOM_DOWNLOAD_FULLDUMP | QCOM_DOWNLOAD_MINIDUMP)

Now order is broken.

>  #define QCOM_DOWNLOAD_NODUMP	0x0

...

> @@ -1420,13 +1422,16 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -

Stray change and ping-pong style at the same time.

...

>  	if (download_mode == QCOM_DOWNLOAD_FULLDUMP)
>  		len = sysfs_emit(buffer, "full\n");
> +	else if (download_mode == QCOM_DOWNLOAD_MINIDUMP)
> +		len = sysfs_emit(buffer, "mini\n");
> +	else if (download_mode == QCOM_DOWNLOAD_BOTHDUMP)

> +		len = sysfs_emit(buffer, "full,mini\n");

Why not "both" ?

>  	else if (download_mode == QCOM_DOWNLOAD_NODUMP)
>  		len = sysfs_emit(buffer, "off\n");


With an array (for streq_match_string() call suggested earlier) this become as
simple as

	if (mode >= ARRAY_SIZE(...))
		return sysfs_emit("Oh heh!\n");

	return sysfs_emit("%s\n", array[mode]);

...

> -	if (sysfs_streq(val, "full")) {

Why changing this line?

> +	if (sysfs_streq(val, "full,mini") || sysfs_streq(val, "mini,full")) {
> +		download_mode = QCOM_DOWNLOAD_BOTHDUMP;

It's way too hard, esp. taking into account that once user enters wrong order,
user can't simply validate this by reading value back.

Use "both" and that's it.

> +	} else if (sysfs_streq(val, "full")) {
>  		download_mode = QCOM_DOWNLOAD_FULLDUMP;
> +	} else if (sysfs_streq(val, "mini")) {
> +		download_mode = QCOM_DOWNLOAD_MINIDUMP;

...

>  module_param_cb(download_mode, &download_mode_param_ops, NULL, 0644);
>  MODULE_PARM_DESC(download_mode,
> -		 "Download mode: off/full or 0/1 for existing users");
> +		"download mode: off/full/mini/full,mini or mini,full and 0/1 for existing users");

You really must be consistent with at least a couple of things:
1) capitalization;
2) indentation.

-- 
With Best Regards,
Andy Shevchenko