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;
Message-ID: <0248d597-b72a-5b7c-63a4-6d72384f9854@kernel.org>
Date:   Tue, 8 Aug 2023 09:38:48 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.14.0
Subject: Re: [PATCH 1/2] hwmon: Add driver for EMC181x temperature sensors
Content-Language: en-US
To:     Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>,
        Jean Delvare <jdelvare@suse.com>,
        Guenter Roeck <linux@roeck-us.net>
Cc:     linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org,
        Mathew McBride <matt@traverse.com.au>
References: <20230808013157.80913-1-mark.tomlinson@alliedtelesis.co.nz>
From:   Krzysztof Kozlowski <krzk@kernel.org>
In-Reply-To: <20230808013157.80913-1-mark.tomlinson@alliedtelesis.co.nz>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 08/08/2023 03:31, Mark Tomlinson wrote:
> This patch adds a HWMON driver for the EMC1812, EMC1813, EMC1814,
> EMC1815 and EMC1833 temperature sensor chips from microchip. Does not
> currently support the alert outputs.
> 
> Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
> Co-developed-by: Mathew McBride <matt@traverse.com.au>
> Signed-off-by: Mathew McBride <matt@traverse.com.au>

The order of these tags is clearly not correct. It says Mathew is
sending, but you are not Mathew?

> ---
>  drivers/hwmon/Kconfig   |  10 ++
>  drivers/hwmon/Makefile  |   1 +
>  drivers/hwmon/emc181x.c | 296 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 307 insertions(+)
>  create mode 100644 drivers/hwmon/emc181x.c

...

> +
> +static const struct i2c_device_id emc181x_i2c_id[] = {
> +	{ "emc1812", emc1812 },
> +	{ "emc1813", emc1813 },
> +	{ "emc1814", emc1814 },
> +	{ "emc1815", emc1815 },
> +	{ "emc1833", emc1833 },
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, emc181x_i2c_id);

Keep the entire table next to OF device ID table.

> +
> +/* Return 0 if detection is successful, -ENODEV otherwise */
> +static int emc181x_i2c_detect(struct i2c_client *client,
> +		       struct i2c_board_info *info)
> +{
> +	struct i2c_adapter *adapter = client->adapter;
> +	int pid, id, rev;
> +	const char *name;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_WORD_DATA))
> +		return -ENODEV;
> +
> +	/* Determine the chip type */
> +	id = i2c_smbus_read_byte_data(client, EMC181X_SMSC_ID_REG);
> +	if (id < 0)
> +		return id;
> +	if (id != MICROCHIP_ID)
> +		return -ENODEV;
> +	pid = i2c_smbus_read_byte_data(client, EMC181X_PID_REG);
> +	if (pid < 0)
> +		return pid;
> +	rev = i2c_smbus_read_byte_data(client, EMC181X_REVISION_REG);
> +	if (rev < 0)
> +		return rev;
> +
> +	switch (pid) {
> +	case EMC1812_ID:
> +		name = emc181x_i2c_id[emc1812].name;
> +		break;
> +	case EMC1813_ID:
> +		name = emc181x_i2c_id[emc1813].name;
> +		break;
> +	case EMC1814_ID:
> +		name = emc181x_i2c_id[emc1814].name;
> +		break;
> +	case EMC1815_ID:
> +		name = emc181x_i2c_id[emc1815].name;
> +		break;
> +	case EMC1833_ID:
> +		name = emc181x_i2c_id[emc1833].name;
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +	strscpy(info->type, name, I2C_NAME_SIZE);
> +
> +	dev_dbg(&client->dev,
> +		"Detected device %s at 0x%02x with COMPANY: 0x%02x and PID: 0x%02x REV: 0x%02x\n",
> +		name, client->addr, id, pid, rev);

If you have detection, why not using it for device matching? All devices
should be OF-compatible with one fallback.

> +
> +	return 0;
> +}
> +
> +static int emc181x_i2c_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct device *hwmon_dev;
> +	struct emc181x_data *data;
> +	s32 regval;
> +	u8 config;
> +
> +	data = devm_kzalloc(dev, sizeof(struct emc181x_data), GFP_KERNEL);

sizeof(*)

> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->i2c = client;
> +	if (dev_fwnode(dev)) {
> +		data->type = (enum chips)device_get_match_data(dev);
> +		data->is_extended_range = device_property_present(dev, "emc181x,extended-range");
> +	} else
> +		data->type = i2c_match_id(emc181x_i2c_id, client)->driver_data;

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

> +
> +	regval = i2c_smbus_read_byte_data(client, EMC181X_CONFIG_REG);
> +	if (regval < 0) {
> +		dev_dbg(dev, "Failed to read config reg %d", regval);
> +		return regval;
> +	}
> +
> +	/* By default, extended range is disabled in the EMC181X,

/*
 *
...

> +	 * if enabled we need to set this in the CONFIG register.
> +	 */
> +	config = regval & ~0x04;
> +	if (data->is_extended_range)
> +		config |= 0x04;
> +
> +	dev_dbg(dev, "EMC181X setting CONFIG to %d\n", config);
> +	if (config != regval)
> +		i2c_smbus_write_byte_data(client, EMC181X_CONFIG_REG, config);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev,
> +			client->name,
> +			data,
> +			&emc181x_chip_info[data->type],
> +			NULL);
> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct of_device_id __maybe_unused emc181x_of_match[] = {
> +	{
> +		.compatible = "microchip,emc1812",

Bindings go before the user.

> +		.data = (void *)emc1812
> +	},
> +	{
> +		.compatible = "microchip,emc1813",
> +		.data = (void *)emc1813
> +	},
> +	{
> +		.compatible = "microchip,emc1814",
> +		.data = (void *)emc1814
> +	},
> +	{
> +		.compatible = "microchip,emc1815",
> +		.data = (void *)emc1815
> +	},
> +	{
> +		.compatible = "microchip,emc1833",
> +		.data = (void *)emc1833
> +	},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, emc181x_of_match);
> +
> +static struct i2c_driver emc181x_i2c_driver = {
> +	.driver = {
> +		.name = "emc181x",
> +		.of_match_table = of_match_ptr(emc181x_of_match),
> +	},

Drop of_match_ptr(). This will cause warnings.


> +	.probe = emc181x_i2c_probe,
> +	.id_table = emc181x_i2c_id,
> +	.detect = emc181x_i2c_detect,
> +	.address_list = emc181x_address_list,
> +};
> +
> +module_i2c_driver(emc181x_i2c_driver);
> +
> +MODULE_DESCRIPTION("EMC181X Sensor Driver");
> +MODULE_AUTHOR("Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>");
> +MODULE_LICENSE("GPL");

Best regards,
Krzysztof