From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Gene Chen <gene.chen.richtek@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Hartmut Knaack <knaack.h@gmx.de>,
Lars-Peter Clausen <lars@metafoo.de>,
Peter Meerwald <pmeerw@pmeerw.net>,
linux-iio <linux-iio@vger.kernel.org>,
linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@lists.infradead.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
gene_chen@richtek.com, Wilma.Wu@mediatek.com,
shufan_lee@richtek.com, cy_huang@richtek.com,
benjamin.chao@mediatek.com
Subject: Re: [PATCH v3 1/2] iio: adc: mt6360: Add ADC driver for MT6360
Date: Sun, 30 Aug 2020 20:39:52 +0300 [thread overview]
Message-ID: <CAHp75VdmzurQQB2597VB=PCt0THV0TRVdhXdvO3Z3qcdLCeEPg@mail.gmail.com> (raw)
In-Reply-To: <1598259985-12517-2-git-send-email-gene.chen.richtek@gmail.com>
On Mon, Aug 24, 2020 at 12:07 PM Gene Chen <gene.chen.richtek@gmail.com> wrote:
>
> Add MT6360 ADC driver include Charger Current, Voltage, and
include -> that includes
> Temperature.
...
> + help
> + Say Y here to enable MT6360 ADC Part.
> + Integrated for System Monitoring include
> + Charger and Battery Current, Voltage and
> + Temperature
We have a wider field for this text. Also, use proper punctuation.
...
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
I would rather move this block after linux/* headers because it's
subsystem related (not so generic).
> +#include <linux/irq.h>
Are you sure about this?
> +#include <linux/kernel.h>
> +#include <linux/ktime.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
...
> +#define MT6360_AICR_MASK 0xFC
GENMASK() (and include bits.h for that).
> +#define MT6360_ADCEN_MASK 0x8000
> +#define MT6360_PREFERCH_MASK 0xF0
> +#define MT6360_RPTCH_MASK 0x0F
Ditto for all above.
...
> +enum {
> + MT6360_CHAN_USBID = 0,
> + MT6360_CHAN_VBUSDIV5,
> + MT6360_CHAN_VBUSDIV2,
> + MT6360_CHAN_VSYS,
> + MT6360_CHAN_VBAT,
> + MT6360_CHAN_IBUS,
> + MT6360_CHAN_IBAT,
> + MT6360_CHAN_CHG_VDDP,
> + MT6360_CHAN_TEMP_JC,
> + MT6360_CHAN_VREF_TS,
> + MT6360_CHAN_TS,
> + MT6360_CHAN_MAX,
No comma for terminator.
> +};
...
> + const struct converter {
> + int multiplier;
> + int offset;
> + int divisor;
> + } adc_converter[MT6360_CHAN_MAX] = {
> + { 1250, 0, 1}, /* USBID */
> + { 6250, 0, 1}, /* VBUSDIV5 */
> + { 2500, 0, 1}, /* VBUSDIV2 */
> + { 1250, 0, 1}, /* VSYS */
> + { 1250, 0, 1}, /* VBAT */
> + { 2500, 0, 1}, /* IBUS */
> + { 2500, 0, 1}, /* IBAT */
> + { 1250, 0, 1}, /* CHG_VDDP */
> + { 105, -8000, 100}, /* TEMP_JC */
> + { 1250, 0, 1}, /* VREF_TS */
> + { 1250, 0, 1}, /* TS */
> + }, sp_ibus_adc_converter = { 1900, 0, 1 }, *sel_converter;
This is quite hidden in the code. Better to move out from the function at least.
...
> + start_t = ktime_get();
> + predict_end_t = ktime_add_ms(mad->last_off_timestamps[channel], 50);
> +
> + if (ktime_after(start_t, predict_end_t))
> + predict_end_t = ktime_add_ms(start_t, 25);
> + else
> + predict_end_t = ktime_add_ms(start_t, 75);
> +
> + timeout = wait_for_completion_timeout(&mad->adc_complete, msecs_to_jiffies(200));
Above code full of magic numbers.
...
> + value = (rpt[1] << 8) | rpt[2];
put_unaligned_be16() (or what is this?)
...
> + /* set prefer channel to 0xf */
What 0x0f is?
> + regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
> + 0xF << MT6360_PREFERCH_SHFT);
0xf should be GENMASK() and have its descriptive name.
> +out_adc:
The rule of thumb is to explain in the label what is going to do,
rather than some abstract words. Here, i.e.,
out_mutex_unlock:
> + mutex_unlock(&mad->adc_lock);
...
> + if (mask == IIO_CHAN_INFO_PROCESSED)
> + return mt6360_adc_read_processed(mad, chan->channel, val);
> +
> + return -EINVAL;
Usual pattern is
if (err_condition)
...handle error...
...
> + /* 11 ch s32 numbers + 1 s64 timestamp */
> + s32 data[MT6360_CHAN_MAX + 2] __aligned(8);
We have a better approach now (with a struct being used).
...
> + u8 configs[3] = {0x80, 0, 0};
Magic.
...
> +static int mt6360_adc_probe(struct platform_device *pdev)
> +{
> + mad->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> + if (!mad->regmap) {
> + dev_err(&pdev->dev, "Failed to get parent regmap\n");
> + return -ENODEV;
> + }
You may do this before allocation happens. Also consider to introduce
temporary variable to simplify below code, i.e.
struct device *dev = &pdev->dev;
struct regmap *regmap;
...
> + mad->irq = platform_get_irq_byname(pdev, "adc_donei");
> + if (mad->irq < 0) {
> + dev_err(&pdev->dev, "Failed to get adc_done irq\n");
This duplicates the core message.
> + return mad->irq;
> + }
...
> + irq_set_status_flags(mad->irq, IRQ_NOAUTOEN);
Why?
> + ret = devm_iio_device_register(&pdev->dev, indio_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register iio device\n");
> + return ret;
> + }
> +
> + return 0;
return ret; (At least, but I would go even further and do return
devm_iio_device_register(); directly)
> +}
...
> +static const struct of_device_id __maybe_unused mt6360_adc_of_id[] = {
> + { .compatible = "mediatek,mt6360-adc", },
> + {},
No comma for terminator line.
> +};
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2020-08-30 17:40 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-24 9:06 [PATCH v3 0/2] iio: adc: mt6360: Add ADC driver for MT6360 Gene Chen
2020-08-24 9:06 ` [PATCH v3 1/2] " Gene Chen
2020-08-27 12:16 ` kernel test robot
2020-08-29 17:11 ` Jonathan Cameron
2020-09-08 6:17 ` Gene Chen
2020-09-08 7:43 ` Andy Shevchenko
2020-09-08 9:07 ` Jonathan Cameron
2020-09-08 11:08 ` Gene Chen
2020-09-08 12:58 ` Jonathan Cameron
2020-09-08 23:39 ` Gene Chen
2020-09-09 9:34 ` Jonathan Cameron
2020-09-10 0:14 ` Gene Chen
2020-08-30 17:39 ` Andy Shevchenko [this message]
2020-08-24 9:06 ` [PATCH v3 2/2] Documentation: ABI: testing: mt6360: Add ADC sysfs guideline Gene Chen
2020-08-29 17:15 ` 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='CAHp75VdmzurQQB2597VB=PCt0THV0TRVdhXdvO3Z3qcdLCeEPg@mail.gmail.com' \
--to=andy.shevchenko@gmail.com \
--cc=Wilma.Wu@mediatek.com \
--cc=benjamin.chao@mediatek.com \
--cc=cy_huang@richtek.com \
--cc=gene.chen.richtek@gmail.com \
--cc=gene_chen@richtek.com \
--cc=jic23@kernel.org \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=pmeerw@pmeerw.net \
--cc=shufan_lee@richtek.com \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).