Re: [PATCH v3 10/14] iio: adc: mt6370: Add Mediatek MT6370 support
flat view
From: Andy Shevchenko <hidden>
Date: 2022-06-23 19:22:04
Also in:
dri-devel, linux-arm-kernel, linux-devicetree, linux-iio, linux-leds, linux-mediatek, linux-pm, linux-usb, lkml
On Thu, Jun 23, 2022 at 2:00 PM ChiaEn Wu [off-list ref] wrote:
From: ChiaEn Wu <redacted> Add Mediatek MT6370 ADC support.
...
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+#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.
quoted hunk ↗ jump to hunk
+#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? ...
quoted hunk ↗ jump to hunk
+ 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
quoted hunk ↗ jump to hunk
+ 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; + }
...
quoted hunk ↗ jump to hunk
+#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(...);
+ }
...
quoted hunk ↗ jump to hunk
+ 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