Re: [PATCH net-next v11 03/14] dpll: Add basic Microchip ZL3073x support

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Mon, 16 Jun 2025 22:13:53 +0200
Ivan Vecera <ivecera@redhat.com> wrote:

> Microchip Azurite ZL3073x represents chip family providing DPLL
> and optionally PHC (PTP) functionality. The chips can be connected
> be connected over I2C or SPI bus.
> 
> They have the following characteristics:
> * up to 5 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 operate in either differential or
>   single-ended mode (differential mode uses 2 pins)
> * Each output is connected to one of the synthesizers
> * Each synthesizer is driven by one of the DPLL unit
> 
> The device uses 7-bit addresses and 8-bits values. It exposes 8-, 16-,
> 32- and 48-bits registers in address range <0x000,0x77F>. Due to 7bit
> addressing, the range is organized into pages of 128 bytes, with each
> page containing a page selector register at address 0x7F.
> For reading/writing multi-byte registers, the device supports bulk
> transfers.
> 
> Add basic functionality to access device registers and probe
> functionality for both I2C and SPI cases.
> 
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
A few trivial drive by comments.

> diff --git a/drivers/dpll/zl3073x/i2c.c b/drivers/dpll/zl3073x/i2c.c
> new file mode 100644
> index 0000000000000..bca1cd729895c
> --- /dev/null
> +++ b/drivers/dpll/zl3073x/i2c.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#include "core.h"
> +
> +static int zl3073x_i2c_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct zl3073x_dev *zldev;
> +
> +	zldev = zl3073x_devm_alloc(dev);
> +	if (IS_ERR(zldev))
> +		return PTR_ERR(zldev);
> +
> +	zldev->regmap = devm_regmap_init_i2c(client, &zl3073x_regmap_config);
> +	if (IS_ERR(zldev->regmap)) {
> +		dev_err_probe(dev, PTR_ERR(zldev->regmap),
> +			      "Failed to initialize regmap\n");
> +		return PTR_ERR(zldev->regmap);
As below.

> +	}

> diff --git a/drivers/dpll/zl3073x/spi.c b/drivers/dpll/zl3073x/spi.c
> new file mode 100644
> index 0000000000000..219676da71b78
> --- /dev/null
> +++ b/drivers/dpll/zl3073x/spi.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +
> +#include "core.h"
> +
> +static int zl3073x_spi_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct zl3073x_dev *zldev;
> +
> +	zldev = zl3073x_devm_alloc(dev);
> +	if (IS_ERR(zldev))
> +		return PTR_ERR(zldev);
> +
> +	zldev->regmap = devm_regmap_init_spi(spi, &zl3073x_regmap_config);
> +	if (IS_ERR(zldev->regmap)) {
> +		dev_err_probe(dev, PTR_ERR(zldev->regmap),
> +			      "Failed to initialize regmap\n");
> +		return PTR_ERR(zldev->regmap);

return dev_err_probe();
One of it's biggest advantages is that dev_err_probe() returns the
ret value passed in avoiding duplication like this and saving
a few lines of code each time.

> +	}
> +
> +	return zl3073x_dev_probe(zldev, spi_get_device_match_data(spi));
> +}
> +
> +static const struct spi_device_id zl3073x_spi_id[] = {
> +	{
> +		.name = "zl30731",
> +		.driver_data = (kernel_ulong_t)&zl3073x_chip_info[ZL30731],

Not my subsystem so up to you, but in general over time we've found that
an enum + array tends to bring few benefits over appropriately named
zl30731_chip_info separate structures.

> +	},
> +	{
> +		.name = "zl30732",
> +		.driver_data = (kernel_ulong_t)&zl3073x_chip_info[ZL30732],
> +	},
> +	{
> +		.name = "zl30733",
> +		.driver_data = (kernel_ulong_t)&zl3073x_chip_info[ZL30733],
> +	},
> +	{
> +		.name = "zl30734",
> +		.driver_data = (kernel_ulong_t)&zl3073x_chip_info[ZL30734],
> +	},
> +	{
> +		.name = "zl30735",
> +		.driver_data = (kernel_ulong_t)&zl3073x_chip_info[ZL30735]
> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(spi, zl3073x_spi_id);
> +
> +static const struct of_device_id zl3073x_spi_of_match[] = {
> +	{
> +		.compatible = "microchip,zl30731",
> +		.data = &zl3073x_chip_info[ZL30731]
> +	},
> +	{
> +		.compatible = "microchip,zl30732",
> +		.data = &zl3073x_chip_info[ZL30732]
> +	},
> +	{
> +		.compatible = "microchip,zl30733",
> +		.data = &zl3073x_chip_info[ZL30733]
> +	},
> +	{
> +		.compatible = "microchip,zl30734",
> +		.data = &zl3073x_chip_info[ZL30734]
> +	},
> +	{
> +		.compatible = "microchip,zl30735",
> +		.data = &zl3073x_chip_info[ZL30735]
> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, zl3073x_spi_of_match);
> +
> +static struct spi_driver zl3073x_spi_driver = {
> +	.driver = {
> +		.name = "zl3073x-spi",
> +		.of_match_table = zl3073x_spi_of_match,
> +	},
> +	.probe = zl3073x_spi_probe,
> +	.id_table = zl3073x_spi_id,
> +};
> +module_spi_driver(zl3073x_spi_driver);
> +
> +MODULE_AUTHOR("Ivan Vecera <ivecera@redhat.com>");
> +MODULE_DESCRIPTION("Microchip ZL3073x SPI driver");
> +MODULE_IMPORT_NS("ZL3073X");
> +MODULE_LICENSE("GPL");