Re: [PATCH linux dev-5.8] hwmon: Ampere Computing ALTRA SMPMPRO sensor driver

[Patrick Williams <patrick@stwcx.xyz>]

[-- Attachment #1: Type: text/plain, Size: 3441 bytes --]

On Wed, Sep 30, 2020 at 05:26:57PM -0500, Lancelot wrote:
> From: Lancelot Kao <lancelot.kao@fii-usa.com>
> 
> Add SMPMPro-hwmon driver to monitor Ampere CPU/Memory/VR via an
> i2c interface of the CPU's smpmpro management device.
> 
> Signed-off-by: Xiaopeng XP Chen <xiao-peng.chen@fii-na.com>
> Signed-off-by: Lancelot Kao <lancelot.kao@fii-usa.com>

Nice work at adding this driver.

It does look like you've missed CC'ing upstream though.  Was this
intentional?  (linux-hwmon@vger.kernel.org)

> +/* Capability Registers  */
> +#define TEMP_SENSOR_SUPPORT_REG	0x05
> +#define PWR_SENSOR_SUPPORT_REG	0x06
> +#define VOLT_SENSOR_SUPPORT_REG	0x07
> +#define OTHER_CAP_REG		    0x08
> +#define CORE_CLUSTER_CNT_REG	0x0B
> +#define SYS_CACHE_PCIE_CNT_REG	0x0C
> +#define SOCKET_INFO_REG	        0x0D

There seems to be some sporatic indentation throughout all the #defines
in this file, where it appears you attempted to align the values.  Make
sure you have tabs set to 8-step spacing for kernel code.

> +static void smpmpro_init_device(struct i2c_client *client,
> +				struct smpmpro_data *data)
> +{
> +	u16 ret;
> +
> +	ret = i2c_smbus_read_word_swapped(client, TEMP_SENSOR_SUPPORT_REG);
> +	if (ret < 0)
> +		return;
> +	data->temp_support_regs = ret;

i2c_smbus_read_word_swapped returns a s32 even though you're looking for
a u16.  By setting `ret` to u16 you've caused two problems:

    * You are immediately truncating -ERRNO values into a u16 so that
      you are unable to differentiate values like 0xFFFFFFFF as a
      register value and -1 as an errno.

    * The if condition here can never be true, so you're never catching
      error conditions.  (An u16 can never be negative, so ret < 0 can
      never be true.)

This issue occurs throughout the driver.

> +static int smpmpro_read_temp(struct device *dev, u32 attr, int channel,
> +			     long *val)
> +{
> +	struct smpmpro_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	int ret;

You might want a sized int on this one?  Repeated in most other
functions.

> +static int smpmpro_read_power(struct device *dev, u32 attr, int channel,
> +			     long *val)
> +{
> +	struct smpmpro_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client = data->client;
> +	int ret, ret_mw;
> +	int ret2 = 0, ret2_mw = 0;

Any reason to not initialize ret/ret_mw?  By it being different from
ret2/ret2_mw it makes me question "is this ok?", which spends more time
in review.

> +static int smpmpro_i2c_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)
...
> +	/* Initialize the Altra SMPMPro chip */
> +	smpmpro_init_device(client, data);

I didn't see anything in the smpmpro_init_device function, but is there
anything you can or should do to ensure this device really is an
SMPMPro rather than exclusively relying on the device tree compatible?

> +static struct i2c_driver smpmpro_driver = {
> +	.class		= I2C_CLASS_HWMON,
> +	.probe		= smpmpro_i2c_probe,
> +	.driver = {
> +		.name	= "smpmpro",
> +	},
> +	.id_table	= smpmpro_i2c_id,
> +};
> +
> +module_i2c_driver(smpmpro_driver);

Are you missing the .of_match_table inside .driver?  Is that necessary
or useful for your use?  I'm not sure if you can have device tree
entries that automatically instantiate the hwmon driver otherwise.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]