On 11/15/21 1:11 AM, Rafał Miłecki wrote:

From: Rafał Miłecki <rafal@milecki.pl>

It's a new controller used on BCM4908, some BCM68xx and some BCM63xxx
SoCs.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Same comment as the binding, please s/bcm63xxx/bcm63xx/ for matchign
existing drivers/patterns.

[snip]

+
+#define BCM63XXX_MAX_LEDS			32> +
+#define BCM63XXX_GLB_CTRL			0x00
+#define BCM63XXX_MASK				0x04

This define appears unused.

+#define BCM63XXX_HW_LED_EN			0x08
+#define BCM63XXX_SERIAL_LED_SHIFT_SEL		0x0c
+#define BCM63XXX_FLASH_RATE_CTRL1		0x10
+#define BCM63XXX_FLASH_RATE_CTRL2		0x14
+#define BCM63XXX_FLASH_RATE_CTRL3		0x18
+#define BCM63XXX_FLASH_RATE_CTRL4		0x1c
+#define BCM63XXX_BRIGHT_CTRL1			0x20
+#define BCM63XXX_BRIGHT_CTRL2			0x24
+#define BCM63XXX_BRIGHT_CTRL3			0x28
+#define BCM63XXX_BRIGHT_CTRL4			0x2c
+#define BCM63XXX_POWER_LED_CFG			0x30
+#define BCM63XXX_HW_POLARITY			0xb4
+#define BCM63XXX_SW_DATA			0xb8

This is called SW_LED_IP in the register but I guess this name is a bit
clearer.

+#define BCM63XXX_SW_POLARITY			0xbc
+#define BCM63XXX_PARALLEL_LED_POLARITY		0xc0
+#define BCM63XXX_SERIAL_LED_POLARITY		0xc4
+#define BCM63XXX_HW_LED_STATUS			0xc8
+#define BCM63XXX_FLASH_CTRL_STATUS		0xcc
+#define BCM63XXX_FLASH_BRT_CTRL			0xd0
+#define BCM63XXX_FLASH_P_LED_OUT_STATUS		0xd4
+#define BCM63XXX_FLASH_S_LED_OUT_STATUS		0xd8
+
+struct bcm63xxx_leds {
+	struct device *dev;
+	void __iomem *base;
+	spinlock_t lock;
+};
+
+struct bcm63xxx_led {
+	struct bcm63xxx_leds *leds;
+	struct led_classdev cdev;
+	u32 pin;
+	bool active_low;
+};
+
+/*
+ * I/O access
+ */
+
+static void bcm63xxx_leds_write(struct bcm63xxx_leds *leds, unsigned int reg,
+				u32 data)
+{
+	writel(data, leds->base + reg);
+}
+
+static unsigned long bcm63xxx_leds_read(struct bcm63xxx_leds *leds,
+					unsigned int reg)
+{
+	return readl(leds->base + reg);
+}
+
+static void bcm63xxx_leds_update_bits(struct bcm63xxx_leds *leds,
+				      unsigned int reg, u32 mask, u32 val)
+{
+	WARN_ON(val & ~mask);
+
+	bcm63xxx_leds_write(leds, reg, (bcm63xxx_leds_read(leds, reg) & ~mask) | (val & mask));
+}
+
+/*
+ * Helpers
+ */
+
+static void bcm63xxx_leds_set_flash_rate(struct bcm63xxx_leds *leds,
+					 struct bcm63xxx_led *led,
+					 u8 value)
+{
+	int reg_offset = (led->pin >> 3) * 4;

Maybe add some definitions here, like LEDS_PER_WORD and LED_SHIFT and
LED_MASK?

[snip]

+static int bcm63xxx_leds_create_led(struct bcm63xxx_leds *leds, struct device_node *np)
+{

You are not checking the return value of this function, make it void?

[snip]

+static int bcm63xxx_leds_probe(struct platform_device *pdev)
+{
+	struct device_node *np = dev_of_node(&pdev->dev);
+	struct device *dev = &pdev->dev;
+	struct bcm63xxx_leds *leds;
+	struct device_node *child;
+
+	leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL);
+	if (!leds)
+		return -ENOMEM;
+
+	leds->dev = dev;
+
+	leds->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(leds->base))
+		return PTR_ERR(leds->base);
+
+	spin_lock_init(&leds->lock);
+
+	bcm63xxx_leds_write(leds, BCM63XXX_GLB_CTRL, 0xa);

We would need a define for that:

0x2 -> SERIAL_LED_DATA_PPOL
0x8 -> SERIAL_LED_EN_POL

+	bcm63xxx_leds_write(leds, BCM63XXX_BRIGHT_CTRL1, 0x88888888);

Cannot we let the LED subsystem change the default brightness?
-- 
Florian