Re: [PATCH v3 10/14] iio: adc: mt6370: Add Mediatek MT6370 support

Andy Shevchenko
2022-06-23T19:22:02Z

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