Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Using PLATFORM_DEVID_NONE instead of PLATFORM_DEVID_AUTO causes sysfs device naming collisions.
- [Low] Dynamic creation of a local stack copy of the mfd_cell array for filtering violates MFD architectural rules.
--
commit 5b7692c955e218224dd53b696f2b08c851828b93
Author: Jernej Skrabec <jernej.skrabec@gmail.com>
mfd: ac200: Add codec and TV encoder cells
Register the AC200 audio codec and TV encoder as MFD cells when their
enabled child nodes are present. Filtering the cell list avoids creating
unused platform devices.
> diff --git a/drivers/mfd/ac200.c b/drivers/mfd/ac200.c
> index 94aa13deb0f8c..f7d818141d9cb 100644
> --- a/drivers/mfd/ac200.c
> +++ b/drivers/mfd/ac200.c
[ ... ]
> +static int ac200_add_devices(struct device *dev)
> +{
> + struct mfd_cell cells[ARRAY_SIZE(ac200_cells)];
> + unsigned int num_cells = 0;
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(ac200_cells); i++) {
> + const struct mfd_cell *cell = &ac200_cells[i];
> + struct device_node *child;
> +
> + child = of_get_compatible_child(dev->of_node,
> + cell->of_compatible);
> + if (!child)
> + continue;
> + if (of_device_is_available(child))
> + cells[num_cells++] = *cell;
[Severity: Low]
Does creating a dynamic local stack copy of the mfd_cell array here
violate the MFD architectural guidelines?
The MFD subsystem rules typically require cell arrays to remain static const.
Dynamically amending and explicitly copying into a local array during probe
in ac200_add_devices() bypasses the standard static definition approach.
Could this instead use static references or a switch statement if variants
or filtering are needed?
> + of_node_put(child);
> + }
> +
> + if (!num_cells)
> + return 0;
> +
> + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, cells, num_cells,
> + NULL, 0, NULL);
[Severity: Medium]
Can passing PLATFORM_DEVID_NONE here cause sysfs device naming collisions?
If a system developer instantiates multiple AC200 chips in the device tree,
the second instance might fail during devm_mfd_add_devices() because both
instances will attempt to register child platform devices with identical
hard-coded names (like 'ac200-codec').
Would it be safer to use PLATFORM_DEVID_AUTO to allow automatic cell
indexing and properly support multi-instance configurations?
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to add function devices\n");
> +
> + return 0;
> +}
@@ -210,12 +210,15 @@ config MFD_AC200
depends on COMMON_CLK
depends on I2C
depends on OF
+ select MFD_CORE
select REGMAP_I2C
+ select REGMAP_IRQ
help
Support for the X-Powers AC200 mixed-signal companion IC. The AC200
contains audio, video, RTC and Fast Ethernet PHY functions and is
co-packaged with some Allwinner H6 and H616 SoCs. This driver provides
- the shared register access used by the individual function drivers.
+ the shared register access and instantiates the individual function
+ devices.
config MFD_AXP20X
tristate
@@ -9,11 +9,24 @@
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/mfd/core.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/regmap.h>
+#include <dt-bindings/mfd/x-powers,ac200.h>
+
#define AC200_SYS_CONTROL_REG 0x0002
#define AC200_SYS_CONTROL_CHIP_RESET_DEASSERT BIT(0)
+#define AC200_SYS_IRQ_ENABLE_REG 0x0004
+#define AC200_SYS_IRQ_INTB_ENABLE BIT(15)
+#define AC200_SYS_IRQ_INTB_ACTIVE_HIGH BIT(14)
+#define AC200_SYS_IRQ_RTC BIT(12)
+#define AC200_SYS_IRQ_EPHY BIT(8)
+#define AC200_SYS_IRQ_TVE BIT(4)
+#define AC200_SYS_IRQ_STATUS_REG 0x0006
/* Interface register accessible from every register page. */
#define AC200_TWI_REG_ADDR_H 0x00fe
@@ -51,6 +64,103 @@ static const struct regmap_config ac200_regmap_config = {
.cache_type = REGCACHE_MAPLE,
};
+static const struct regmap_irq ac200_irqs[] = {
+ REGMAP_IRQ_REG(AC200_IRQ_TVE, 0, AC200_SYS_IRQ_TVE),
+ REGMAP_IRQ_REG(AC200_IRQ_EPHY, 0, AC200_SYS_IRQ_EPHY),
+ REGMAP_IRQ_REG(AC200_IRQ_RTC, 0, AC200_SYS_IRQ_RTC),
+};
+
+/*
+ * SYS_IRQ_ENABLE is an enable register rather than a mask register, hence
+ * unmask_base. SYS_IRQ_STATUS reflects the source levels, so the function
+ * which raised an interrupt is responsible for clearing it.
+ */
+static const struct regmap_irq_chip ac200_irq_chip = {
+ .name = "ac200",
+ .status_base = AC200_SYS_IRQ_STATUS_REG,
+ .unmask_base = AC200_SYS_IRQ_ENABLE_REG,
+ .num_regs = 1,
+ .irqs = ac200_irqs,
+ .num_irqs = ARRAY_SIZE(ac200_irqs),
+};
+
+static const struct mfd_cell ac200_cells[] = {
+ {
+ .name = "ac200-codec",
+ .of_compatible = "x-powers,ac200-codec",
+ }, {
+ .name = "ac200-tve",
+ .of_compatible = "x-powers,ac200-tve",
+ },
+};
+
+static int ac200_init_irq(struct device *dev, struct regmap *regmap, int irq)
+{
+ struct regmap_irq_chip_data *irq_data;
+ unsigned int trigger;
+ u16 value = AC200_SYS_IRQ_INTB_ENABLE;
+ int ret;
+
+ trigger = irq_get_trigger_type(irq);
+ switch (trigger) {
+ case IRQ_TYPE_LEVEL_HIGH:
+ value |= AC200_SYS_IRQ_INTB_ACTIVE_HIGH;
+ break;
+ case IRQ_TYPE_NONE:
+ case IRQ_TYPE_LEVEL_LOW:
+ break;
+ default:
+ return dev_err_probe(dev, -EINVAL,
+ "INTB is level triggered, not type %u\n",
+ trigger);
+ }
+
+ ret = regmap_update_bits(regmap, AC200_SYS_IRQ_ENABLE_REG,
+ AC200_SYS_IRQ_INTB_ENABLE |
+ AC200_SYS_IRQ_INTB_ACTIVE_HIGH, value);
+ if (ret)
+ return ret;
+
+ ret = devm_regmap_add_irq_chip(dev, regmap, irq, IRQF_ONESHOT, 0,
+ &ac200_irq_chip, &irq_data);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to add IRQ chip\n");
+
+ return 0;
+}
+
+static int ac200_add_devices(struct device *dev)
+{
+ struct mfd_cell cells[ARRAY_SIZE(ac200_cells)];
+ unsigned int num_cells = 0;
+ unsigned int i;
+ int ret;
+
+ for (i = 0; i < ARRAY_SIZE(ac200_cells); i++) {
+ const struct mfd_cell *cell = &ac200_cells[i];
+ struct device_node *child;
+
+ child = of_get_compatible_child(dev->of_node,
+ cell->of_compatible);
+ if (!child)
+ continue;
+ if (of_device_is_available(child))
+ cells[num_cells++] = *cell;
+ of_node_put(child);
+ }
+
+ if (!num_cells)
+ return 0;
+
+ ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, cells, num_cells,
+ NULL, 0, NULL);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "failed to add function devices\n");
+
+ return 0;
+}
+
static int ac200_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
@@ -83,7 +193,13 @@ static int ac200_probe(struct i2c_client *client)
if (ret)
return ret;
- return 0;
+ if (client->irq > 0) {
+ ret = ac200_init_irq(dev, regmap, client->irq);
+ if (ret)
+ return ret;
+ }
+
+ return ac200_add_devices(dev);
}
static const struct of_device_id ac200_of_match[] = {
@@ -108,6 +224,7 @@ static struct i2c_driver ac200_driver = {
};
module_i2c_driver(ac200_driver);
+MODULE_AUTHOR("Jernej Skrabec <jernej.skrabec@gmail.com>");
MODULE_AUTHOR("James Hilliard <james.hilliard1@gmail.com>");
MODULE_DESCRIPTION("X-Powers AC200 MFD core driver");
MODULE_LICENSE("GPL");