| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VdmzurQQB2597VB=PCt0THV0TRVdhXdvO3Z3qcdLCeEPg@mail.gmail.com>
Date: Sun, 30 Aug 2020 20:39:52 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Gene Chen <gene.chen.richtek@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald <pmeerw@...erw.net>,
linux-iio <linux-iio@...r.kernel.org>,
linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@...ts.infradead.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
gene_chen@...htek.com, Wilma.Wu@...iatek.com,
shufan_lee@...htek.com, cy_huang@...htek.com,
benjamin.chao@...iatek.com
Subject: Re: [PATCH v3 1/2] iio: adc: mt6360: Add ADC driver for MT6360
On Mon, Aug 24, 2020 at 12:07 PM Gene Chen <gene.chen.richtek@...il.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
Powered by blists - more mailing lists