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

[James Hogan]

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

Attachment: signature.asc
Description: Digital signature