Re: [PATCH v2] iio: magnetometer: ak8974: Fix runtime PM imbalance on error in ak8974_probe()

Re: [PATCH v2] iio: magnetometer: ak8974: Fix runtime PM imbalance on error in ak8974_probe()

[Markus Elfring]

> When devm_regmap_init_i2c() returns an error code, a pairing
> runtime PM usage counter decrement is needed to keep the
> counter balanced.

How do you think about to replace the word “pairing” by “corresponding”?


> For error paths after ak8974_set_power(),
> ak8974_detect() and ak8974_reset(), things are the same.

Will an other wording become more helpful than this information?


> However, When iio_triggered_buffer_setup() returns an error
> code, there will be two PM usgae counter decrements.

Please avoid two typos in this sentence.


Would you like to add the tag “Fixes” to the commit message?


…
+++ b/drivers/iio/magnetometer/ak8974.c
…
@@ -854,7 +856,6 @@  static int ak8974_probe(struct i2c_client *i2c,
…

Can a source code variant like the following make sense
for a more complete exception handling?

power_off:
	ak8974_set_power(ak8974, AK8974_PWR_OFF);
put_pm:
	pm_runtime_put_noidle(&i2c->dev);
	pm_runtime_disable(&i2c->dev);
disable_regulator:
	regulator_bulk_disable(ARRAY_SIZE(ak8974->regs), ak8974->regs);
	return ret;


Regards,
Markus

Re: [PATCH v2] iio: magnetometer: ak8974: Fix runtime PM imbalance on error in ak8974_probe()

[Markus Elfring]

https://lkml.org/lkml/2020/6/14/117
https://lore.kernel.org/patchwork/comment/1452516/
https://lore.kernel.org/linux-iio/20200614131116.143bcf92@archlinux/

> There was a lot of fuzz in this one due to other changes to the driver.
> I 'think' it went it cleanly though.

Is it interesting anyhow that you committed a change description
which contained typos (on 2020-06-14 13:08:47 +0100)?
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/drivers/iio/magnetometer/ak8974.c?h=fixes-togreg&id=0187294d227dfc42889e1da8f8ce1e44fc25f147

How were the chances that my patch review comments would be taken
better into account?
https://lore.kernel.org/linux-iio/dd84c12f-277d-27e7-3727-4592e530e4ed@web.de/
https://lkml.org/lkml/2020/5/31/152

Regards,
Markus

Re: [PATCH v2] iio: magnetometer: ak8974: Fix runtime PM imbalance on error in ak8974_probe()

[Jonathan Cameron]

On Thu, 25 Jun 2020 14:34:44 +0200
Markus Elfring <Markus.Elfring@web.de> wrote:

> https://lkml.org/lkml/2020/6/14/117
> https://lore.kernel.org/patchwork/comment/1452516/
> https://lore.kernel.org/linux-iio/20200614131116.143bcf92@archlinux/
> 
> > There was a lot of fuzz in this one due to other changes to the driver.
> > I 'think' it went it cleanly though.  
> 
> Is it interesting anyhow that you committed a change description
> which contained typos (on 2020-06-14 13:08:47 +0100)?
> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/drivers/iio/magnetometer/ak8974.c?h=fixes-togreg&id=0187294d227dfc42889e1da8f8ce1e44fc25f147
> 
> How were the chances that my patch review comments would be taken
> better into account?
> https://lore.kernel.org/linux-iio/dd84c12f-277d-27e7-3727-4592e530e4ed@web.de/
> https://lkml.org/lkml/2020/5/31/152

I'm not sure why, but your reply did not have a reply-to field in the header
as such my email client did not present it alongside the patch. Hence I
missed it when applying.

Agreed it would have been nicer to have fixed those typos.
However, they don't affect comprehensibility of the message so I'm not that
worried about having them in the log.

Jonathan



> 
> Regards,
> Markus

Re: [v2] iio: magnetometer: ak8974: Fix runtime PM imbalance on error in ak8974_probe()

[Markus Elfring]

>> How were the chances that my patch review comments would be taken
>> better into account?
>> https://lore.kernel.org/linux-iio/dd84c12f-277d-27e7-3727-4592e530e4ed@web.de/
>> https://lkml.org/lkml/2020/5/31/152
>
> I'm not sure why, but your reply did not have a reply-to field in the header
> as such my email client did not present it alongside the patch.

There are some factors involved for this undesirable effect.

Example:
My software selection contains open issues in the handling of mailto links
according to the communication interface “public inbox”.


> Hence I missed it when applying.

Can my approach for a patch review reminder get more attention?


> Agreed it would have been nicer to have fixed those typos.

Thanks for this positive feedback.


> However, they don't affect comprehensibility of the message
> so I'm not that worried about having them in the log.

Can you get other concerns around the quality of commit messages?


Will it become more interesting to take another look at jump targets
for the exception handling?

Regards,
Markus