Re: [PATCH v5 1/1] hwmon: Add driver for Astera Labs PT5161L retimer

[Krzysztof Kozlowski]

On 05/02/2024 16:20, Cosmo Chou wrote:
> This driver implements support for temperature monitoring of Astera Labs
> PT5161L series PCIe retimer chips.
> 
> This driver implementation originates from the CSDK available at
> Link: https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14
> The communication protocol utilized is based on the I2C/SMBus standard.
> 
> Signed-off-by: Cosmo Chou <chou.cosmo@xxxxxxxxx>
> ---
>  Documentation/hwmon/index.rst   |   1 +
>  Documentation/hwmon/pt5161l.rst |  42 ++
>  MAINTAINERS                     |   7 +
>  drivers/hwmon/Kconfig           |  10 +

...

> +
> +static int pt5161l_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct device *hwmon_dev;
> +	struct pt5161l_data *data;
> +
> +	data = devm_kzalloc(dev, sizeof(struct pt5161l_data), GFP_KERNEL);

sizeof(*)

> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +	mutex_init(&data->lock);
> +	pt5161l_init_dev(data);
> +	dev_set_drvdata(dev, data);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> +							 data,
> +							 &pt5161l_chip_info,
> +							 NULL);
> +
> +	pt5161l_init_debugfs(data);
> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static void pt5161l_remove(struct i2c_client *client)
> +{
> +	struct pt5161l_data *data = i2c_get_clientdata(client);
> +
> +	debugfs_remove_recursive(data->debugfs);
> +}
> +
> +static const struct of_device_id __maybe_unused pt5161l_of_match[] = {
> +	{ .compatible = "asteralabs,pt5161l" },

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.

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, pt5161l_of_match);
> +
> +static const struct acpi_device_id __maybe_unused pt5161l_acpi_match[] = {
> +	{ "PT5161L", 0 },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(acpi, pt5161l_acpi_match);
> +
> +static const struct i2c_device_id pt5161l_id[] = {
> +	{ "pt5161l", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, pt5161l_id);
> +
> +static struct i2c_driver pt5161l_driver = {
> +	.class = I2C_CLASS_HWMON,
> +	.driver = {
> +		.name = "pt5161l",
> +		.of_match_table = of_match_ptr(pt5161l_of_match),
> +		.acpi_match_table = ACPI_PTR(pt5161l_acpi_match),
> +	},
> +	.probe = pt5161l_probe,
> +	.remove = pt5161l_remove,
> +	.id_table = pt5161l_id,
> +};
> +
> +static int __init pt5161l_init(void)
> +{
> +	pt5161l_debugfs_dir = debugfs_create_dir("pt5161l", NULL);

Drivers don't need initcalls. For sure any debugfs should not be handled
here but in probe.


Best regards,
Krzysztof