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

[Krzysztof Kozlowski]

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