Re: [PATCH v2 07/14] iio: accel: bma220: reset registers during init stage

[Krzysztof Kozlowski <krzk@kernel.org>]

On 11/09/2025 14:36, Petre Rodan wrote:
> 
> Hi Krzysztof,
> 
> On Thu, Sep 11, 2025 at 09:35:52AM +0200, Krzysztof Kozlowski wrote:
>> On 10/09/2025 09:57, Petre Rodan wrote:
>>> Bring all configuration registers to default values during device probe().
>>>
>>> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
>>> ---
>>>  drivers/iio/accel/bma220_core.c | 71 ++++++++++++++++++++++++++++-------------
>>>  1 file changed, 49 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/bma220_core.c b/drivers/iio/accel/bma220_core.c
>>> index b6f1374a9cca52966c1055113710061a7284cf5a..322df516c90a7c645eeca579cae9803eb31caad1 100644
>>> --- a/drivers/iio/accel/bma220_core.c
>>> -static int bma220_init(struct spi_device *spi)
>>> +static int bma220_reset(struct spi_device *spi, bool up)
>>>  {
>>> -	int ret;
>>> -	static const char * const regulator_names[] = { "vddd", "vddio", "vdda" };
>>> +	int i, ret;
>>>  
>>> -	ret = devm_regulator_bulk_get_enable(&spi->dev,
>>
>>
>> You just added this code in patch 6. Don't add code which immediately
>> you remove. I understand you re-add this later, so basically it is a
>> move, but such patch diff is still confusing.
> 
> sorry, but this is an artefact of 'git diff' I don't think I have no control of.


Don't think so. Before bma220_init() was above bma220_power(). After
your patch bma220_init() is BELOW bma220_power(), so that's a move.



> 
> the bma220_reset() function was added to bma220_core.c with this patch and the
> diff process merged lines from this new function with lines from bma220_init()
> causing the apparent removal of the lines added in the previous patch.
> if you look a few lines below your cut, the bma220_init() function contains the
> code:
> 
> +static int bma220_init(struct spi_device *spi)
> +{
> +	int ret;
> +	static const char * const regulator_names[] = { "vddd", "vddio", "vdda" };
> +
> +	ret = devm_regulator_bulk_get_enable(&spi->dev,
> +					     ARRAY_SIZE(regulator_names),
> +					     regulator_names);
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret, "Failed to get regulators\n");
> [..]
> 
> Just for my curiosity, do reviewers apply the patches one by one to (a branch of)
> the tree itself or do they provide feedback directly based on the diffs?


Each commit must stand on its own, is reviewed independently and entire
patchset must be 100% bisectable.


Best regards,
Krzysztof