Re: [PATCH 2/2] leds: bcm63xxx: add support for BCM63xxx controller

[Florian Fainelli <florian.fainelli@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 3532 bytes --]

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]