linux-kernel - Re: [PATCH v6 2/4] MIPS: Loongson64: Yeeloong add platform driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180124115833.GC5446@saruman>
Date:   Wed, 24 Jan 2018 11:58:34 +0000
From:   James Hogan <jhogan@...nel.org>
To:     Jiaxun Yang <jiaxun.yang@...goat.com>
Cc:     Ralf Baechle <ralf@...ux-mips.org>,
        Huacai CHen <chenhc@...ote.com>, linux-mips@...ux-mips.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 2/4] MIPS: Loongson64: Yeeloong add platform driver

On Tue, Dec 26, 2017 at 11:26:00AM +0800, Jiaxun Yang wrote:
> diff --git a/drivers/platform/mips/yeeloong_laptop.c b/drivers/platform/mips/yeeloong_laptop.c
> new file mode 100755

Checkpatch complains about having the execute permission set on this
file.

> +static int __init yeeloong_init(void)
> +{
> +	int ret;
> +
> +	if (mips_machtype != MACH_LEMOTE_YL2F89) {
> +		pr_err("Unsupported system.\n");
> +		return -ENODEV;
> +	}
> +
> +	pr_info("Load YeeLoong Laptop Platform Specific Driver.\n");
> +
> +	/* Register platform stuff */
> +	ret = platform_driver_register(&platform_driver);
> +	if (ret) {
> +		pr_err("Fail to register yeeloong platform driver.\n");
> +		return ret;
> +	}
> +
> +	ret = yeeloong_backlight_init();
> +	if (ret) {
> +		pr_err("Fail to register yeeloong backlight driver.\n");
> +		yeeloong_backlight_exit();
> +		return ret;
> +	}
> +
> +	ret = yeeloong_bat_init();
> +	if (ret) {
> +		pr_err("Fail to register yeeloong battery driver.\n");
> +		yeeloong_bat_exit();
> +		return ret;
> +	}
> +
> +	ret = yeeloong_hwmon_init();
> +	if (ret) {
> +		pr_err("Fail to register yeeloong hwmon driver.\n");
> +		yeeloong_hwmon_exit();
> +		return ret;
> +	}
> +
> +	ret = yeeloong_hotkey_init();
> +	if (ret) {
> +		pr_err("Fail to register yeeloong hotkey driver.\n");
> +		yeeloong_hotkey_exit();
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit yeeloong_exit(void)
> +{
> +	yeeloong_hotkey_exit();
> +	yeeloong_hwmon_exit();
> +	yeeloong_bat_exit();
> +	yeeloong_backlight_exit();

I can't help thinking it would be better to separate this into separate
drivers for each part (backlight, power supply etc), and move them into
the appropriate driver directories (drivers/power/supply,
drivers/video/backlight etc). That way each part would get proper review
from the appropriate maintainers (or at least they should be Cc'd).

Is there a particular reason for it to be a single driver?

Cheers
James

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)