Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 27 Aug 2020 12:51:50 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Tim Harvey <tharvey@gateworks.com>
Cc:     Jean Delvare <jdelvare@suse.com>,
        Robert Jones <rjones@gateworks.com>,
        linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] hwmon: gsc-hwmon: add fan sensor
Message-ID: <20200827195150.GA233489@roeck-us.net>
References: <1598548892-17103-1-git-send-email-tharvey@gateworks.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1598548892-17103-1-git-send-email-tharvey@gateworks.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Aug 27, 2020 at 10:21:32AM -0700, Tim Harvey wrote:
> Add a fan sensor to report RPM's from a fan tach input.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  drivers/hwmon/gsc-hwmon.c               | 34 ++++++++++++++++++++++++++++++---
>  include/linux/platform_data/gsc_hwmon.h |  1 +
>  2 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/gsc-hwmon.c b/drivers/hwmon/gsc-hwmon.c
> index c6d4567..a86a0e5 100644
> --- a/drivers/hwmon/gsc-hwmon.c
> +++ b/drivers/hwmon/gsc-hwmon.c
> @@ -17,6 +17,7 @@
>  
>  #define GSC_HWMON_MAX_TEMP_CH	16
>  #define GSC_HWMON_MAX_IN_CH	16
> +#define GSC_HWMON_MAX_FAN_CH	16
>  
>  #define GSC_HWMON_RESOLUTION	12
>  #define GSC_HWMON_VREF		2500
> @@ -27,11 +28,14 @@ struct gsc_hwmon_data {
>  	struct regmap *regmap;
>  	const struct gsc_hwmon_channel *temp_ch[GSC_HWMON_MAX_TEMP_CH];
>  	const struct gsc_hwmon_channel *in_ch[GSC_HWMON_MAX_IN_CH];
> +	const struct gsc_hwmon_channel *fan_ch[GSC_HWMON_MAX_FAN_CH];
>  	u32 temp_config[GSC_HWMON_MAX_TEMP_CH + 1];
>  	u32 in_config[GSC_HWMON_MAX_IN_CH + 1];
> +	u32 fan_config[GSC_HWMON_MAX_FAN_CH + 1];
>  	struct hwmon_channel_info temp_info;
>  	struct hwmon_channel_info in_info;
> -	const struct hwmon_channel_info *info[3];
> +	struct hwmon_channel_info fan_info;
> +	const struct hwmon_channel_info *info[4];
>  	struct hwmon_chip_info chip;
>  };
>  
> @@ -155,6 +159,9 @@ gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>  	case hwmon_temp:
>  		ch = hwmon->temp_ch[channel];
>  		break;
> +	case hwmon_fan:
> +		ch = hwmon->fan_ch[channel];
> +		break;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -187,6 +194,9 @@ gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>  		/* adjust by uV offset */
>  		tmp += ch->mvoffset;
>  		break;
> +	case mode_fan:
> +		tmp *= 30; /* convert to revolutions per minute */
> +		break;
>  	case mode_voltage_24bit:
>  	case mode_voltage_16bit:
>  		/* no adjustment needed */
> @@ -211,6 +221,9 @@ gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
>  	case hwmon_temp:
>  		*buf = hwmon->temp_ch[channel]->name;
>  		break;
> +	case hwmon_fan:
> +		*buf = hwmon->fan_ch[channel]->name;
> +		break;
>  	default:
>  		return -ENOTSUPP;
>  	}
> @@ -304,7 +317,7 @@ static int gsc_hwmon_probe(struct platform_device *pdev)
>  	struct gsc_hwmon_platform_data *pdata = dev_get_platdata(dev);
>  	struct gsc_hwmon_data *hwmon;
>  	const struct attribute_group **groups;
> -	int i, i_in, i_temp;
> +	int i, i_in, i_temp, i_fan;
>  
>  	if (!pdata) {
>  		pdata = gsc_hwmon_get_devtree_pdata(dev);
> @@ -324,7 +337,9 @@ static int gsc_hwmon_probe(struct platform_device *pdev)
>  	if (IS_ERR(hwmon->regmap))
>  		return PTR_ERR(hwmon->regmap);
>  
> -	for (i = 0, i_in = 0, i_temp = 0; i < hwmon->pdata->nchannels; i++) {
> +	for (i = 0, i_in = 0, i_temp = 0, i_fan = 0;
> +	     i < hwmon->pdata->nchannels; i++)
> +	{

ERROR: that open brace { should be on the previous line
#125: FILE: drivers/hwmon/gsc-hwmon.c:340:
+	for (i = 0, i_in = 0, i_temp = 0, i_fan = 0;
+	     i < hwmon->pdata->nchannels; i++)
+	{

Besides, the line length limit is now 100 columns, so the line split
is unnecessary.

Thanks,
Guenter