On 11/15/21 1:11 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@xxxxxxxxxx>
>
> It's a new controller used on BCM4908, some BCM68xx and some BCM63xxx
> SoCs.
>
> Signed-off-by: Rafał Miłecki <rafal@xxxxxxxxxx>
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
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
