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

[Ivan Vecera]

On 07. 04. 25 8:05 odp., Andy Shevchenko wrote:
> 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"
>
> ...

Will fix in the next series.

> > +/*
> > + * 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?

What is wrong here?

I have a device that uses 7-bit addresses and have 16 register pages.
Each pages is from 0x00-0x7f and register 0x7f is used as page selector 
where bits 0-3 select the page.

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

Will fix.

> > +	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?

This function is used and filled later. I will drop it here and 
introduce when it will be necessary.

> > +#include <linux/i2c.h>
>
> > +#include <linux/kernel.h>
>
> No usual driver should include kernel.h, please follow IWYU principle.

Will follow IWYU in v2.

> > +#include <linux/module.h>
>
> Again, this is just a random list of headers, see above and follow the IWYU
> principle.

Ditto.

> > +#include "zl3073x.h"
>
> ...
>
> > +static const struct i2c_device_id zl3073x_i2c_id[] = {
> > +	{ "zl3073x-i2c", },
>
> Redundant inner comma.

Ack

> > +	{ /* sentinel */ },
>
> No comma for the sentinel.
>
> > +};

Ack

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

Ack

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

Sorry for that, it was originally necessary.
Will drop.

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

I tried to follow strictly 80 chars/line. Will fix.

> > +	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(...);

Will change.

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

Ack

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

Will rename zl3073x_dev_alloc() to zl3073x_devm_alloc() to indicate that 
devres is used... Probably will drop zl3073x_dev_exit() entirely and 
take care of devlink unregistration by devres way.

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

Ack

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

Will fix.

> > +#include <linux/module.h>
>
> > +#include <linux/of.h>

Ack

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

Ack

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

OK, will fix.

> > +#ifndef __ZL3073X_CORE_H
> > +#define __ZL3073X_CORE_H
>
> > +#include <linux/mfd/zl3073x.h>
>
> How is it used here, please?

Will change to forward declaration.

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

The same, forward declaration and inclusion of <linux/mutex.h>

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

Thank you Andy for the review.

I.