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
Reviewed-by: Michal Schmidt <mschmidt@redhat.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
MAINTAINERS | 8 +++++
drivers/mfd/Kconfig | 30 ++++++++++++++++
drivers/mfd/Makefile | 5 +++
drivers/mfd/zl3073x-core.c | 70 ++++++++++++++++++++++++++++++++++++
drivers/mfd/zl3073x-i2c.c | 70 ++++++++++++++++++++++++++++++++++++
drivers/mfd/zl3073x-spi.c | 71 +++++++++++++++++++++++++++++++++++++
drivers/mfd/zl3073x.h | 13 +++++++
include/linux/mfd/zl3073x.h | 15 ++++++++
8 files changed, 282 insertions(+)
create mode 100644 drivers/mfd/zl3073x-core.c
create mode 100644 drivers/mfd/zl3073x-i2c.c
create mode 100644 drivers/mfd/zl3073x-spi.c
create mode 100644 drivers/mfd/zl3073x.h
create mode 100644 include/linux/mfd/zl3073x.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 4c5c2e2c12787..c69a69d862310 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15994,6 +15994,14 @@ L: linux-wireless@vger.kernel.org
S: Supported
F: drivers/net/wireless/microchip/
+MICROCHIP ZL3073X DRIVER
+M: Ivan Vecera <ivecera@redhat.com>
+M: Prathosh Satish <Prathosh.Satish@microchip.com>
+L: netdev@vger.kernel.org
+S: Supported
+F: drivers/mfd/zl3073x*
+F: include/linux/mfd/zl3073x.h
+
MICROSEMI MIPS SOCS
M: Alexandre Belloni <alexandre.belloni@bootlin.com>
M: UNGLinuxDriver@microchip.com
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 22b9363100394..30b36e3ee8f7f 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2422,5 +2422,35 @@ config MFD_UPBOARD_FPGA
To compile this driver as a module, choose M here: the module will be
called upboard-fpga.
+config MFD_ZL3073X_CORE
+ tristate
+ select MFD_CORE
+
+config MFD_ZL3073X_I2C
+ tristate "Microchip Azurite DPLL/PTP/SyncE with I2C"
+ depends on I2C
+ select MFD_ZL3073X_CORE
+ select REGMAP_I2C
+ help
+ Support for Microchip Azurite DPLL/PTP/SyncE chip family. This option
+ supports I2C as the control interface.
+
+ This driver provides common support for accessing the device.
+ Additional drivers must be enabled in order to use the functionality
+ of the device.
+
+config MFD_ZL3073X_SPI
+ tristate "Microchip Azurite DPLL/PTP/SyncE with SPI"
+ depends on SPI
+ select MFD_ZL3073X_CORE
+ select REGMAP_SPI
+ help
+ Support for Microchip Azurite DPLL/PTP/SyncE chip family. This option
+ supports SPI as the control interface.
+
+ This driver provides common support for accessing the device.
+ Additional drivers must be enabled in order to use the functionality
+ of the device.
+
endmenu
endif
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 948cbdf42a18b..76e2babc1538f 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -290,3 +290,8 @@ obj-$(CONFIG_MFD_RSMU_I2C) += rsmu_i2c.o rsmu_core.o
obj-$(CONFIG_MFD_RSMU_SPI) += rsmu_spi.o rsmu_core.o
obj-$(CONFIG_MFD_UPBOARD_FPGA) += upboard-fpga.o
+
+zl3073x-y := zl3073x-core.o
+obj-$(CONFIG_MFD_ZL3073X_CORE) += zl3073x.o
+obj-$(CONFIG_MFD_ZL3073X_I2C) += zl3073x-i2c.o
+obj-$(CONFIG_MFD_ZL3073X_SPI) += zl3073x-spi.o
diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
new file mode 100644
index 0000000000000..67a9d5a0e2d8c
--- /dev/null
+++ b/drivers/mfd/zl3073x-core.c
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/module.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,
+ .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,
+ },
+};
+
+/*
+ * Regmap config
+ */
+const struct regmap_config zl3073x_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = ZL3073x_NUM_PAGES * ZL3073x_PAGE_SIZE,
+ .ranges = zl3073x_regmap_ranges,
+ .num_ranges = ARRAY_SIZE(zl3073x_regmap_ranges),
+};
+
+/**
+ * zl3073x_get_regmap_config - return pointer to regmap config
+ *
+ * Returns pointer to regmap config
+ */
+const struct regmap_config *zl3073x_get_regmap_config(void)
+{
+ return &zl3073x_regmap_config;
+}
+EXPORT_SYMBOL_NS_GPL(zl3073x_get_regmap_config, "ZL3073X");
+
+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);
+
+ 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");
+
+MODULE_AUTHOR("Ivan Vecera <ivecera@redhat.com>");
+MODULE_DESCRIPTION("Microchip ZL3073x core driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/zl3073x-i2c.c b/drivers/mfd/zl3073x-i2c.c
new file mode 100644
index 0000000000000..8c8b2ba176766
--- /dev/null
+++ b/drivers/mfd/zl3073x-i2c.c
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include "zl3073x.h"
+
+static const struct i2c_device_id zl3073x_i2c_id[] = {
+ { "zl3073x-i2c", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i2c, zl3073x_i2c_id);
+
+static const struct of_device_id zl3073x_i2c_of_match[] = {
+ { .compatible = "microchip,zl3073x-i2c" },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, zl3073x_i2c_of_match);
+
+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;
+
+ 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());
+ if (IS_ERR(zldev->regmap)) {
+ rc = PTR_ERR(zldev->regmap);
+ dev_err(dev, "Failed to allocate register map: %d\n", rc);
+ return rc;
+ }
+
+ 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);
+ zl3073x_dev_exit(zldev);
+}
+
+static struct i2c_driver zl3073x_i2c_driver = {
+ .driver = {
+ .name = "zl3073x-i2c",
+ .of_match_table = of_match_ptr(zl3073x_i2c_of_match),
+ },
+ .probe = zl3073x_i2c_probe,
+ .remove = zl3073x_i2c_remove,
+ .id_table = zl3073x_i2c_id,
+};
+
+module_i2c_driver(zl3073x_i2c_driver);
+
+MODULE_AUTHOR("Ivan Vecera <ivecera@redhat.com>");
+MODULE_DESCRIPTION("Microchip ZL3073x I2C driver");
+MODULE_IMPORT_NS("ZL3073X");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/zl3073x-spi.c b/drivers/mfd/zl3073x-spi.c
new file mode 100644
index 0000000000000..a6b9a366a7585
--- /dev/null
+++ b/drivers/mfd/zl3073x-spi.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#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);
+
+static int zl3073x_spi_probe(struct spi_device *spidev)
+{
+ struct device *dev = &spidev->dev;
+ const struct spi_device_id *id;
+ struct zl3073x_dev *zldev;
+ int rc;
+
+ zldev = zl3073x_dev_alloc(dev);
+ if (!zldev)
+ return -ENOMEM;
+
+ id = spi_get_device_id(spidev);
+ zldev->dev = dev;
+
+ zldev->regmap = devm_regmap_init_spi(spidev,
+ zl3073x_get_regmap_config());
+ if (IS_ERR(zldev->regmap)) {
+ rc = PTR_ERR(zldev->regmap);
+ dev_err(dev, "Failed to allocate register map: %d\n", rc);
+ return rc;
+ }
+
+ spi_set_drvdata(spidev, zldev);
+
+ return zl3073x_dev_init(zldev);
+}
+
+static void zl3073x_spi_remove(struct spi_device *spidev)
+{
+ struct zl3073x_dev *zldev;
+
+ zldev = spi_get_drvdata(spidev);
+ zl3073x_dev_exit(zldev);
+}
+
+static struct spi_driver zl3073x_spi_driver = {
+ .driver = {
+ .name = "zl3073x-spi",
+ .of_match_table = of_match_ptr(zl3073x_spi_of_match),
+ },
+ .probe = zl3073x_spi_probe,
+ .remove = zl3073x_spi_remove,
+ .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");
diff --git a/drivers/mfd/zl3073x.h b/drivers/mfd/zl3073x.h
new file mode 100644
index 0000000000000..582cb40d681d3
--- /dev/null
+++ b/drivers/mfd/zl3073x.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __ZL3073X_CORE_H
+#define __ZL3073X_CORE_H
+
+#include <linux/mfd/zl3073x.h>
+
+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 */
diff --git a/include/linux/mfd/zl3073x.h b/include/linux/mfd/zl3073x.h
new file mode 100644
index 0000000000000..7b80c3059b5f3
--- /dev/null
+++ b/include/linux/mfd/zl3073x.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __LINUX_MFD_ZL3073X_H
+#define __LINUX_MFD_ZL3073X_H
+
+#include <linux/device.h>
+#include <linux/regmap.h>
+
+struct zl3073x_dev {
+ struct device *dev;
+ struct regmap *regmap;
+ struct mutex lock;
+};
+
+#endif /* __LINUX_MFD_ZL3073X_H */
--
2.48.1On 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
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.
On Wed, Apr 09, 2025 at 08:40:22AM +0200, Ivan Vecera wrote:
> On 07. 04. 25 8:05 odp., Andy Shevchenko wrote:
> > On Mon, Apr 07, 2025 at 07:28:28PM +0200, Ivan Vecera wrote:
...
> > > +/*
> > > + * 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.
The problem is that you overlap virtual page over the real one (the main one).
The drivers you mentioned in v2 discussions most likely are also buggy.
As I implied in the above question the developers hardly get the regmap ranges
right. It took me quite a while to see the issue, so it's not particularly your
fault.
> > > + .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,
> > > + },
> > > +};
...
> > > + 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.
Yes, but in some cases when it gains in readability it is allowed to use longer
lines even if one sticks with stricter 80 characters limit.
--
With Best Regards,
Andy Shevchenko
On 14. 04. 25 8:36 dop., Andy Shevchenko wrote:
>> 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.
> The problem is that you overlap virtual page over the real one (the main one).
>
> The drivers you mentioned in v2 discussions most likely are also buggy.
> As I implied in the above question the developers hardly get the regmap ranges
> right. It took me quite a while to see the issue, so it's not particularly your
> fault.
Hi Andy,
thank you I see the point.
Do you mean that the selector register should not be part of the range?
If so, does it mean that I have to specify a range for each page? Like this:
{
/* Page 0 */
.range_min = 0x000,
.range_max = 0x07e,
.selector_reg = ZL3073x_PAGE_SEL,
.selector_mask = GENMASK(3, 0),
.selector_shift = 0,
.window_start = 0,
.window_len = 0x7e,
},
{
/* Page 1 */
.range_min = 0x080,
.range_max = 0x0fe,
.selector_reg = ZL3073x_PAGE_SEL,
.selector_mask = GENMASK(3, 0),
.selector_shift = 0,
.window_start = 0,
.window_len = 0x7e,
},
...
Thank you,
Ivan
On Mon, Apr 14, 2025 at 2:40 PM Ivan Vecera <ivecera@redhat.com> wrote:
> On 14. 04. 25 8:36 dop., Andy Shevchenko wrote:
...
> >> 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.
> > The problem is that you overlap virtual page over the real one (the main one).
> >
> > The drivers you mentioned in v2 discussions most likely are also buggy.
> > As I implied in the above question the developers hardly get the regmap ranges
> > right. It took me quite a while to see the issue, so it's not particularly your
> > fault.
> thank you I see the point.
>
> Do you mean that the selector register should not be part of the range?
No.
> If so, does it mean that I have to specify a range for each page? Like this:
No.
Yes, tough thingy :-)
> {
> /* Page 0 */
> .range_min = 0x000,
> .range_max = 0x07e,
> .selector_reg = ZL3073x_PAGE_SEL,
> .selector_mask = GENMASK(3, 0),
> .selector_shift = 0,
> .window_start = 0,
> .window_len = 0x7e,
> },
Page 0 shouldn't exist in your case in the *virtual* ranges. And this
struct defines the *virtual* ranges.
> {
> /* Page 1 */
> .range_min = 0x080,
> .range_max = 0x0fe,
> .selector_reg = ZL3073x_PAGE_SEL,
> .selector_mask = GENMASK(3, 0),
> .selector_shift = 0,
> .window_start = 0,
> .window_len = 0x7e,
> },
--
With Best Regards,
Andy Shevchenko
On 14. 04. 25 1:39 odp., Ivan Vecera wrote:
>
>
> On 14. 04. 25 8:36 dop., Andy Shevchenko wrote:
>>> 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.
>> The problem is that you overlap virtual page over the real one (the
>> main one).
>>
>> The drivers you mentioned in v2 discussions most likely are also buggy.
>> As I implied in the above question the developers hardly get the
>> regmap ranges
>> right. It took me quite a while to see the issue, so it's not
>> particularly your
>> fault.
> Hi Andy,
>
> thank you I see the point.
>
> Do you mean that the selector register should not be part of the range?
>
> If so, does it mean that I have to specify a range for each page? Like
> this:
>
> {
> /* Page 0 */
> .range_min = 0x000,
> .range_max = 0x07e,
> .selector_reg = ZL3073x_PAGE_SEL,
> .selector_mask = GENMASK(3, 0),
> .selector_shift = 0,
> .window_start = 0,
> .window_len = 0x7e,
> },
> {
> /* Page 1 */
> .range_min = 0x080,
> .range_max = 0x0fe,
> .selector_reg = ZL3073x_PAGE_SEL,
> .selector_mask = GENMASK(3, 0),
> .selector_shift = 0,
> .window_start = 0,
> .window_len = 0x7e,
> },
> ...
>
>
> Thank you,
> Ivan
Sorry,
.window_len = 0x7f /* Exclude selector reg */
I.
On 14. 04. 25 1:52 odp., Ivan Vecera wrote:
>
>
> On 14. 04. 25 1:39 odp., Ivan Vecera wrote:
>>
>>
>> On 14. 04. 25 8:36 dop., Andy Shevchenko wrote:
>>>> 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.
>>> The problem is that you overlap virtual page over the real one (the
>>> main one).
>>>
>>> The drivers you mentioned in v2 discussions most likely are also buggy.
>>> As I implied in the above question the developers hardly get the
>>> regmap ranges
>>> right. It took me quite a while to see the issue, so it's not
>>> particularly your
>>> fault.
>> Hi Andy,
>>
>> thank you I see the point.
>>
>> Do you mean that the selector register should not be part of the range?
>>
>> If so, does it mean that I have to specify a range for each page? Like
>> this:
>>
>> {
>> /* Page 0 */
>> .range_min = 0x000,
>> .range_max = 0x07e,
>> .selector_reg = ZL3073x_PAGE_SEL,
>> .selector_mask = GENMASK(3, 0),
>> .selector_shift = 0,
>> .window_start = 0,
>> .window_len = 0x7e,
>> },
>> {
>> /* Page 1 */
>> .range_min = 0x080,
>> .range_max = 0x0fe,
>> .selector_reg = ZL3073x_PAGE_SEL,
>> .selector_mask = GENMASK(3, 0),
>> .selector_shift = 0,
>> .window_start = 0,
>> .window_len = 0x7e,
>> },
>> ...
No, I will answer by myself... this is non-sense.... window_len has to
be 0x80. But I probably know what do you mean...
regmap range should not overlap... so I should use something like:
{
/* original <0x000-0x77f> with offset of 0x100 to move
the range outside of <0x00-0x7f> used by real one */
.range_min = 0x100,
.range_max = 0x87f,
.selector_reg = 0x7f,
.selector_mask = GENMASK(3, 0),
.selector_shift = 0,
.window_start = 0,
.window_len = 0x80,
},
With this I have to modify the driver to use this 0x100 offset. I mean
the datasheet says that register BLAH is at 0x201-0x202. So in the
driver I have to use 0x301-0x302.
Then the _regmap_select_page maps this 0x301 this way:
window_offset = (0x301 - range_min) % window_len;
window_page = (0x301 - range_min) / window len;
thus
window_offset = (0x301 - 0x100) % 0x80 = 0x001
window_page = (0x301 - 0x100) / 0x80 = 4
Long story short, I have to move virtual range outside real address
range and apply this offset in the driver code.
Is this correct?
Thanks,
Ivan
On Mon, Apr 14, 2025 at 5:07 PM Ivan Vecera <ivecera@redhat.com> wrote: > On 14. 04. 25 1:52 odp., Ivan Vecera wrote: ... > Long story short, I have to move virtual range outside real address > range and apply this offset in the driver code. > > Is this correct? Bingo! And for the offsets, you form them as "page number * page offset + offset inside the page". -- With Best Regards, Andy Shevchenko
On Mon, Apr 14, 2025 at 5:10 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Apr 14, 2025 at 5:07 PM Ivan Vecera <ivecera@redhat.com> wrote: > > On 14. 04. 25 1:52 odp., Ivan Vecera wrote: > > ... > > > Long story short, I have to move virtual range outside real address > > range and apply this offset in the driver code. > > > > Is this correct? > > Bingo! > > And for the offsets, you form them as "page number * page offset + > offset inside the page". Note, for easier reference you may still map page 0 to the virtual space, but make sure that page 0 (or main page) is available outside of the ranges, or i.o.w. ranges do not overlap the main page, even if they include page 0. -- With Best Regards, Andy Shevchenko
On Mon, Apr 14, 2025 at 5:13 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Apr 14, 2025 at 5:10 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Mon, Apr 14, 2025 at 5:07 PM Ivan Vecera <ivecera@redhat.com> wrote: > > > On 14. 04. 25 1:52 odp., Ivan Vecera wrote: ... > > > Long story short, I have to move virtual range outside real address > > > range and apply this offset in the driver code. > > > > > > Is this correct? > > > > Bingo! > > > > And for the offsets, you form them as "page number * page offset + > > offset inside the page". > > Note, for easier reference you may still map page 0 to the virtual > space, but make sure that page 0 (or main page) is available outside > of the ranges, or i.o.w. ranges do not overlap the main page, even if > they include page 0. So, you will have the following layout 0x00 - 0xnn - real registers of page 0. 0x100 - 0xppp -- pages 0 ... N Register access either direct for when direct is required, or as 0x100 + PageSize * Index + RegOffset -- With Best Regards, Andy Shevchenko
On 14. 04. 25 4:16 odp., Andy Shevchenko wrote:
> On Mon, Apr 14, 2025 at 5:13 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Mon, Apr 14, 2025 at 5:10 PM Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Mon, Apr 14, 2025 at 5:07 PM Ivan Vecera <ivecera@redhat.com> wrote:
>>>> On 14. 04. 25 1:52 odp., Ivan Vecera wrote:
>
> ...
>
>>>> Long story short, I have to move virtual range outside real address
>>>> range and apply this offset in the driver code.
>>>>
>>>> Is this correct?
>>>
>>> Bingo!
>>>
>>> And for the offsets, you form them as "page number * page offset +
>>> offset inside the page".
>>
>> Note, for easier reference you may still map page 0 to the virtual
>> space, but make sure that page 0 (or main page) is available outside
>> of the ranges, or i.o.w. ranges do not overlap the main page, even if
>> they include page 0.
>
> So, you will have the following layout
>
> 0x00 - 0xnn - real registers of page 0.
>
> 0x100 - 0xppp -- pages 0 ... N
>
> Register access either direct for when direct is required, or as
> 0x100 + PageSize * Index + RegOffset
Now, get it...
I was a little bit confused by code of _regmap_select_page() that takes
care of selector_reg.
Btw, why is this needed? why they cannot overlap?
Let's say I have virtual range <0, 0xfff>, window <0, 0xff> and window
selector 0xff>.
1. I'm calling regmap_read(regmap, 0x8f, ...)
2. The regmap looks for the range and it finds it (0..0xfff)
3. Then it calls _regmap_select_page() that computes:
window_offset = (0x8f - 0x000) % 0x100 = 0x8f
window_page = (0x8f - 0x000) / 0x100 = 0
4. _regmap_select_page() set window selector to 0 and reg is updated to
reg = window_start + window_offset = 0x8f
And for window_selector value: regmap_read(regmap, 0xff, ...) is the
same except _regmap_select_page() checks that the given address is
selector_reg and won't perform page switching.
When I think about it, in my case there is no normal page, there is only
volatile register window <0x00-0x7e> and only single direct register
that is page selector at 0x7f.
Thanks,
Ivan
On Mon, Apr 14, 2025 at 5:53 PM Ivan Vecera <ivecera@redhat.com> wrote: > On 14. 04. 25 4:16 odp., Andy Shevchenko wrote: > > On Mon, Apr 14, 2025 at 5:13 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > >> On Mon, Apr 14, 2025 at 5:10 PM Andy Shevchenko > >> <andy.shevchenko@gmail.com> wrote: > >>> On Mon, Apr 14, 2025 at 5:07 PM Ivan Vecera <ivecera@redhat.com> wrote: > >>>> On 14. 04. 25 1:52 odp., Ivan Vecera wrote: > > > > ... > > > >>>> Long story short, I have to move virtual range outside real address > >>>> range and apply this offset in the driver code. > >>>> > >>>> Is this correct? > >>> > >>> Bingo! > >>> > >>> And for the offsets, you form them as "page number * page offset + > >>> offset inside the page". > >> > >> Note, for easier reference you may still map page 0 to the virtual > >> space, but make sure that page 0 (or main page) is available outside > >> of the ranges, or i.o.w. ranges do not overlap the main page, even if > >> they include page 0. > > > > So, you will have the following layout > > > > 0x00 - 0xnn - real registers of page 0. > > > > 0x100 - 0xppp -- pages 0 ... N > > > > Register access either direct for when direct is required, or as > > 0x100 + PageSize * Index + RegOffset > > Now, get it... > > I was a little bit confused by code of _regmap_select_page() that takes > care of selector_reg. > > Btw, why is this needed? why they cannot overlap? > > Let's say I have virtual range <0, 0xfff>, window <0, 0xff> and window > selector 0xff>. > 1. I'm calling regmap_read(regmap, 0x8f, ...) > 2. The regmap looks for the range and it finds it (0..0xfff) > 3. Then it calls _regmap_select_page() that computes: > window_offset = (0x8f - 0x000) % 0x100 = 0x8f > window_page = (0x8f - 0x000) / 0x100 = 0 > 4. _regmap_select_page() set window selector to 0 and reg is updated to > reg = window_start + window_offset = 0x8f > > And for window_selector value: regmap_read(regmap, 0xff, ...) is the > same except _regmap_select_page() checks that the given address is > selector_reg and won't perform page switching. > > When I think about it, in my case there is no normal page, there is only > volatile register window <0x00-0x7e> and only single direct register > that is page selector at 0x7f. Because you are effectively messing up with cache. Also it's not recommended in general to do such due to some registers that might need to be accessed directly. And also note, that each time you access this you will call a selector write _each_ time you want to write the register in the map (if there is no cache, or if cache is messed up). -- With Best Regards, Andy Shevchenko
On 14. 04. 25 7:09 odp., Andy Shevchenko wrote: >> Now, get it... >> >> I was a little bit confused by code of _regmap_select_page() that takes >> care of selector_reg. >> >> Btw, why is this needed? why they cannot overlap? >> >> Let's say I have virtual range <0, 0xfff>, window <0, 0xff> and window >> selector 0xff>. >> 1. I'm calling regmap_read(regmap, 0x8f, ...) >> 2. The regmap looks for the range and it finds it (0..0xfff) >> 3. Then it calls _regmap_select_page() that computes: >> window_offset = (0x8f - 0x000) % 0x100 = 0x8f >> window_page = (0x8f - 0x000) / 0x100 = 0 >> 4. _regmap_select_page() set window selector to 0 and reg is updated to >> reg = window_start + window_offset = 0x8f >> >> And for window_selector value: regmap_read(regmap, 0xff, ...) is the >> same except _regmap_select_page() checks that the given address is >> selector_reg and won't perform page switching. >> >> When I think about it, in my case there is no normal page, there is only >> volatile register window <0x00-0x7e> and only single direct register >> that is page selector at 0x7f. > Because you are effectively messing up with cache. Also it's not > recommended in general to do such due to some registers that might > need to be accessed directly. And also note, that each time you access > this you will call a selector write_each_ time you want to write the > register in the map (if there is no cache, or if cache is messed up). Get it fully now... Thank you, Andy, for the explanation and for the patience. Will send v3 soon. Ivan
On Mon, Apr 14, 2025 at 2:52 PM Ivan Vecera <ivecera@redhat.com> wrote:
> On 14. 04. 25 1:39 odp., Ivan Vecera wrote:
> > On 14. 04. 25 8:36 dop., Andy Shevchenko wrote:
> >>> 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.
> >> The problem is that you overlap virtual page over the real one (the
> >> main one).
> >>
> >> The drivers you mentioned in v2 discussions most likely are also buggy.
> >> As I implied in the above question the developers hardly get the
> >> regmap ranges
> >> right. It took me quite a while to see the issue, so it's not
> >> particularly your
> >> fault.
> > Hi Andy,
> >
> > thank you I see the point.
> >
> > Do you mean that the selector register should not be part of the range?
> >
> > If so, does it mean that I have to specify a range for each page? Like
> > this:
> >
> > {
> > /* Page 0 */
> > .range_min = 0x000,
> > .range_max = 0x07e,
> > .selector_reg = ZL3073x_PAGE_SEL,
> > .selector_mask = GENMASK(3, 0),
> > .selector_shift = 0,
> > .window_start = 0,
> > .window_len = 0x7e,
> > },
> > {
> > /* Page 1 */
> > .range_min = 0x080,
> > .range_max = 0x0fe,
> > .selector_reg = ZL3073x_PAGE_SEL,
> > .selector_mask = GENMASK(3, 0),
> > .selector_shift = 0,
> > .window_start = 0,
> > .window_len = 0x7e,
> > },
...
> Sorry,
> .window_len = 0x7f /* Exclude selector reg */
It actually will make things worse. If selector register is accessible
to all of the pages, it's better to include it in all pages.
--
With Best Regards,
Andy Shevchenko
On 14. 04. 25 3:55 odp., Andy Shevchenko wrote:
> On Mon, Apr 14, 2025 at 2:52 PM Ivan Vecera <ivecera@redhat.com> wrote:
>> On 14. 04. 25 1:39 odp., Ivan Vecera wrote:
>>> On 14. 04. 25 8:36 dop., Andy Shevchenko wrote:
>>>>> 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.
>>>> The problem is that you overlap virtual page over the real one (the
>>>> main one).
>>>>
>>>> The drivers you mentioned in v2 discussions most likely are also buggy.
>>>> As I implied in the above question the developers hardly get the
>>>> regmap ranges
>>>> right. It took me quite a while to see the issue, so it's not
>>>> particularly your
>>>> fault.
>>> Hi Andy,
>>>
>>> thank you I see the point.
>>>
>>> Do you mean that the selector register should not be part of the range?
>>>
>>> If so, does it mean that I have to specify a range for each page? Like
>>> this:
>>>
>>> {
>>> /* Page 0 */
>>> .range_min = 0x000,
>>> .range_max = 0x07e,
>>> .selector_reg = ZL3073x_PAGE_SEL,
>>> .selector_mask = GENMASK(3, 0),
>>> .selector_shift = 0,
>>> .window_start = 0,
>>> .window_len = 0x7e,
>>> },
>>> {
>>> /* Page 1 */
>>> .range_min = 0x080,
>>> .range_max = 0x0fe,
>>> .selector_reg = ZL3073x_PAGE_SEL,
>>> .selector_mask = GENMASK(3, 0),
>>> .selector_shift = 0,
>>> .window_start = 0,
>>> .window_len = 0x7e,
>>> },
>
> ...
>
>> Sorry,
>> .window_len = 0x7f /* Exclude selector reg */
>
> It actually will make things worse. If selector register is accessible
> to all of the pages, it's better to include it in all pages.
Yes :-) I know this is non-sense.
See my recent reply.
Thanks,
Ivan
On 07/04/2025 19:28, Ivan Vecera wrote:
> This adds base MFD driver for Microchip Azurite ZL3073x chip family.
Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> These chips provide DPLL and PHC (PTP) functionality and they can
> be connected over I2C or SPI bus.
>
...
> +/**
> + * zl3073x_get_regmap_config - return pointer to regmap config
> + *
> + * Returns pointer to regmap config
> + */
> +const struct regmap_config *zl3073x_get_regmap_config(void)
> +{
> + return &zl3073x_regmap_config;
> +}
> +EXPORT_SYMBOL_NS_GPL(zl3073x_get_regmap_config, "ZL3073X");
> +
> +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);
> +
> + 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");
Why do you add empty exports?
> diff --git a/drivers/mfd/zl3073x-spi.c b/drivers/mfd/zl3073x-spi.c
> new file mode 100644
> index 0000000000000..a6b9a366a7585
> --- /dev/null
> +++ b/drivers/mfd/zl3073x-spi.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#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" },
You need bindings. If they are somewhere in this patchset then you need
correct order so before users (see DT submitting patches).
> +static void zl3073x_spi_remove(struct spi_device *spidev)
> +{
> + struct zl3073x_dev *zldev;
> +
> + zldev = spi_get_drvdata(spidev);
> + zl3073x_dev_exit(zldev);
> +}
> +
> +static struct spi_driver zl3073x_spi_driver = {
> + .driver = {
> + .name = "zl3073x-spi",
> + .of_match_table = of_match_ptr(zl3073x_spi_of_match),
Drop of_match_ptr, you have warnings here.
> + },
> + .probe = zl3073x_spi_probe,
> + .remove = zl3073x_spi_remove,
> + .id_table = zl3073x_spi_id,
> +};
> +
Best regards,
Krzysztof
On 07. 04. 25 7:53 odp., Krzysztof Kozlowski wrote:
> On 07/04/2025 19:28, Ivan Vecera wrote:
>> This adds base MFD driver for Microchip Azurite ZL3073x chip family.
>
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
Will fix in v2.
>> These chips provide DPLL and PHC (PTP) functionality and they can
>> be connected over I2C or SPI bus.
>>
>
> ...
>
>> +/**
>> + * zl3073x_get_regmap_config - return pointer to regmap config
>> + *
>> + * Returns pointer to regmap config
>> + */
>> +const struct regmap_config *zl3073x_get_regmap_config(void)
>> +{
>> + return &zl3073x_regmap_config;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(zl3073x_get_regmap_config, "ZL3073X");
>> +
>> +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);
>> +
>> + 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");
>
> Why do you add empty exports?
It is filled in the later commits but yeah I will include the function
once it will be necessary.
>> diff --git a/drivers/mfd/zl3073x-spi.c b/drivers/mfd/zl3073x-spi.c
>> new file mode 100644
>> index 0000000000000..a6b9a366a7585
>> --- /dev/null
>> +++ b/drivers/mfd/zl3073x-spi.c
>> @@ -0,0 +1,71 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#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" },
>
>
> You need bindings. If they are somewhere in this patchset then you need
> correct order so before users (see DT submitting patches).
Yes, there are. I will reorder the patches in v2 and also split the
whole series into several ones.
>> +static void zl3073x_spi_remove(struct spi_device *spidev)
>> +{
>> + struct zl3073x_dev *zldev;
>> +
>> + zldev = spi_get_drvdata(spidev);
>> + zl3073x_dev_exit(zldev);
>> +}
>> +
>> +static struct spi_driver zl3073x_spi_driver = {
>> + .driver = {
>> + .name = "zl3073x-spi",
>> + .of_match_table = of_match_ptr(zl3073x_spi_of_match),
>
> Drop of_match_ptr, you have warnings here.
Ack, will avoid.
>> + },
>> + .probe = zl3073x_spi_probe,
>> + .remove = zl3073x_spi_remove,
>> + .id_table = zl3073x_spi_id,
>> +};
>> +
>
>
>
> Best regards,
> Krzysztof
>
© 2016 - 2026 Red Hat, Inc.