Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 23 Jun 2022 20:25:48 +0200 | Subject | Re: [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
| |