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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZDfgJltWqKOVmYx+@duo.ucw.cz>
Date:   Thu, 13 Apr 2023 12:57:42 +0200
From:   Pavel Machek <pavel@....cz>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
Cc:     lee@...nel.org, sean.wang@...iatek.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, matthias.bgg@...il.com,
        linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        kernel@...labora.com
Subject: Re: [PATCH v2 3/7] leds: leds-mt6323: Specify registers and specs in
 platform data

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@....cz>

BR,
								Pavel

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

Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)