Re: [PATCH v3 10/14] iio: adc: mt6370: Add Mediatek MT6370 support
From: Andy Shevchenko
Date: Thu Jun 23 2022 - 15:22:04 EST
On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu <peterwu.pub@xxxxxxxxx> wrote:
>
> From: ChiaEn Wu <chiaen_wu@xxxxxxxxxxx>
>
> 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