Re: [PATCH v2 3/7] leds: leds-mt6323: Specify registers and specs in platform data

From: Pavel Machek <pavel@ucw.cz>
To: AngeloGioacchino Del Regno  <angelogioacchino.delregno@collabora.com>
Cc: lee@kernel.org, sean.wang@mediatek.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
	linux-leds@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
	kernel@collabora.com
Subject: Re: [PATCH v2 3/7] leds: leds-mt6323: Specify registers and specs in platform data
Date: Thu, 13 Apr 2023 12:57:42 +0200	[thread overview]
Message-ID: <ZDfgJltWqKOVmYx+@duo.ucw.cz> (raw)
In-Reply-To: <20230412153310.241046-4-angelogioacchino.delregno@collabora.com>

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

Hi!

> In order to enhance the flexibility of this driver and let it support
> more than just one MediaTek LEDs IP for more than just one PMIC,
> add platform data structure specifying the register offsets and
> data that commonly varies between different IPs.
> 
> This commit brings no functional changes.

> @@ -63,12 +61,9 @@
>  #define MT6323_ISINK_CH_EN_MASK(i)	BIT(i)
>  #define MT6323_ISINK_CH_EN(i)		BIT(i)
>  
> -#define MT6323_MAX_PERIOD		10000
> -#define MT6323_MAX_LEDS			4
> -#define MT6323_MAX_BRIGHTNESS		6
> -#define MT6323_UNIT_DUTY		3125
> -#define MT6323_CAL_HW_DUTY(o, p)	DIV_ROUND_CLOSEST((o) * 100000ul,\
> -					(p) * MT6323_UNIT_DUTY)
> +#define MAX_SUPPORTED_LEDS		8
> +#define MT6323_CAL_HW_DUTY(o, p, u)	DIV_ROUND_CLOSEST((o) * 100000ul,\
> +					(p) * (u))
>

You increase number of supported leds from 4 to 8. That's ok, but I'd
not call it "no functional changes".

> +/**
> + * struct mt6323_regs - register spec for the LED device
> + * @top_ckpdn:		Offset to ISINK_CKPDN[0..x] registers
> + * @num_top_ckpdn:	Number of ISINK_CKPDN registers
> + * @top_ckcon:		Offset to ISINK_CKCON[0..x] registers
> + * @num_top_ckcon:	Number of ISINK_CKCON registers
> + * @isink_con:		Offset to ISINKx_CON[0..x] registers
> + * @num_isink_con:	Number of ISINKx_CON registers
> + * @isink_max_regs:	Number of ISINK[0..x] registers
> + * @isink_en_ctrl:	Offset to ISINK_EN_CTRL register
> + */
> +struct mt6323_regs {
> +	const u16 *top_ckpdn;
> +	u8 num_top_ckpdn;
> +	const u16 *top_ckcon;
> +	u8 num_top_ckcon;
> +	const u16 *isink_con;
> +	u8 num_isink_con;
> +	u8 isink_max_regs;
> +	u16 isink_en_ctrl;
> +};

I'd use ints here. Should get similar memory usage and maybe less code
size. But ... no need to resubmit just for this.

> @@ -469,8 +525,31 @@ static int mt6323_led_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct mt6323_regs mt6323_registers = {
> +	.top_ckpdn = (const u16[]){ 0x102, 0x106, 0x10e },
> +	.num_top_ckpdn = 3,
> +	.top_ckcon = (const u16[]){ 0x120, 0x126 },
> +	.num_top_ckcon = 2,
> +	.isink_con = (const u16[]){ 0x330, 0x332, 0x334 },
> +	.num_isink_con = 3,
> +	.isink_max_regs = 4, /* ISINK[0..3] */
> +	.isink_en_ctrl = 0x356,
> +};
> +
> +static const struct mt6323_hwspec mt6323_spec = {
> +	.max_period = 10000,
> +	.max_leds = 4,
> +	.max_brightness = 6,
> +	.unit_duty = 3125,
> +};
> +
> +static const struct mt6323_data mt6323_pdata = {
> +	.regs = &mt6323_registers,
> +	.spec = &mt6323_spec,
> +};
> +

Acked-by: Pavel Machek <pavel@ucw.cz>

BR,
								Pavel

-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]