From: Stanimir Varbanov <svarbanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org> To: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Cc: Hartmut Knaack <knaack.h-Mmb7MZpHnFY@public.gmane.org>, Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>, Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>, Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>, Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>, Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>, Angelo Compagnucci <angelo.compagnucci-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>, Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>, Johannes Thumshirn <johannes.thumshirn-csrFAY9JiS4@public.gmane.org>, Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>, Philippe Reynes <tremyfr-Qt13gs6zZMY@public.gmane.org>, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>, Josh Cartwright <joshc-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>, Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>, David Collins <collinsd@code> Subject: Re: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver Date: Thu, 18 Sep 2014 12:57:20 +0300 [thread overview] Message-ID: <541AAC80.1090708@mm-sol.com> (raw) In-Reply-To: <4dbc485f-599a-4b50-854c-c2e1c44d4810-2ueSQiBKiTY7tOexoI0I+QC/G2K4zDHf@public.gmane.org> Hi Jonathan, On 09/15/2014 07:11 PM, Jonathan Cameron wrote: > > > On September 15, 2014 3:12:50 PM GMT+01:00, Stanimir Varbanov <svarbanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org> wrote: >> Hi Jonathan, >> >> Thanks for the review! >> >> On 09/13/2014 08:27 PM, Jonathan Cameron wrote: >>> On 13/09/14 00:27, Hartmut Knaack wrote: >>>> Stanimir Varbanov schrieb, Am 11.09.2014 17:13: >>>>> The voltage ADC is peripheral of Qualcomm SPMI PMIC chips. It has >>>>> 15bits resolution and register space inside PMIC accessible across >>>>> SPMI bus. >>>>> >>>>> The vadc driver registers itself through IIO interface. >>>>> >>>> Looks already pretty good. Things you should consider in regard of >> common coding style are to use the variable name ret instead of rc, >> since it is used in almost all adc drivers and thus makes reviewing a >> bit easier. Besides that, you seem to use unsigned as well as unsigned >> int, so to be consistent, please stick to one of them. Other comments >> in line. >>> >>> A few additional comments from me. My biggest question is whether >>> you are actually making life difficult for yourself by having >>> vadc_channels and vadc->channels (don't like the similar naming btw!) >>> in different orders. I think you can move the ordering into the >> device >>> tree reading code rather than doing it in lots of other places. >> Hence >>> rather than an order based on the device tree description, put the >>> data into a fixed ofer in vadc->channels. >>> >>> Entirely possible I'm missing something though :) >>>>> Signed-off-by: Stanimir Varbanov <svarbanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org> >>>>> Signed-off-by: Ivan T. Ivanov <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org> >>>>> --- >>>>> drivers/iio/adc/Kconfig | 11 + >>>>> drivers/iio/adc/Makefile | 1 + >>>>> drivers/iio/adc/qcom-spmi-vadc.c | 999 >> +++++++++++++++++++++++++ >>>>> include/dt-bindings/iio/qcom,spmi-pmic-vadc.h | 119 +++ >>>>> 4 files changed, 1130 insertions(+), 0 deletions(-) >>>>> create mode 100644 drivers/iio/adc/qcom-spmi-vadc.c >>>>> create mode 100644 include/dt-bindings/iio/qcom,spmi-pmic-vadc.h <snip> >>>>> + vchan = vadc_find_channel(vadc, chan->channel); >>>>> + if (!vchan) >>>>> + return -EINVAL; >>>>> + >>>>> + if (!vadc->is_ref_measured) { >>>>> + rc = vadc_measure_reference_points(vadc); >>>>> + if (rc) >>>>> + return rc; >>>>> + >>>>> + vadc->is_ref_measured = true; >>>>> + } >>>>> + >>>>> + switch (mask) { >>>>> + case IIO_CHAN_INFO_PROCESSED: >>>>> + rc = vadc_do_conversion(vadc, vchan, &result.adc_code); >>>>> + if (rc) >>>>> + return rc; >>>>> + >>>>> + vadc_calibrate(vadc, vchan, &result); >>>>> + >>>>> + *val = result.physical; >>> I'm a little suspicious here. Are the resulting values in milivolts >> for >>> all the channels? Very handy if so, but seems a little unlikely with >> 15 bit >>> ADC that you'd have no part of greater accuracy than a milivolt. >> >> In fact *val is in microvolts. What is the expected unit from IIO ADC >> users? > See Documentation/ABI/sysfs-bus-iio > Millivolts I think... We copied hwmon where possible. I'm a bit confused about these units. I searched references of iio_read_channel_processed() and found a few. The iio_hwmon expecting milivolts. On the other side lp8788-charger.c registers a get_property method in charger-manager.c, which expects microvolts in get_batt_uV(). I also wonder how to implement IIO read_raw() method so as not to lose precision (if assume we must return millivolts). As far I can see it should be some combination of IIO_CHAN_INFO_RAW and IIO_CHAN_INFO_SCALE masks. Any hints? >> >>>>> + rc = IIO_VAL_INT; >>>> return IIO_VAL_INT; >>>>> + break; >>>>> + default: >>>>> + rc = -EINVAL; >>>>> + break; >>>> Drop default case, or leave empty. >>>>> + } >>>>> + >>>>> + return rc; >>>> return -EINVAL; >>>>> +} -- regards, Stan
WARNING: multiple messages have this Message-ID (diff)
From: Stanimir Varbanov <svarbanov@mm-sol.com> To: Jonathan Cameron <jic23@kernel.org> Cc: Hartmut Knaack <knaack.h@gmx.de>, Ian Campbell <ijc+devicetree@hellion.org.uk>, Pawel Moll <pawel.moll@arm.com>, Rob Herring <robh+dt@kernel.org>, Kumar Gala <galak@codeaurora.org>, Mark Rutland <mark.rutland@arm.com>, Grant Likely <grant.likely@linaro.org>, Arnd Bergmann <arnd@arndb.de>, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Lars-Peter Clausen <lars@metafoo.de>, Angelo Compagnucci <angelo.compagnucci@gmail.com>, Doug Anderson <dianders@chromium.org>, Fugang Duan <B38611@freescale.com>, Johannes Thumshirn <johannes.thumshirn@men.de>, Jean Delvare <jdelvare@suse.de>, Philippe Reynes <tremyfr@yahoo.fr>, Lee Jones <lee.jones@linaro.org>, Josh Cartwright <joshc@codeaurora.org>, Stephen Boyd <sboyd@codeaurora.org>, David Collins <collinsd@codeaurora.org>, "Ivan T. Ivanov" <iivanov@mm-sol.com> Subject: Re: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver Date: Thu, 18 Sep 2014 12:57:20 +0300 [thread overview] Message-ID: <541AAC80.1090708@mm-sol.com> (raw) In-Reply-To: <4dbc485f-599a-4b50-854c-c2e1c44d4810@email.android.com> Hi Jonathan, On 09/15/2014 07:11 PM, Jonathan Cameron wrote: > > > On September 15, 2014 3:12:50 PM GMT+01:00, Stanimir Varbanov <svarbanov@mm-sol.com> wrote: >> Hi Jonathan, >> >> Thanks for the review! >> >> On 09/13/2014 08:27 PM, Jonathan Cameron wrote: >>> On 13/09/14 00:27, Hartmut Knaack wrote: >>>> Stanimir Varbanov schrieb, Am 11.09.2014 17:13: >>>>> The voltage ADC is peripheral of Qualcomm SPMI PMIC chips. It has >>>>> 15bits resolution and register space inside PMIC accessible across >>>>> SPMI bus. >>>>> >>>>> The vadc driver registers itself through IIO interface. >>>>> >>>> Looks already pretty good. Things you should consider in regard of >> common coding style are to use the variable name ret instead of rc, >> since it is used in almost all adc drivers and thus makes reviewing a >> bit easier. Besides that, you seem to use unsigned as well as unsigned >> int, so to be consistent, please stick to one of them. Other comments >> in line. >>> >>> A few additional comments from me. My biggest question is whether >>> you are actually making life difficult for yourself by having >>> vadc_channels and vadc->channels (don't like the similar naming btw!) >>> in different orders. I think you can move the ordering into the >> device >>> tree reading code rather than doing it in lots of other places. >> Hence >>> rather than an order based on the device tree description, put the >>> data into a fixed ofer in vadc->channels. >>> >>> Entirely possible I'm missing something though :) >>>>> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com> >>>>> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> >>>>> --- >>>>> drivers/iio/adc/Kconfig | 11 + >>>>> drivers/iio/adc/Makefile | 1 + >>>>> drivers/iio/adc/qcom-spmi-vadc.c | 999 >> +++++++++++++++++++++++++ >>>>> include/dt-bindings/iio/qcom,spmi-pmic-vadc.h | 119 +++ >>>>> 4 files changed, 1130 insertions(+), 0 deletions(-) >>>>> create mode 100644 drivers/iio/adc/qcom-spmi-vadc.c >>>>> create mode 100644 include/dt-bindings/iio/qcom,spmi-pmic-vadc.h <snip> >>>>> + vchan = vadc_find_channel(vadc, chan->channel); >>>>> + if (!vchan) >>>>> + return -EINVAL; >>>>> + >>>>> + if (!vadc->is_ref_measured) { >>>>> + rc = vadc_measure_reference_points(vadc); >>>>> + if (rc) >>>>> + return rc; >>>>> + >>>>> + vadc->is_ref_measured = true; >>>>> + } >>>>> + >>>>> + switch (mask) { >>>>> + case IIO_CHAN_INFO_PROCESSED: >>>>> + rc = vadc_do_conversion(vadc, vchan, &result.adc_code); >>>>> + if (rc) >>>>> + return rc; >>>>> + >>>>> + vadc_calibrate(vadc, vchan, &result); >>>>> + >>>>> + *val = result.physical; >>> I'm a little suspicious here. Are the resulting values in milivolts >> for >>> all the channels? Very handy if so, but seems a little unlikely with >> 15 bit >>> ADC that you'd have no part of greater accuracy than a milivolt. >> >> In fact *val is in microvolts. What is the expected unit from IIO ADC >> users? > See Documentation/ABI/sysfs-bus-iio > Millivolts I think... We copied hwmon where possible. I'm a bit confused about these units. I searched references of iio_read_channel_processed() and found a few. The iio_hwmon expecting milivolts. On the other side lp8788-charger.c registers a get_property method in charger-manager.c, which expects microvolts in get_batt_uV(). I also wonder how to implement IIO read_raw() method so as not to lose precision (if assume we must return millivolts). As far I can see it should be some combination of IIO_CHAN_INFO_RAW and IIO_CHAN_INFO_SCALE masks. Any hints? >> >>>>> + rc = IIO_VAL_INT; >>>> return IIO_VAL_INT; >>>>> + break; >>>>> + default: >>>>> + rc = -EINVAL; >>>>> + break; >>>> Drop default case, or leave empty. >>>>> + } >>>>> + >>>>> + return rc; >>>> return -EINVAL; >>>>> +} -- regards, Stan
next prev parent reply other threads:[~2014-09-18 9:57 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-09-11 15:13 [PATCH v2 0/2] Intial support for voltage ADC Stanimir Varbanov 2014-09-11 15:13 ` [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver Stanimir Varbanov 2014-09-12 23:27 ` Hartmut Knaack 2014-09-13 17:27 ` Jonathan Cameron [not found] ` <54147E97.60808-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 2014-09-15 14:12 ` Stanimir Varbanov 2014-09-15 14:12 ` Stanimir Varbanov [not found] ` <5416F3E2.3030009-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org> 2014-09-15 16:11 ` Jonathan Cameron 2014-09-15 16:11 ` Jonathan Cameron [not found] ` <4dbc485f-599a-4b50-854c-c2e1c44d4810-2ueSQiBKiTY7tOexoI0I+QC/G2K4zDHf@public.gmane.org> 2014-09-18 9:57 ` Stanimir Varbanov [this message] 2014-09-18 9:57 ` Stanimir Varbanov [not found] ` <541AAC80.1090708-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org> 2014-09-21 13:29 ` Jonathan Cameron 2014-09-21 13:29 ` Jonathan Cameron 2014-09-22 1:01 ` Kim, Milo 2014-09-22 1:01 ` Kim, Milo 2014-09-22 1:01 ` Kim, Milo 2014-09-22 6:28 ` Jonathan Cameron [not found] ` <54138151.8010902-Mmb7MZpHnFY@public.gmane.org> 2014-09-15 14:16 ` Stanimir Varbanov 2014-09-15 14:16 ` Stanimir Varbanov 2014-09-11 15:13 ` [PATCH v2 2/2] DT: iio: vadc: document dt binding Stanimir Varbanov [not found] ` <1410448403-19402-3-git-send-email-svarbanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org> 2014-09-12 23:35 ` Hartmut Knaack 2014-09-12 23:35 ` Hartmut Knaack [not found] ` <5413832F.6040503-Mmb7MZpHnFY@public.gmane.org> 2014-09-13 17:32 ` Jonathan Cameron 2014-09-13 17:32 ` Jonathan Cameron
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=541AAC80.1090708@mm-sol.com \ --to=svarbanov-neyub+7iv8pqt0dzr+alfa@public.gmane.org \ --cc=B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org \ --cc=angelo.compagnucci-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ --cc=arnd-r2nGTMty4D4@public.gmane.org \ --cc=collinsd@code \ --cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org \ --cc=galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org \ --cc=grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \ --cc=gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org \ --cc=ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org \ --cc=jdelvare-l3A5Bk7waGM@public.gmane.org \ --cc=jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \ --cc=johannes.thumshirn-csrFAY9JiS4@public.gmane.org \ --cc=joshc-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org \ --cc=knaack.h-Mmb7MZpHnFY@public.gmane.org \ --cc=lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org \ --cc=lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \ --cc=linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=mark.rutland-5wv7dgnIgG8@public.gmane.org \ --cc=pawel.moll-5wv7dgnIgG8@public.gmane.org \ --cc=robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \ --cc=sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org \ --cc=tremyfr-Qt13gs6zZMY@public.gmane.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.