Re: [PATCH v3 1/2] iio: adc: mt6360: Add ADC driver for MT6360

[Andy Shevchenko]

On Mon, Aug 24, 2020 at 12:07 PM Gene Chen <gene.chen.richtek@xxxxxxxxx> 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