> -----Original Message----- > From: Jonathan Cameron <jic23@xxxxxxxxxx> > Sent: 2022年8月7日 0:03 > To: Bough Chen <haibo.chen@xxxxxxx> > Cc: lars@xxxxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; > shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; > festevam@xxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; > linux-iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 1/3] iio: adc: add imx93 adc support > > On Wed, 3 Aug 2022 17:12:25 +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> > > Nice. > > A few comments inline. > > Thanks, > > Jonathan > > > > diff --git a/drivers/iio/adc/imx93_adc.c b/drivers/iio/adc/imx93_adc.c > > new file mode 100644 index 000000000000..6e3cf6d3e629 > > --- /dev/null > > +++ b/drivers/iio/adc/imx93_adc.c > > @@ -0,0 +1,448 @@ > > .. . ... > > +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; > > + > > + pm_runtime_get_sync(dev); > > I would prefer to see the runtime pm tear down all up here as it corresponds > to the setup at the end of probe() I don't it will make any functional difference > and it makes the code easier to reason about in terms of ordering of calls. Hi Jonathan, I can't get your point here, you mean call pm_runtime_disable and pm_runtime_put_noidle here? I get sync the runtime pm here, because there are register operation in imx93_adc_power_down(), need to make sure clock is on. Best Regards Haibo Chen > > > + > > + iio_device_unregister(indio_dev); > > + imx93_adc_power_down(adc); > > + clk_disable_unprepare(adc->ipg_clk); > > + regulator_disable(adc->vref); > > + > > + pm_runtime_disable(dev); > > + pm_runtime_put_noidle(dev); > > + > > + return 0; > > +} > > +