[PATCH v5 0/1] iio:pressure:ms5611: fix and enhancements

[PATCH v5 0/1] iio:pressure:ms5611: fix and enhancements

[Gregor Boirie]

Changes since v1:
* temperature compensation fix:
    * reword commit message
    * remove useless parenthesis
* power regulator: reword commit message
* add oversampling rate support

Changes since v2:
* remove non printable characters from commit messages

Changes since v3:
* drop first 3 patches since applied onto testing
* fix oops at regulator probing time if regulator support not compiled-in
* complete device-tree support and documentation
* remove continuous sampling support (see Jonathan's "kthread tight loop
  trigger" proposal)
* checkpatch'ed

Changes since v4:
* drop first 2 patches since applied
* amend commit to include "Fixes" tag

Gregor Boirie (1):
  Fixes: 3145229f9191 ("iio:pressure:ms5611: power regulator support")

 drivers/iio/pressure/ms5611_core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

-- 
2.1.4

[PATCH v5 1/1] Fixes: 3145229f9191 ("iio:pressure:ms5611: power regulator support")

[Gregor Boirie]

When not compiled-in, regulator layer will return a NULL pointer when
trying to get a reference to any regulator using devm_regulator_get().  As
IS_ERR() does not consider this an error, the ms5611 probing operation will
try to enable a NULL regulator, which will invariably cause a kernel
crash.
This patch fixes this situation by using devm_regulator_get_optional()
instead of devm_regulator_get().

Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
---
 drivers/iio/pressure/ms5611_core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/pressure/ms5611_core.c b/drivers/iio/pressure/ms5611_core.c
index 37dbc04..a2a871b 100644
--- a/drivers/iio/pressure/ms5611_core.c
+++ b/drivers/iio/pressure/ms5611_core.c
@@ -387,8 +387,8 @@ static const struct iio_info ms5611_info = {
 static int ms5611_init(struct iio_dev *indio_dev)
 {
 	int ret;
-	struct regulator *vdd = devm_regulator_get(indio_dev->dev.parent,
-	                                           "vdd");
+	struct regulator *vdd =
+		devm_regulator_get_optional(indio_dev->dev.parent, "vdd");
 
 	/* Enable attached regulator if any. */
 	if (!IS_ERR(vdd)) {
@@ -398,6 +398,10 @@ static int ms5611_init(struct iio_dev *indio_dev)
 			        "failed to enable Vdd supply: %d\n", ret);
 			return ret;
 		}
+	} else {
+		ret = PTR_ERR(vdd);
+		if (ret != -ENODEV)
+			return ret;
 	}
 
 	ret = ms5611_reset(indio_dev);
-- 
2.1.4

Re: [PATCH v5 1/1] Fixes: 3145229f9191 ("iio:pressure:ms5611: power regulator support")

[Daniel Baluta]

On Tue, Mar 8, 2016 at 8:00 PM, Gregor Boirie <gregor.boirie@parrot.com> wrote:
> When not compiled-in, regulator layer will return a NULL pointer when
> trying to get a reference to any regulator using devm_regulator_get().  As
> IS_ERR() does not consider this an error, the ms5611 probing operation will
> try to enable a NULL regulator, which will invariably cause a kernel
> crash.
> This patch fixes this situation by using devm_regulator_get_optional()
> instead of devm_regulator_get().
>
> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>

$Subject should be here. And the commit subject should be something
more appropriate like:

iio: pressure: ms5611: Fix crash when ...


> ---
>  drivers/iio/pressure/ms5611_core.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/pressure/ms5611_core.c b/drivers/iio/pressure/ms5611_core.c
> index 37dbc04..a2a871b 100644
> --- a/drivers/iio/pressure/ms5611_core.c
> +++ b/drivers/iio/pressure/ms5611_core.c
> @@ -387,8 +387,8 @@ static const struct iio_info ms5611_info = {
>  static int ms5611_init(struct iio_dev *indio_dev)
>  {
>         int ret;
> -       struct regulator *vdd = devm_regulator_get(indio_dev->dev.parent,
> -                                                  "vdd");
> +       struct regulator *vdd =
> +               devm_regulator_get_optional(indio_dev->dev.parent, "vdd");
>
>         /* Enable attached regulator if any. */
>         if (!IS_ERR(vdd)) {
> @@ -398,6 +398,10 @@ static int ms5611_init(struct iio_dev *indio_dev)
>                                 "failed to enable Vdd supply: %d\n", ret);
>                         return ret;
>                 }
> +       } else {
> +               ret = PTR_ERR(vdd);
> +               if (ret != -ENODEV)
> +                       return ret;
>         }
>
>         ret = ms5611_reset(indio_dev);
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/1] Fixes: 3145229f9191 ("iio:pressure:ms5611: power regulator support")

[Gregor Boirie]

Hi Daniel,

On 03/08/2016 10:16 PM, Daniel Baluta wrote:
> On Tue, Mar 8, 2016 at 8:00 PM, Gregor Boirie <gregor.boirie@parrot.com> wrote:
>> When not compiled-in, regulator layer will return a NULL pointer when
>> trying to get a reference to any regulator using devm_regulator_get().  As
>> IS_ERR() does not consider this an error, the ms5611 probing operation will
>> try to enable a NULL regulator, which will invariably cause a kernel
>> crash.
>> This patch fixes this situation by using devm_regulator_get_optional()
>> instead of devm_regulator_get().
>>
>> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
> $Subject should be here. And the commit subject should be something
> more appropriate like:
>
> iio: pressure: ms5611: Fix crash when ...
I'm a bit lost here since when Jonathan asked for "a fixes tag" inclusion
(http://permalink.gmane.org/gmane.linux.kernel.iio/22425), I supposed he
meant a tag such as one described in the Documentation/SubmittingPatches
file.
My initial attempt was sent with the subject:
"iio:pressure:ms5611: fix oops when probing regulator".

Would you please be kind enough to give me some sort of rules as to when 
and how
to write subject lines related to fixes patch ?

Regards,
Grégor.

>
>
>> ---
>>   drivers/iio/pressure/ms5611_core.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/pressure/ms5611_core.c b/drivers/iio/pressure/ms5611_core.c
>> index 37dbc04..a2a871b 100644
>> --- a/drivers/iio/pressure/ms5611_core.c
>> +++ b/drivers/iio/pressure/ms5611_core.c
>> @@ -387,8 +387,8 @@ static const struct iio_info ms5611_info = {
>>   static int ms5611_init(struct iio_dev *indio_dev)
>>   {
>>          int ret;
>> -       struct regulator *vdd = devm_regulator_get(indio_dev->dev.parent,
>> -                                                  "vdd");
>> +       struct regulator *vdd =
>> +               devm_regulator_get_optional(indio_dev->dev.parent, "vdd");
>>
>>          /* Enable attached regulator if any. */
>>          if (!IS_ERR(vdd)) {
>> @@ -398,6 +398,10 @@ static int ms5611_init(struct iio_dev *indio_dev)
>>                                  "failed to enable Vdd supply: %d\n", ret);
>>                          return ret;
>>                  }
>> +       } else {
>> +               ret = PTR_ERR(vdd);
>> +               if (ret != -ENODEV)
>> +                       return ret;
>>          }
>>
>>          ret = ms5611_reset(indio_dev);
>> --
>> 2.1.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/1] Fixes: 3145229f9191 ("iio:pressure:ms5611: power regulator support")

[Martin Kepplinger]

Am 2016-03-09 um 11:02 schrieb Gregor Boirie:
> Hi Daniel,
> 
> On 03/08/2016 10:16 PM, Daniel Baluta wrote:
>> On Tue, Mar 8, 2016 at 8:00 PM, Gregor Boirie
>> <gregor.boirie@parrot.com> wrote:
>>> When not compiled-in, regulator layer will return a NULL pointer when
>>> trying to get a reference to any regulator using
>>> devm_regulator_get().  As
>>> IS_ERR() does not consider this an error, the ms5611 probing
>>> operation will
>>> try to enable a NULL regulator, which will invariably cause a kernel
>>> crash.
>>> This patch fixes this situation by using devm_regulator_get_optional()
>>> instead of devm_regulator_get().
>>>
>>> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
>> $Subject should be here. And the commit subject should be something
>> more appropriate like:
>>
>> iio: pressure: ms5611: Fix crash when ...
> I'm a bit lost here since when Jonathan asked for "a fixes tag" inclusion
> (http://permalink.gmane.org/gmane.linux.kernel.iio/22425), I supposed he
> meant a tag such as one described in the Documentation/SubmittingPatches
> file.
> My initial attempt was sent with the subject:
> "iio:pressure:ms5611: fix oops when probing regulator".
> 
> Would you please be kind enough to give me some sort of rules as to when
> and how
> to write subject lines related to fixes patch ?
> 

I guess you can just put in a "Fixes:" line, along with your
"Signed-off-by:" lines.

> Regards,
> Grégor.
> 
>>
>>
>>> ---
>>>   drivers/iio/pressure/ms5611_core.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iio/pressure/ms5611_core.c
>>> b/drivers/iio/pressure/ms5611_core.c
>>> index 37dbc04..a2a871b 100644
>>> --- a/drivers/iio/pressure/ms5611_core.c
>>> +++ b/drivers/iio/pressure/ms5611_core.c
>>> @@ -387,8 +387,8 @@ static const struct iio_info ms5611_info = {
>>>   static int ms5611_init(struct iio_dev *indio_dev)
>>>   {
>>>          int ret;
>>> -       struct regulator *vdd =
>>> devm_regulator_get(indio_dev->dev.parent,
>>> -                                                  "vdd");
>>> +       struct regulator *vdd =
>>> +               devm_regulator_get_optional(indio_dev->dev.parent,
>>> "vdd");
>>>
>>>          /* Enable attached regulator if any. */
>>>          if (!IS_ERR(vdd)) {
>>> @@ -398,6 +398,10 @@ static int ms5611_init(struct iio_dev *indio_dev)
>>>                                  "failed to enable Vdd supply: %d\n",
>>> ret);
>>>                          return ret;
>>>                  }
>>> +       } else {
>>> +               ret = PTR_ERR(vdd);
>>> +               if (ret != -ENODEV)
>>> +                       return ret;
>>>          }
>>>
>>>          ret = ms5611_reset(indio_dev);
>>> -- 
>>> 2.1.4
>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/1] Fixes: 3145229f9191 ("iio:pressure:ms5611: power regulator support")

[Gregor Boirie]

On 03/09/2016 11:06 AM, Martin Kepplinger wrote:
> Am 2016-03-09 um 11:02 schrieb Gregor Boirie:
>> Hi Daniel,
>>
>> On 03/08/2016 10:16 PM, Daniel Baluta wrote:
>>> On Tue, Mar 8, 2016 at 8:00 PM, Gregor Boirie
>>> <gregor.boirie@parrot.com> wrote:
>>>> When not compiled-in, regulator layer will return a NULL pointer when
>>>> trying to get a reference to any regulator using
>>>> devm_regulator_get().  As
>>>> IS_ERR() does not consider this an error, the ms5611 probing
>>>> operation will
>>>> try to enable a NULL regulator, which will invariably cause a kernel
>>>> crash.
>>>> This patch fixes this situation by using devm_regulator_get_optional()
>>>> instead of devm_regulator_get().
>>>>
>>>> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
>>> $Subject should be here. And the commit subject should be something
>>> more appropriate like:
>>>
>>> iio: pressure: ms5611: Fix crash when ...
>> I'm a bit lost here since when Jonathan asked for "a fixes tag" inclusion
>> (http://permalink.gmane.org/gmane.linux.kernel.iio/22425), I supposed he
>> meant a tag such as one described in the Documentation/SubmittingPatches
>> file.
>> My initial attempt was sent with the subject:
>> "iio:pressure:ms5611: fix oops when probing regulator".
>>
>> Would you please be kind enough to give me some sort of rules as to when
>> and how
>> to write subject lines related to fixes patch ?
>>
> I guess you can just put in a "Fixes:" line, along with your
> "Signed-off-by:" lines.
Oh... <light bulb enlightenment/> ! I definitely need some decent 
manners lessons :)
Thanks.

>
>> Regards,
>> Grégor.
>>
>>>
>>>> ---
>>>>    drivers/iio/pressure/ms5611_core.c | 8 ++++++--
>>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/pressure/ms5611_core.c
>>>> b/drivers/iio/pressure/ms5611_core.c
>>>> index 37dbc04..a2a871b 100644
>>>> --- a/drivers/iio/pressure/ms5611_core.c
>>>> +++ b/drivers/iio/pressure/ms5611_core.c
>>>> @@ -387,8 +387,8 @@ static const struct iio_info ms5611_info = {
>>>>    static int ms5611_init(struct iio_dev *indio_dev)
>>>>    {
>>>>           int ret;
>>>> -       struct regulator *vdd =
>>>> devm_regulator_get(indio_dev->dev.parent,
>>>> -                                                  "vdd");
>>>> +       struct regulator *vdd =
>>>> +               devm_regulator_get_optional(indio_dev->dev.parent,
>>>> "vdd");
>>>>
>>>>           /* Enable attached regulator if any. */
>>>>           if (!IS_ERR(vdd)) {
>>>> @@ -398,6 +398,10 @@ static int ms5611_init(struct iio_dev *indio_dev)
>>>>                                   "failed to enable Vdd supply: %d\n",
>>>> ret);
>>>>                           return ret;
>>>>                   }
>>>> +       } else {
>>>> +               ret = PTR_ERR(vdd);
>>>> +               if (ret != -ENODEV)
>>>> +                       return ret;
>>>>           }
>>>>
>>>>           ret = ms5611_reset(indio_dev);
>>>> -- 
>>>> 2.1.4
>>>>
>>>> -- 
>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/1] Fixes: 3145229f9191 ("iio:pressure:ms5611: power regulator support")

[Daniel Baluta]

On Wed, Mar 9, 2016 at 1:20 PM, Gregor Boirie <gregor.boirie@parrot.com> wrote:
>
>
> On 03/09/2016 11:06 AM, Martin Kepplinger wrote:
>>
>> Am 2016-03-09 um 11:02 schrieb Gregor Boirie:
>>>
>>> Hi Daniel,
>>>
>>> On 03/08/2016 10:16 PM, Daniel Baluta wrote:
>>>>
>>>> On Tue, Mar 8, 2016 at 8:00 PM, Gregor Boirie
>>>> <gregor.boirie@parrot.com> wrote:
>>>>>
>>>>> When not compiled-in, regulator layer will return a NULL pointer when
>>>>> trying to get a reference to any regulator using
>>>>> devm_regulator_get().  As
>>>>> IS_ERR() does not consider this an error, the ms5611 probing
>>>>> operation will
>>>>> try to enable a NULL regulator, which will invariably cause a kernel
>>>>> crash.
>>>>> This patch fixes this situation by using devm_regulator_get_optional()
>>>>> instead of devm_regulator_get().
>>>>>
>>>>> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
>>>>
>>>> $Subject should be here. And the commit subject should be something
>>>> more appropriate like:
>>>>
>>>> iio: pressure: ms5611: Fix crash when ...
>>>
>>> I'm a bit lost here since when Jonathan asked for "a fixes tag" inclusion
>>> (http://permalink.gmane.org/gmane.linux.kernel.iio/22425), I supposed he
>>> meant a tag such as one described in the Documentation/SubmittingPatches
>>> file.
>>> My initial attempt was sent with the subject:
>>> "iio:pressure:ms5611: fix oops when probing regulator".

The initial subject was OK.

>>>
>>> Would you please be kind enough to give me some sort of rules as to when
>>> and how
>>> to write subject lines related to fixes patch ?
>>>
>> I guess you can just put in a "Fixes:" line, along with your
>> "Signed-off-by:" lines.
>
> Oh... <light bulb enlightenment/> ! I definitely need some decent manners
> lessons :)
> Thanks.

What Martin said :). For example, you can look at this patch:

https://lkml.org/lkml/2016/2/16/505

Daniel.