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

Ivan Vecera
2025-04-09T06:40:35Z

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.