lkml.org 
[lkml]   [2020]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3 1/2] iio: adc: mt6360: Add ADC driver for MT6360
On Mon, Aug 24, 2020 at 12:07 PM Gene Chen <gene.chen.richtek@gmail.com> 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

\
 
 \ /
  Last update: 2020-08-30 19:41    [W:0.095 / U:28.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site