Messages in this thread | | | From | Andy Shevchenko <> | Date | Sun, 30 Aug 2020 20:39:52 +0300 | Subject | Re: [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
| |