Re: [PATCH 01/28] mfd: Add Microchip ZL3073x support

[Andy Shevchenko <andy@kernel.org>]

On Mon, Apr 07, 2025 at 07:28:28PM +0200, Ivan Vecera wrote:
> This adds base MFD driver for Microchip Azurite ZL3073x chip family.
> These chips provide DPLL and PHC (PTP) functionality and they can
> be connected over I2C or SPI bus.
> 
> The MFD driver provide basic communication and synchronization
> over the bus and common functionality that are used by the DPLL
> driver (later in this series) and by the PTP driver (will be
> added later).
> 
> The chip family is characterized by following properties:
> * 2 separate DPLL units (channels)
> * 5 synthesizers
> * 10 input pins (references)
> * 10 outputs
> * 20 output pins (output pin pair shares one output)
> * Each reference and output can act in differential or single-ended
>   mode (reference or output in differential mode consumes 2 pins)
> * Each output is connected to one of the synthesizers
> * Each synthesizer is driven by one of the DPLL unit
.
The comments below are applicable to entire series, take your time and fix
*all* stylic and header issues before sending v2.

...

+ array_size.h
+ bits.h

+ device/devres.h

> +#include <linux/module.h>

This file uses *much* amore than that.

+ regmap.h


> +#include "zl3073x.h"

...

> +/*
> + * Regmap ranges
> + */
> +#define ZL3073x_PAGE_SIZE	128
> +#define ZL3073x_NUM_PAGES	16
> +#define ZL3073x_PAGE_SEL	0x7F
> +
> +static const struct regmap_range_cfg zl3073x_regmap_ranges[] = {
> +	{
> +		.range_min	= 0,

Are you sure you get the idea of virtual address pages here?

> +		.range_max	= ZL3073x_NUM_PAGES * ZL3073x_PAGE_SIZE,
> +		.selector_reg	= ZL3073x_PAGE_SEL,
> +		.selector_mask	= GENMASK(3, 0),
> +		.selector_shift	= 0,
> +		.window_start	= 0,
> +		.window_len	= ZL3073x_PAGE_SIZE,
> +	},
> +};

...

> +struct zl3073x_dev *zl3073x_dev_alloc(struct device *dev)
> +{
> +	struct zl3073x_dev *zldev;
> +
> +	return devm_kzalloc(dev, sizeof(*zldev), GFP_KERNEL);
> +}
> +EXPORT_SYMBOL_NS_GPL(zl3073x_dev_alloc, "ZL3073X");
> +
> +int zl3073x_dev_init(struct zl3073x_dev *zldev)
> +{
> +	devm_mutex_init(zldev->dev, &zldev->lock);

Missing check.

> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(zl3073x_dev_init, "ZL3073X");
> +
> +void zl3073x_dev_exit(struct zl3073x_dev *zldev)
> +{
> +}
> +EXPORT_SYMBOL_NS_GPL(zl3073x_dev_exit, "ZL3073X");

What's the point in these stubs?

...

> +#include <linux/i2c.h>

> +#include <linux/kernel.h>

No usual driver should include kernel.h, please follow IWYU principle.

> +#include <linux/module.h>

Again, this is just a random list of headers, see above and follow the IWYU
principle.

> +#include "zl3073x.h"

...

> +static const struct i2c_device_id zl3073x_i2c_id[] = {
> +	{ "zl3073x-i2c", },

Redundant inner comma.

> +	{ /* sentinel */ },

No comma for the sentinel.

> +};

...

> +static const struct of_device_id zl3073x_i2c_of_match[] = {
> +	{ .compatible = "microchip,zl3073x-i2c" },
> +	{ /* sentinel */ },

No comma.

> +};

> +static int zl3073x_i2c_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	const struct i2c_device_id *id;
> +	struct zl3073x_dev *zldev;

> +	int rc = 0;

Useless assignment.

> +	zldev = zl3073x_dev_alloc(dev);
> +	if (!zldev)
> +		return -ENOMEM;
> +
> +	id = i2c_client_get_device_id(client);
> +	zldev->dev = dev;
> +
> +	zldev->regmap = devm_regmap_init_i2c(client,
> +					     zl3073x_get_regmap_config());

It's perfectly a single line.

> +	if (IS_ERR(zldev->regmap)) {
> +		rc = PTR_ERR(zldev->regmap);
> +		dev_err(dev, "Failed to allocate register map: %d\n", rc);
> +		return rc;

		return dev_err_probe(...);

> +	}
> +
> +	i2c_set_clientdata(client, zldev);
> +
> +	return zl3073x_dev_init(zldev);
> +}

...

> +static void zl3073x_i2c_remove(struct i2c_client *client)
> +{

> +	struct zl3073x_dev *zldev;
> +
> +	zldev = i2c_get_clientdata(client);

Just make it one line definition + assignment.

> +	zl3073x_dev_exit(zldev);

This is a red flag and because you haven't properly named the calls (i.e. devm
to show that they are only for probe stage and use managed resources) this is
not easy to catch.

> +}
> +
> +static struct i2c_driver zl3073x_i2c_driver = {
> +	.driver = {
> +		.name = "zl3073x-i2c",
> +		.of_match_table = of_match_ptr(zl3073x_i2c_of_match),

Please, never use of_match_ptr() or ACPI_PTR() in a new code.

> +	},
> +	.probe = zl3073x_i2c_probe,
> +	.remove = zl3073x_i2c_remove,
> +	.id_table = zl3073x_i2c_id,
> +};

> +

Redundant blank line.

> +module_i2c_driver(zl3073x_i2c_driver);

...

> +#include <linux/kernel.h>

Just no. You should know what you are doing in the driver.

> +#include <linux/module.h>

> +#include <linux/of.h>

Just no. Use agnostic APIs.

> +#include <linux/spi/spi.h>
> +#include "zl3073x.h"

...

> +static const struct spi_device_id zl3073x_spi_id[] = {
> +	{ "zl3073x-spi", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(spi, zl3073x_spi_id);
> +
> +static const struct of_device_id zl3073x_spi_of_match[] = {
> +	{ .compatible = "microchip,zl3073x-spi" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, zl3073x_spi_of_match);

Move the above closer to its user.

...

> +static int zl3073x_spi_probe(struct spi_device *spidev)

Usual name is spi for the above, it's shorter and allows to tidy up the code.

And below same comments as for i2c part of the driver.

...

> +#ifndef __ZL3073X_CORE_H
> +#define __ZL3073X_CORE_H

> +#include <linux/mfd/zl3073x.h>

How is it used here, please?

> +struct zl3073x_dev *zl3073x_dev_alloc(struct device *dev);
> +int zl3073x_dev_init(struct zl3073x_dev *zldev);
> +void zl3073x_dev_exit(struct zl3073x_dev *zldev);
> +const struct regmap_config *zl3073x_get_regmap_config(void);
> +
> +#endif /* __ZL3073X_CORE_H */

...

> +#ifndef __LINUX_MFD_ZL3073X_H
> +#define __LINUX_MFD_ZL3073X_H

> +#include <linux/device.h>
> +#include <linux/regmap.h>

Ditto. Two unused headers and one which must be included is missed.

> +struct zl3073x_dev {
> +	struct device		*dev;
> +	struct regmap		*regmap;
> +	struct mutex		lock;
> +};

-- 
With Best Regards,
Andy Shevchenko