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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux