On Mon, Aug 24, 2020 at 12:07 PM Gene Chen [off-list ref] wrote:

Add MT6360 ADC driver include Charger Current, Voltage, and

include -> that includes

Temperature.

...

+       help
+         Say Y here to enable MT6360 ADC Part.
+         Integrated for System Monitoring include
+         Charger and Battery Current, Voltage and
+         Temperature

We have a wider field for this text. Also, use proper punctuation.

...

+#include <linux/completion.h>
+#include <linux/interrupt.h>

+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>

I would rather move this block after linux/* headers because it's
subsystem related (not so generic).

+#include <linux/irq.h>

Are you sure about this?

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

...

+#define MT6360_AICR_MASK       0xFC

GENMASK() (and include bits.h for that).

+#define MT6360_ADCEN_MASK      0x8000

+#define MT6360_PREFERCH_MASK   0xF0

+#define MT6360_RPTCH_MASK      0x0F

Ditto for all above.

...

+enum {
+       MT6360_CHAN_USBID = 0,
+       MT6360_CHAN_VBUSDIV5,
+       MT6360_CHAN_VBUSDIV2,
+       MT6360_CHAN_VSYS,
+       MT6360_CHAN_VBAT,
+       MT6360_CHAN_IBUS,
+       MT6360_CHAN_IBAT,
+       MT6360_CHAN_CHG_VDDP,
+       MT6360_CHAN_TEMP_JC,
+       MT6360_CHAN_VREF_TS,
+       MT6360_CHAN_TS,

+       MT6360_CHAN_MAX,

No comma for terminator.

+};

...

+       const struct converter {
+               int multiplier;
+               int offset;
+               int divisor;
+       } adc_converter[MT6360_CHAN_MAX] = {
+               { 1250, 0, 1}, /* USBID */
+               { 6250, 0, 1}, /* VBUSDIV5 */
+               { 2500, 0, 1}, /* VBUSDIV2 */
+               { 1250, 0, 1}, /* VSYS */
+               { 1250, 0, 1}, /* VBAT */
+               { 2500, 0, 1}, /* IBUS */
+               { 2500, 0, 1}, /* IBAT */
+               { 1250, 0, 1}, /* CHG_VDDP */
+               { 105, -8000, 100}, /* TEMP_JC */
+               { 1250, 0, 1}, /* VREF_TS */
+               { 1250, 0, 1}, /* TS */
+       }, sp_ibus_adc_converter = { 1900, 0, 1 }, *sel_converter;

This is quite hidden in the code. Better to move out from the function at least.

...

+       start_t = ktime_get();
+       predict_end_t = ktime_add_ms(mad->last_off_timestamps[channel], 50);
+
+       if (ktime_after(start_t, predict_end_t))
+               predict_end_t = ktime_add_ms(start_t, 25);
+       else
+               predict_end_t = ktime_add_ms(start_t, 75);
+

+       timeout = wait_for_completion_timeout(&mad->adc_complete, msecs_to_jiffies(200));

Above code full of magic numbers.

...

+       value = (rpt[1] << 8) | rpt[2];

put_unaligned_be16() (or what is this?)

...

+       /* set prefer channel to 0xf */

What 0x0f is?

+       regmap_update_bits(mad->regmap, MT6360_REG_PMUADCRPT1, MT6360_PREFERCH_MASK,
+                          0xF << MT6360_PREFERCH_SHFT);

0xf should be GENMASK() and have its descriptive name.

+out_adc:

The rule of thumb is to explain in the label what is going to do,
rather than some abstract words. Here, i.e.,
out_mutex_unlock:

+       mutex_unlock(&mad->adc_lock);

...

+       if (mask == IIO_CHAN_INFO_PROCESSED)
+               return mt6360_adc_read_processed(mad, chan->channel, val);
+
+       return -EINVAL;

Usual pattern is
 if (err_condition)
  ...handle error...

...

+       /* 11 ch s32 numbers + 1 s64 timestamp */
+       s32 data[MT6360_CHAN_MAX + 2] __aligned(8);

We have a better approach now (with a struct being used).

...

+       u8 configs[3] = {0x80, 0, 0};

Magic.

...

+static int mt6360_adc_probe(struct platform_device *pdev)
+{

+       mad->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+       if (!mad->regmap) {
+               dev_err(&pdev->dev, "Failed to get parent regmap\n");
+               return -ENODEV;
+       }

You may do this before allocation happens. Also consider to introduce
temporary variable to simplify below code, i.e.

struct device *dev = &pdev->dev;
struct regmap *regmap;

...

+       mad->irq = platform_get_irq_byname(pdev, "adc_donei");
+       if (mad->irq < 0) {

+               dev_err(&pdev->dev, "Failed to get adc_done irq\n");

This duplicates the core message.

+               return mad->irq;
+       }

...

+       irq_set_status_flags(mad->irq, IRQ_NOAUTOEN);

Why?

+       ret = devm_iio_device_register(&pdev->dev, indio_dev);
+       if (ret) {
+               dev_err(&pdev->dev, "Failed to register iio device\n");

+               return ret;
+       }
+
+       return 0;

return ret; (At least, but I would go even further and do return
devm_iio_device_register(); directly)

+}

...

+static const struct of_device_id __maybe_unused mt6360_adc_of_id[] = {
+       { .compatible = "mediatek,mt6360-adc", },
+       {},

No comma for terminator line.

+};

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel