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;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <a9c6c44c-21fb-9d42-9a6d-6685e110015d@roeck-us.net>
Date:   Tue, 4 Jul 2023 06:39:07 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Content-Language: en-US
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-hwmon@vger.kernel.org
Cc:     samsagax@gmail.com, linux-kernel@vger.kernel.org
References: <20230704131715.44454-5-gregkh@linuxfoundation.org>
 <20230704131715.44454-6-gregkh@linuxfoundation.org>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH 1/3] hwmon: (oxp-sensors): remove static board variable
In-Reply-To: <20230704131715.44454-6-gregkh@linuxfoundation.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 7/4/23 06:17, Greg Kroah-Hartman wrote:
> Drivers should not have a single static variable for the type of device
> they are bound to.  While this driver is really going to only have one
> device at a time in the system, remove the static variable and instead,
> look up the device type when needed.
> 

This is expensive. I think it would be much better to just move
the board type detection into the init code and not instantiate
the driver in the fist place if the board type is unknown.

We can handle the static variable separately if it really bothers
you that much.

> Cc: Joaquín Ignacio Aramendía <samsagax@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-hwmon@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>   drivers/hwmon/oxp-sensors.c | 47 ++++++++++++++++++++-----------------
>   1 file changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> index e1a907cae820..3bcba0c476c4 100644
> --- a/drivers/hwmon/oxp-sensors.c
> +++ b/drivers/hwmon/oxp-sensors.c
> @@ -48,10 +48,9 @@ enum oxp_board {
>   	oxp_mini_amd,
>   	oxp_mini_amd_a07,
>   	oxp_mini_amd_pro,
> +	UNKNOWN,
>   };
>   
> -static enum oxp_board board;
> -
>   /* Fan reading and PWM */
>   #define OXP_SENSOR_FAN_REG		0x76 /* Fan reading is 2 registers long */
>   #define OXP_SENSOR_PWM_ENABLE_REG	0x4A /* PWM enable is 1 register long */
> @@ -136,6 +135,24 @@ static const struct dmi_system_id dmi_table[] = {
>   	{},
>   };
>   
> +static enum oxp_board get_board_type(void)
> +{
> +	const struct dmi_system_id *dmi_entry;
> +
> +	/*
> +	 * Have to check for AMD processor here because DMI strings are the
> +	 * same between Intel and AMD boards, the only way to tell them apart
> +	 * is the CPU.
> +	 * Intel boards seem to have different EC registers and values to
> +	 * read/write.
> +	 */
> +	dmi_entry = dmi_first_match(dmi_table);
> +	if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> +		return UNKNOWN;
> +
> +	return (enum oxp_board)(unsigned long)dmi_entry->driver_data;
> +}
> +
>   /* Helper functions to handle EC read/write */
>   static int read_from_ec(u8 reg, int size, long *val)
>   {
> @@ -182,7 +199,7 @@ static int tt_toggle_enable(void)
>   	u8 reg;
>   	u8 val;
>   
> -	switch (board) {
> +	switch (get_board_type()) {
>   	case oxp_mini_amd_a07:
>   		reg = OXP_OLD_TURBO_SWITCH_REG;
>   		val = OXP_OLD_TURBO_TAKE_VAL;
> @@ -203,7 +220,7 @@ static int tt_toggle_disable(void)
>   	u8 reg;
>   	u8 val;
>   
> -	switch (board) {
> +	switch (get_board_type()) {
>   	case oxp_mini_amd_a07:
>   		reg = OXP_OLD_TURBO_SWITCH_REG;
>   		val = OXP_OLD_TURBO_RETURN_VAL;
> @@ -249,7 +266,7 @@ static ssize_t tt_toggle_show(struct device *dev,
>   	u8 reg;
>   	long val;
>   
> -	switch (board) {
> +	switch (get_board_type()) {
>   	case oxp_mini_amd_a07:
>   		reg = OXP_OLD_TURBO_SWITCH_REG;
>   		break;
> @@ -315,7 +332,7 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
>   			ret = read_from_ec(OXP_SENSOR_PWM_REG, 1, val);
>   			if (ret)
>   				return ret;
> -			switch (board) {
> +			switch (get_board_type()) {
>   			case aya_neo_2:
>   			case aya_neo_air:
>   			case aya_neo_air_pro:
> @@ -357,7 +374,7 @@ static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
>   		case hwmon_pwm_input:
>   			if (val < 0 || val > 255)
>   				return -EINVAL;
> -			switch (board) {
> +			switch (get_board_type()) {
>   			case aya_neo_2:
>   			case aya_neo_air:
>   			case aya_neo_air_pro:
> @@ -412,25 +429,11 @@ static const struct hwmon_chip_info oxp_ec_chip_info = {
>   /* Initialization logic */
>   static int oxp_platform_probe(struct platform_device *pdev)
>   {
> -	const struct dmi_system_id *dmi_entry;
>   	struct device *dev = &pdev->dev;
>   	struct device *hwdev;
>   	int ret;
>   
> -	/*
> -	 * Have to check for AMD processor here because DMI strings are the
> -	 * same between Intel and AMD boards, the only way to tell them apart
> -	 * is the CPU.
> -	 * Intel boards seem to have different EC registers and values to
> -	 * read/write.
> -	 */
> -	dmi_entry = dmi_first_match(dmi_table);
> -	if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> -		return -ENODEV;
> -
> -	board = (enum oxp_board)(unsigned long)dmi_entry->driver_data;
> -
> -	switch (board) {
> +	switch (get_board_type()) {

This now always registers the hwmon device, which really should not happen.

Guenter

>   	case aok_zoe_a1:
>   	case oxp_mini_amd_a07:
>   	case oxp_mini_amd_pro: