Re: [PATCH v1 2/4] iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC
From: Andy Shevchenko
Date: Thu May 30 2024 - 09:35:17 EST
On Thu, May 30, 2024 at 12:34 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxx> 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