linux-kernel - Re: [PATCH v2 1/2] leds: flash: add driver to support flash LED module in QCOM PMICs

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] [day] [month] [year] [list]

Date:   Mon, 10 Oct 2022 06:20:01 -0400
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Fenglin Wu <quic_fenglinw@...cinc.com>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Pavel Machek <pavel@....cz>, Gene Chen <gene_chen@...htek.com>,
        Jacek Anaszewski <jacek.anaszewski@...il.com>,
        linux-leds@...r.kernel.org
Cc:     collinsd@...eaurora.org, subbaram@...eaurora.org
Subject: Re: [PATCH v2 1/2] leds: flash: add driver to support flash LED
 module in QCOM PMICs

On 10/10/2022 06:00, Fenglin Wu wrote:
>>> +
>>> +static const struct qcom_flash_reg mvflash_3ch_reg = {
>>> +	.chan_timer	= 0x40,
>>> +	.itarget	= 0x43,
>>> +	.module_en	= 0x46,
>>> +	.iresolution	= 0x47,
>>> +	.chan_strobe	= 0x49,
>>> +	.chan_en	= 0x4c,
>>> +	.status1	= 0x08,
>>> +	.status2	= 0x09,
>>> +	.status3	= 0x0a,
>>> +};
>>> +
>>> +static const struct qcom_flash_reg mvflash_4ch_reg = {
>>> +	.chan_timer	= 0x3e,
>>> +	.itarget	= 0x42,
>>> +	.module_en	= 0x46,
>>> +	.iresolution	= 0x49,
>>> +	.chan_strobe	= 0x4a,
>>> +	.chan_en	= 0x4e,
>>> +	.status1	= 0x06,
>>> +	.status2	= 0x07,
>>> +	.status3	= 0x09,
>>
>> Don't reinvent the wheel. Use regmap fields.
>>
> 
> Do you mean defining regmap_filed pointer in struct qcom_flash_chip
> for all these registers instead of creating the data structure 
> qcom_flash_reg to include all the registers? Similar like this
> 
> struct qcom_flash_chip {
> 	....
>          struct regmap_field     *chan_timer;
>          struct regmap_field     *chan_timer;
>          struct regmap_field     *itarget;
>          struct regmap_field     *iresolution;
>          struct regmap_field     *chan_strobe;
>          struct regmap_field     *chan_en;
>          struct regmap_field     *status1;
>          struct regmap_field     *status2;
>          struct regmap_field     *status3;
> 	....
> };

This is one way, other is with allocated array and indexing by some enum.

> 
> This won't make the code cleaner actually. If this is the preferred way, 
> I will update it accordingly.

A bit it will, because regmap fields will replace most of your shifts
and masks. The point is rather not doing something by your own, but
using frameworks which are exactly for this purpose.



Best regards,
Krzysztof