Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <412c5d22-d59b-9191-80dd-e3ca11360bc4@gmail.com>
Date:   Sat, 20 Aug 2022 16:38:17 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Content-Language: en-US
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
        =?UTF-8?Q?Nuno_S=c3=a1?= <nuno.sa@analog.com>,
        Lars-Peter Clausen <lars@metafoo.de>,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
References: <cover.1660934107.git.mazziesaccount@gmail.com>
 <a29493f594c84b3bd852e462bbd3e591a8575a27.1660934107.git.mazziesaccount@gmail.com>
 <20220820122120.57dddcab@jic23-huawei>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Subject: Re: [PATCH v3 07/14] iio: ltc2688: Simplify using
 devm_regulator_*get_enable()
In-Reply-To: <20220820122120.57dddcab@jic23-huawei>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 8/20/22 14:21, Jonathan Cameron wrote:
> On Fri, 19 Aug 2022 22:19:17 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Use devm_regulator_bulk_get_enable() instead of open coded bulk-get,
>> bulk-enable, add-action-to-disable-at-detach - pattern.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> ---
>> v2 => v3
>> Split to own patch.
>> ---
>>   drivers/iio/dac/ltc2688.c | 23 +++--------------------
>>   1 file changed, 3 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/iio/dac/ltc2688.c b/drivers/iio/dac/ltc2688.c
>> index 28bdde2d3088..fcad3efe62ea 100644
>> --- a/drivers/iio/dac/ltc2688.c
>> +++ b/drivers/iio/dac/ltc2688.c
>> @@ -84,7 +84,6 @@ struct ltc2688_chan {
>>   struct ltc2688_state {
>>   	struct spi_device *spi;
>>   	struct regmap *regmap;
>> -	struct regulator_bulk_data regulators[2];
>>   	struct ltc2688_chan channels[LTC2688_DAC_CHANNELS];
>>   	struct iio_chan_spec *iio_chan;
>>   	/* lock to protect against multiple access to the device and shared data */
>> @@ -902,13 +901,6 @@ static int ltc2688_setup(struct ltc2688_state *st, struct regulator *vref)
>>   			       LTC2688_CONFIG_EXT_REF);
>>   }
>>   
>> -static void ltc2688_disable_regulators(void *data)
>> -{
>> -	struct ltc2688_state *st = data;
>> -
>> -	regulator_bulk_disable(ARRAY_SIZE(st->regulators), st->regulators);
>> -}
>> -
>>   static void ltc2688_disable_regulator(void *regulator)
>>   {
>>   	regulator_disable(regulator);
>> @@ -970,6 +962,7 @@ static int ltc2688_probe(struct spi_device *spi)
>>   	struct regulator *vref_reg;
>>   	struct device *dev = &spi->dev;
>>   	int ret;
>> +	static const char * const regulators[] = {"vcc", "iovcc"};
> trivial - slight preference for
>   { "vcc", "iovcc" };
> 
> This isn't as important as for numeric values as we get some readability
> from the quotes but still nice to have.

Right. I'll fix it.

> For the whole static / vs non static. My personal preference is not
> to have the static marking but I don't care that much.
> 

I'd like to stick with the static here. I know this one particular array 
does not have much of a footprint - but I'd like to encourage the habit 
of considering the memory usage. This discussion serves as an example of 
how unknown the impact of making const data static is. I didn't know 
this myself until Sebastian educated me :)  Hence my strong preference 
on keeping this 'static' as an example for others who are as ignorant as 
I were ;) After all, having const data arrays static is quite an easy 
way of improving things - and it really does matter when there is many 
of arrays - or when they contain large data.

Yours
-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~