On Mon, 19 Dec 2022 18:13:34 +0800
haibo.chen@xxxxxxx wrote:
> From: Haibo Chen <haibo.chen@xxxxxxx>
>
> The ADC in i.mx93 is a total new ADC IP, add a driver to support
> this ADC.
>
> Currently, only support one shot normal conversion triggered by
> software. For other mode, will add in future.
>
> Signed-off-by: Haibo Chen <haibo.chen@xxxxxxx>
Hi Haibo,
A few minor things inline. Otherwise looks good to me.
Thanks,
Jonathan
> ---
> diff --git a/drivers/iio/adc/imx93_adc.c b/drivers/iio/adc/imx93_adc.c
> new file mode 100644
> index 000000000000..3ea16a70e746
> --- /dev/null
> +++ b/drivers/iio/adc/imx93_adc.c
> @@ -0,0 +1,474 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * NXP i.MX93 ADC driver
> + *
> + * Copyright 2022 NXP
> + */
> +
...
> +/* ADC bit shift */
> +#define IMX93_ADC_MCR_MODE_MASK BIT(29)
> +#define IMX93_ADC_MCR_NSTART_MASK BIT(24)
> +#define IMX93_ADC_MCR_CALSTART_MASK BIT(14)
> +#define IMX93_ADC_MCR_ADCLKSE_MASK BIT(8)
> +#define IMX93_ADC_MCR_PWDN_MASK BIT(0)
> +#define IMX93_ADC_MSR_CALFAIL_MASK BIT(30)
> +#define IMX93_ADC_MSR_CALBUSY_MASK BIT(29)
> +#define IMX93_ADC_MSR_ADCSTATUS_MASK GENMASK(2, 0)
> +#define IMX93_ADC_ISR_ECH_MASK BIT(0)
> +#define IMX93_ADC_ISR_EOC_MASK BIT(1)
> +#define IMX93_ADC_ISR_EOC_ECH_MASK (IMX93_ADC_ISR_EOC_MASK | \
> + IMX93_ADC_ISR_ECH_MASK)
> +#define IMX93_ADC_IMR_JEOC_MASK BIT(3)
> +#define IMX93_ADC_IMR_JECH_MASK BIT(2)
> +#define IMX93_ADC_IMR_EOC_MASK BIT(1)
> +#define IMX93_ADC_IMR_ECH_MASK BIT(0)
> +#define IMX93_ADC_PCDR_CDATA_MASK GENMASK(11, 0)
> +
> +/* ADC status */
Are these field values? If so I'd like the naming to indicate
which field and which register etc. It might end up a bit
long, but it makes it clear the value is matched with
where we are getting it from.
> +#define IMX93_ADC_IDLE 0
> +#define IMX93_ADC_POWER_DOWN 1
> +#define IMX93_ADC_WAIT_STATE 2
> +#define IMX93_ADC_BUSY_IN_CALIBRATION 3
> +#define IMX93_ADC_SAMPLE 4
> +#define IMX93_ADC_CONVERSION 6
> +
> +#define IMX93_ADC_TIMEOUT msecs_to_jiffies(100)
> +static int imx93_adc_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct imx93_adc *adc = iio_priv(indio_dev);
> + struct device *dev = adc->dev;
> + long ret;
> + u32 vref_uv;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + pm_runtime_get_sync(dev);
> + mutex_lock(&adc->lock);
> + ret = imx93_adc_read_channel_conversion(adc, chan->channel, val);
> + mutex_unlock(&adc->lock);
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_sync_autosuspend(dev);
> + if (ret > 0)
> + return IIO_VAL_INT;
> + else
> + return ret;
Prefer the error out of line. Makes for slightly easier reviewing as it
is the more common pattern.
if (ret < 0)
return ret;
return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + ret = vref_uv = regulator_get_voltage(adc->vref);
> + if (ret < 0)
> + return ret;
> + *val = vref_uv / 1000;
> + *val2 = 12;
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = clk_get_rate(adc->ipg_clk);
> + return IIO_VAL_INT;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int imx93_adc_probe(struct platform_device *pdev)
> +{
> +
> + platform_set_drvdata(pdev, indio_dev);
> +
> + init_completion(&adc->completion);
> +
> + indio_dev->name = IMX93_ADC_DRIVER_NAME;
I'd rather see the string here than use a define. There is no strong
reason that this should be the driver name - so I don' want to have
to go look at the define to check it contains "imx93" which I'd expect
to see here.
> +
> +static int imx93_adc_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct imx93_adc *adc = iio_priv(indio_dev);
> + struct device *dev = adc->dev;
> +
> + /* adc power down need clock on */
> + pm_runtime_get_sync(dev);
> + imx93_adc_power_down(adc);
> +
> + pm_runtime_disable(dev);
> + pm_runtime_put_noidle(dev);
> + iio_device_unregister(indio_dev);
You don't remove the userspace interfaces until this iio_device_unregister()
call. Having turned the power off before this point is probably not a good idea.
you should be fine moving the imx93_adc_power_down() after this point.
Hopefully that should reflect any equivalent handling order in probe()
(I haven't checked!)
> + free_irq(adc->irq, adc);
> + clk_disable_unprepare(adc->ipg_clk);
> + regulator_disable(adc->vref);
> +
> + return 0;
> +}
> +
