Re: [PATCH v1 2/4] iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC

Andy Shevchenko
2024-05-30T13:35:06Z

On Thu, May 30, 2024 at 12:34 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Add a driver to support reading the Auxiliary ADC IP found in the
> MediaTek MT6357, MT6358 and MT6359 Power Management ICs.
>
> This driver provides multiple ADC channels for system monitoring,
> such as battery voltage, PMIC temperature, PMIC-internal voltage
> regulators temperature, and others.

> ---

Here is no explanation on how this is differ to any of the three
existing drivers? I.o.w. why do we need a brand new one?

...

+ bits.h

> +#include <linux/delay.h>

> +#include <linux/kernel.h>

Why?

+ mod_devicetable.h
> +#include <linux/module.h>

> +#include <linux/of.h>

Why?

> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

+ types.h

+ blank line

> +#include <linux/iio/iio.h>

+ Blank line

> +#include <linux/mfd/mt6397/core.h>

...

> +#define PMIC_RG_RESET_VAL (BIT(0) | BIT(3))

In this form it requires a comment explaining each mentioned bit.

> +#define PMIC_AUXADC_ADCx(x) ((x) << 1)

Seems like a useless macro, it occupies much more space than in-place shift.

...

> +#define MT6358_IMP0_CLEAR (BIT(14) | BIT(7))

As per above.

...

> +/**
> + * struct mtk_pmic_auxadc_chan - PMIC AUXADC channel data
> + * @req_idx: Request register number
> + * @req_mask: Bitmask to activate a channel
> + * @num_samples: Number of AUXADC samples for averaging
> + * @r_numerator: Resistance ratio numerator
> + * @r_denominator: Resistance ratio denominator
> + */
> +struct mtk_pmic_auxadc_chan {
> + u8 req_idx;
> + u16 req_mask;
> + u16 num_samples;

> + u8 r_numerator;
> + u8 r_denominator;

Can you add struct u8_fract to the math.h and use it? I will Ack/R the
respective patch.

> +};

...

> +struct mtk_pmic_auxadc_pdata {
> + const struct iio_chan_spec *channels;
> + int num_channels;

Why signed?

> + const struct mtk_pmic_auxadc_chan *desc;
> + const u16 *regs;
> + u16 sec_unlock_key;
> + u8 imp_adc_num;
> + int (*read_imp)(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat);
> +};

...

> +static const u16 mt6357_auxadc_regs[] = {
> + [PMIC_HK_TOP_RST_CON0] = 0xf90,

Can we use the fixed-width values so all of them will look nice and
easy to parse?

> + [PMIC_AUXADC_DCM_CON] = 0x122e,
> + [PMIC_AUXADC_ADC0] = 0x1088,
> + [PMIC_AUXADC_IMP0] = 0x119c,
> + [PMIC_AUXADC_IMP1] = 0x119e,
> + [PMIC_AUXADC_RQST0] = 0x110e,
> + [PMIC_AUXADC_RQST1] = 0x1114,
> +};

...

> +static const u16 mt6358_auxadc_regs[] = {

Ditto.

> + [PMIC_HK_TOP_RST_CON0] = 0xf90,
> + [PMIC_AUXADC_DCM_CON] = 0x1260,
> + [PMIC_AUXADC_ADC0] = 0x1088,
> + [PMIC_AUXADC_IMP0] = 0x1208,
> + [PMIC_AUXADC_IMP1] = 0x120a,
> + [PMIC_AUXADC_RQST0] = 0x1108,
> + [PMIC_AUXADC_RQST1] = 0x110a,
> +};

...

> +static const u16 mt6359_auxadc_regs[] = {

Ditto.

> + [PMIC_FGADC_R_CON0] = 0xd88,
> + [PMIC_HK_TOP_WKEY] = 0xfb4,
> + [PMIC_HK_TOP_RST_CON0] = 0xf90,
> + [PMIC_AUXADC_RQST0] = 0x1108,
> + [PMIC_AUXADC_RQST1] = 0x110a,
> + [PMIC_AUXADC_ADC0] = 0x1088,
> + [PMIC_AUXADC_IMP0] = 0x1208,
> + [PMIC_AUXADC_IMP1] = 0x120a,
> + [PMIC_AUXADC_IMP3] = 0x120e,
> +};

...

> + ret = regmap_read_poll_timeout(adc_dev->regmap, pdata->regs[PMIC_AUXADC_IMP0],
> + val, !!(val & MT6358_IMP0_IRQ_RDY),
> + IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
> + if (ret) {
> + mt6358_stop_imp_conv(adc_dev);

> + return ret;
> + }
> +
> + return 0;

if (ret)
foo()

return ret;

...

> + int val_v, ret;

Why is val_v signed?

...

> + int val_v, val_i, ret;

Ditto for all val_*.

...

> + /* If it succeeded, wait for the registers to be populated */
> + usleep_range(IMP_STOP_DELAY_US, IMP_STOP_DELAY_US + 50);

fsleep() ?

...

> + /* Assert ADC reset */
> + regmap_set_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);

No required delay in between?

> + /* De-assert ADC reset */
> + regmap_clear_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);

...

> + /* Wait until all samples are averaged */
> + usleep_range(desc->num_samples * AUXADC_AVG_TIME_US,
> + (desc->num_samples + 1) * AUXADC_AVG_TIME_US);

fsleep()

...

> + ret = regmap_read_poll_timeout(regmap,
> + (pdata->regs[PMIC_AUXADC_ADC0] +
> + PMIC_AUXADC_ADCx(chan->address)),

Drop unneeded parentheses and possibly make it one line.

> + val, (val & PMIC_AUXADC_RDY_BIT),

Unneeded parentheses.

> + AUXADC_POLL_DELAY_US, AUXADC_TIMEOUT_US);
> + if (ret)
> + return ret;

...

> + mutex_lock(&adc_dev->lock);

Why not use cleanup.h?

...

> +static int mt6359_auxadc_probe(struct platform_device *pdev)
> +{

struct device *dev = &pdev->dev;

> + struct device *mt6397_mfd_dev = pdev->dev.parent;

... = dev->parent;

> + struct mt6359_auxadc *adc_dev;
> + struct iio_dev *indio_dev;
> + struct regmap *regmap;
> + int ret;
> +
> + /* Regmap is from SoC PMIC Wrapper, parent of the mt6397 MFD */
> + regmap = dev_get_regmap(mt6397_mfd_dev->parent, NULL);
> + if (!regmap)
> + return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get regmap\n");
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + adc_dev = iio_priv(indio_dev);
> + adc_dev->regmap = regmap;
> + adc_dev->dev = &pdev->dev;
> +
> + adc_dev->pdata = device_get_match_data(&pdev->dev);
> + if (!adc_dev->pdata)
> + return -EINVAL;
> +
> + mutex_init(&adc_dev->lock);
> +
> + mt6359_auxadc_reset(adc_dev);
> +
> + indio_dev->dev.parent = &pdev->dev;
> + indio_dev->name = dev_name(&pdev->dev);
> + indio_dev->info = &mt6359_auxadc_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = adc_dev->pdata->channels;
> + indio_dev->num_channels = adc_dev->pdata->num_channels;
> +
> + ret = devm_iio_device_register(&pdev->dev, indio_dev);
> + if (ret < 0)

Why ' < 0' ?

> + return dev_err_probe(&pdev->dev, ret, "failed to register iio device\n");
> +
> + return 0;
> +}

--
With Best Regards,
Andy Shevchenko