lkml.org 
[lkml]   [2022]   [Jun]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 10/14] iio: adc: mt6370: Add Mediatek MT6370 support
    On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu <peterwu.pub@gmail.com> wrote:
    >
    > From: ChiaEn Wu <chiaen_wu@richtek.com>
    >
    > Add Mediatek MT6370 ADC support.

    ...

    > +config MEDIATEK_MT6370_ADC
    > + tristate "Mediatek MT6370 ADC driver"
    > + depends on MFD_MT6370
    > + help
    > + Say yes here to enable Mediatek MT6370 ADC support.
    > +
    > + This ADC driver provides 9 channels for system monitoring (charger
    > + current, voltage, and temperature).

    What will be the module name?

    ...

    > +#include <dt-bindings/iio/adc/mediatek,mt6370_adc.h>

    Usually this goes after linux/* asm/* as it's not so generic.

    > +#include <linux/bits.h>
    > +#include <linux/bitfield.h>
    > +#include <linux/iio/iio.h>
    > +#include <linux/kernel.h>
    > +#include <linux/module.h>

    > +#include <linux/mod_devicetable.h>

    I believe the order should be otherwise, this is first followed by module.h.

    > +#include <linux/mutex.h>
    > +#include <linux/platform_device.h>
    > +#include <linux/regmap.h>

    ...

    > +#define ADC_CONV_POLLING_TIME 1000

    If it's time, add a unit suffix, if it's a counter, make it clear.

    ...

    > + msleep(ADC_CONV_TIME_US / 1000);

    Why define microseconds if milliseconds are in use?

    ...

    > + ret = regmap_read_poll_timeout(priv->regmap,
    > + MT6370_REG_CHG_ADC, reg_val,
    > + !(reg_val & MT6370_ADC_START_MASK),
    > + ADC_CONV_POLLING_TIME,
    > + ADC_CONV_TIME_US * 3);
    > + if (ret) {
    > + if (ret == -ETIMEDOUT)
    > + dev_err(priv->dev, "Failed to wait ADC conversion\n");

    wait for

    > + else
    > + dev_err(priv->dev,
    > + "Failed to read ADC register (%d)\n", ret);

    Do you really need to differentiate the errors here? I believe the
    latter one covers all cases.

    > + goto adc_unlock;
    > + }

    ...

    > +#define MT6370_ADC_CHAN(_idx, _type, _addr, _extra_info) { \
    > + .type = _type, \
    > + .channel = MT6370_CHAN_##_idx, \
    > + .address = _addr, \
    > + .scan_index = MT6370_CHAN_##_idx, \
    > + .indexed = 1, \
    > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
    > + BIT(IIO_CHAN_INFO_SCALE) | \
    > + _extra_info \

    Leave a comma after the last member as well.

    > +}

    ...

    > + regmap = dev_get_regmap(pdev->dev.parent, NULL);
    > + if (!regmap) {

    > + dev_err(&pdev->dev, "Failed to get regmap\n");
    > + return -ENODEV;

    return dev_err_probe(...);

    > + }

    ...

    > + ret = regmap_write(priv->regmap, MT6370_REG_CHG_ADC, 0);
    > + if (ret) {
    > + dev_err(&pdev->dev, "Failed to reset ADC\n");
    > + return ret;
    > + }

    Ditto.

    --
    With Best Regards,
    Andy Shevchenko

    \
     
     \ /
      Last update: 2022-06-23 21:23    [W:4.184 / U:0.100 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site