linux-kernel - Re: [PATCH v3] hwmon: (asus-ec-sensors) merge setup functions

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]

Date:   Wed, 16 Feb 2022 08:54:22 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Eugene Shalygin <eugene.shalygin@...il.com>
Cc:     Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] hwmon: (asus-ec-sensors) merge setup functions

On 2/16/22 08:01, Eugene Shalygin wrote:
> Merge configure_sensor_setup() into probe().
> 
> Changes:
>   - v2: add local struct device *dev = &pdev->dev;
>   - v3: initialize dev at declaration
> 
> Signed-off-by: Eugene Shalygin <eugene.shalygin@...il.com>


Please use checkpatch and fix what it reports,, and please
do not send patches as reply to previous versions of a patch
or patch series.

> ---
>   drivers/hwmon/asus-ec-sensors.c | 38 ++++++++++++++-------------------
>   1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
> index bfac08a5dc57..b9eb9126e433 100644
> --- a/drivers/hwmon/asus-ec-sensors.c
> +++ b/drivers/hwmon/asus-ec-sensors.c
> @@ -589,23 +589,33 @@ get_board_sensors(const struct device *dev)
>   	return (unsigned long)dmi_entry->driver_data;
>   }
>   
> -static int __init configure_sensor_setup(struct device *dev)
> +static int __init asus_ec_probe(struct platform_device *pdev)
>   {
> -	struct ec_sensors_data *ec_data = dev_get_drvdata(dev);
> +	const struct hwmon_channel_info **ptr_asus_ec_ci;
>   	int nr_count[hwmon_max] = { 0 }, nr_types = 0;
> -	struct device *hwdev;
>   	struct hwmon_channel_info *asus_ec_hwmon_chan;
> -	const struct hwmon_channel_info **ptr_asus_ec_ci;
>   	const struct hwmon_chip_info *chip_info;
>   	const struct ec_sensor_info *si;
> +	struct ec_sensors_data *ec_data;
>   	enum hwmon_sensor_types type;
> +	unsigned long board_sensors;
> +	struct device *hwdev;
>   	unsigned int i;

I don't really see the point of reordering (most of) the variables
above, only to ...

>   
> -	ec_data->board_sensors = get_board_sensors(dev);
> -	if (!ec_data->board_sensors) {
> +	struct device *dev = &pdev->dev;

... add another variable at the and, after an empty line,
with no following empty line, and completely out of order with
the other variable declarations (if reverse christmas tree order
was the idea).

> +	board_sensors = get_board_sensors(dev);
> +	if (!board_sensors) {
>   		return -ENODEV;
>   	}
>   
> +	ec_data = devm_kzalloc(dev, sizeof(struct ec_sensors_data),
> +			     GFP_KERNEL);
> +	if (!ec_data) {
> +		return -ENOMEM;
> +	}
> +
> +	dev_set_drvdata(dev, ec_data);
> +	ec_data->board_sensors = board_sensors;
>   	ec_data->nr_sensors = board_sensors_count(ec_data->board_sensors);
>   	ec_data->sensors = devm_kcalloc(dev, ec_data->nr_sensors,
>   					sizeof(struct ec_sensor), GFP_KERNEL);
> @@ -666,22 +676,6 @@ static int __init configure_sensor_setup(struct device *dev)
>   	return PTR_ERR_OR_ZERO(hwdev);
>   }
>   
> -static int __init asus_ec_probe(struct platform_device *pdev)
> -{
> -	struct ec_sensors_data *state;
> -	int status = 0;
> -
> -	state = devm_kzalloc(&pdev->dev, sizeof(struct ec_sensors_data),
> -			     GFP_KERNEL);
> -
> -	if (!state) {
> -		return -ENOMEM;
> -	}
> -
> -	dev_set_drvdata(&pdev->dev, state);
> -	status = configure_sensor_setup(&pdev->dev);
> -	return status;
> -}
>   
>   static const struct acpi_device_id acpi_ec_ids[] = {
>   	/* Embedded Controller Device */