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

[Ivan Vecera <ivecera@redhat.com>]

On 18. 06. 25 10:56 dop., Jonathan Cameron wrote:
> 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.

Will fix.

>> +	}
>> +
>> +	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.

Will update according this.

>> +	},
>> +	{
>> +		.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");
> 

Thanks for advice.

Ivan