Hi,
On Thu, Feb 20, 2014 at 06:46:26PM +0100, Lars-Peter Clausen wrote:
> On 02/20/2014 05:21 PM, Markus Pargmann wrote:
> >This is a conversion queue driver for the mx25 SoC. It uses the central
> >ADC which is used by two seperate independent queues. This driver
> >prepares different conversion configurations for each possible input.
> >For a conversion it creates a conversionqueue of one item with the
> >correct configuration for the chosen channel. It then executes the queue
> >once and disables the conversion queue afterwards.
> >
> >The reference voltages are configurable through devicetree subnodes,
> >depending on the connections of the ADC inputs.
> >
> >Signed-off-by: Markus Pargmann <mpa@xxxxxxxxxxxxxx>
>
> Looks good mostly. Just a couple of minor code-style issues. Try to
> run checkpatch, sparse and friends on your driver before submission.
> They catch most of the stuff that needs to be fixed in this driver.
I used checkpatch and will have a look at sparse.
>
> [..]
> >diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> >index 2209f28..a421445 100644
> >--- a/drivers/iio/adc/Kconfig
> >+++ b/drivers/iio/adc/Kconfig
> >@@ -113,6 +113,13 @@ config EXYNOS_ADC
> > of SoCs for drivers such as the touchscreen and hwmon to use to share
> > this resource.
> >
> >+config MX25_ADC
> >+ tristate "Freescale MX25 ADC driver"
> >+ depends on MFD_MX25_TSADC
> >+ help
> >+ Generic Conversion Queue driver used for general purpose ADC in the
> >+ MX25. This driver supports single measurements using the MX25 ADC.
> >+
>
> alphabetical order
I changed the config option name to "FSL_MX25_ADC" to have it in
alphabetical order for the user visible menuconfig and the config
symbols.
>
> > config LP8788_ADC
> > bool "LP8788 ADC driver"
> > depends on MFD_LP8788
> >diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> >index ba9a10a..63daa2c 100644
> >--- a/drivers/iio/adc/Makefile
> >+++ b/drivers/iio/adc/Makefile
> >@@ -22,3 +22,4 @@ obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
> > obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
> > obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
> > obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
> >+obj-$(CONFIG_MX25_ADC) += fsl-imx25-gcq.o
>
> Here too
Fixed.
>
> [...]
>
> >+const struct iio_chan_spec mx25_gcq_channels[MX25_NUM_CFGS] = {
>
> static
Fixed.
>
> >+ MX25_IIO_CHAN(0, "xp"),
> >+ MX25_IIO_CHAN(1, "yp"),
> >+ MX25_IIO_CHAN(2, "xn"),
> >+ MX25_IIO_CHAN(3, "yn"),
> >+ MX25_IIO_CHAN(4, "wiper"),
> >+ MX25_IIO_CHAN(5, "inaux0"),
> >+ MX25_IIO_CHAN(6, "inaux1"),
> >+ MX25_IIO_CHAN(7, "inaux2"),
> >+};
> >+
>
> >+struct iio_info mx25_gcq_iio_info = {
>
> static const
Fixed.
>
> >+ .read_raw = mx25_gcq_read_raw,
> >+};
> >+
> >+static struct regmap_config mx25_gcq_regconfig = {
>
> const
Fixed.
>
> >+ .fast_io = true,
>
> You don't need to set fast_io since this is already done at the
> regmap_bus level.
Okay, I removed it.
>
> >+ .max_register = 0x5c,
> >+ .reg_bits = 32,
> >+ .val_bits = 32,
> >+ .reg_stride = 4,
> >+};
> >+
> >+static void mx25_gcq_iio_dev_setup(struct platform_device *pdev,
> >+ struct iio_dev *idev)
> >+{
> >+ idev->dev.parent = &pdev->dev;
> >+ idev->channels = mx25_gcq_channels;
> >+ idev->num_channels = ARRAY_SIZE(mx25_gcq_channels);
> >+ idev->info = &mx25_gcq_iio_info;
>
> Any reason why this needs to be in a separate function and can't be
> inlined in probe()?
No reason for that. I moved it back to probe().
>
> >+}
> >+
> >+static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
> >+ struct mx25_gcq_priv *priv)
> >+{
> >+ struct device_node *np = pdev->dev.of_node;
> >+ struct device_node *child;
> >+ struct device *dev = &pdev->dev;
> >+ int ret;
> >+ int i;
> >+
> >+ /* Setup all configurations registers with a default conversion
> >+ * configuration for each input */
> >+ for (i = 0; i != MX25_NUM_CFGS; ++i)
> >+ regmap_write(priv->regs, MX25_ADCQ_CFG(i),
> >+ MX25_ADCQ_CFG_YPLL_OFF |
> >+ MX25_ADCQ_CFG_XNUR_OFF |
> >+ MX25_ADCQ_CFG_XPUL_OFF |
> >+ MX25_ADCQ_CFG_REFP_INT |
> >+ (i << 4) |
> >+ MX25_ADCQ_CFG_REFN_NGND2);
> >+
> >+ for_each_child_of_node(np, child) {
> >+ u32 reg;
> >+ u32 refn;
> >+ u32 refp;
> >+
> >+ ret = of_property_read_u32(child, "reg", ®);
> >+ if (ret) {
> >+ dev_err(dev, "Failed to get reg property\n");
> >+ return ret;
> >+ }
> >+ if (reg > MX25_NUM_CFGS) {
> >+ dev_err(dev, "reg value is greater than the number of available configuration registers\n");
> >+ return -EINVAL;
> >+ }
> >+
> >+ ret = of_property_read_u32(child, "fsl,adc-refn", &refn);
> >+ if (ret) {
> >+ dev_err(dev, "Failed to get fsl,adc-refn property\n");
> >+ return ret;
> >+ }
> >+
> >+ ret = of_property_read_u32(child, "fsl,adc-refp", &refp);
> >+ if (ret) {
> >+ dev_err(dev, "Failed to get fsl,adc-refp property\n");
> >+ return ret;
> >+ }
>
> Range check for refp and refn?
Range check added for both.
>
> >+
> >+ regmap_update_bits(priv->regs, MX25_ADCQ_CFG(reg),
> >+ MX25_ADCQ_CFG_REFP_MASK |
> >+ MX25_ADCQ_CFG_REFN_MASK,
> >+ (refp << 7) | (refn << 2));
> >+ }
> >+ regmap_update_bits(priv->regs, MX25_ADCQ_CR,
> >+ MX25_ADCQ_CR_FRST | MX25_ADCQ_CR_QRST,
> >+ MX25_ADCQ_CR_FRST | MX25_ADCQ_CR_QRST);
> >+
> >+ regmap_write(priv->regs, MX25_ADCQ_CR,
> >+ MX25_ADCQ_CR_PDMSK |
> >+ MX25_ADCQ_CR_QSM_FQS);
> >+
> >+ return 0;
> >+}
> >+
> >+static int mx25_gcq_probe(struct platform_device *pdev)
> >+{
> >+ struct iio_dev *idev;
> >+ struct mx25_gcq_priv *priv;
> >+ struct resource *res;
> >+ struct device *dev = &pdev->dev;
> >+ int ret;
> >+ int irq;
> >+ void __iomem *mem;
> >+
> >+ idev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
> >+ if (!idev)
> >+ return -ENOMEM;
> >+
> >+ priv = iio_priv(idev);
> >+
> >+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >+ mem = devm_ioremap_resource(dev, res);
> >+ if (!mem) {
> >+ dev_err(dev, "Failed to get iomem");
>
> devm_ioremap_resource already prints a error message for you
Okay, I removed dev_err.
>
> >+ return -ENOMEM;
> >+ }
> >+
> >+ priv->regs = devm_regmap_init_mmio(dev, mem, &mx25_gcq_regconfig);
> >+ if (IS_ERR(priv->regs)) {
> >+ dev_err(dev, "Failed to initialize regmap\n");
> >+ return PTR_ERR(priv->regs);
> >+ }
> >+
> >+ irq = platform_get_irq(pdev, 0);
> >+ if (irq < 0) {
>
> 0 is not a valid IRQ number either
Fixed.
>
> >+ dev_err(dev, "Failed to get IRQ\n");
> >+ return irq;
> >+ }
> >+
> >+ ret = devm_request_irq(dev, irq, mx25_gcq_irq, IRQF_ONESHOT, pdev->name,
> >+ priv);
>
> IRQF_ONESHOT does not make much sense for non-threaded IRQs. Also it
> is probably safer to use the non managed variant of request_irq
> here.
Right, I removed the IRQF_ONESHOT flag. Why is it safer to use the non
managed request_irq?
>
> >+ if (ret) {
> >+ dev_err(dev, "Failed requesting IRQ\n");
>
> It usually makes sense to include the error number in the message.
> Same for the other error messages in probe.
The error values are returned from the probe function, so the error
number should be displayed by really_probe() in drivers/base/dd.c .
>
> >+ return ret;
> >+ }
> >+
> >+ ret = mx25_gcq_setup_cfgs(pdev, priv);
> >+ if (ret)
> >+ return ret;
> >+
> >+ mx25_gcq_iio_dev_setup(pdev, idev);
> >+
> >+ ret = iio_device_register(idev);
> >+ if (ret) {
> >+ dev_err(dev, "Failed to register iio device\n");
> >+ return ret;
> >+ }
> >+
> >+ init_completion(&priv->completed);
>
> Should be done before the interrupt handler is registered. You
> reference it in there.
>
> >+
> >+ priv->clk = mx25_tsadc_get_ipg(pdev->dev.parent);
> >+ ret = clk_prepare_enable(priv->clk);
>
> The clock should probably also be enabled before the interrupt
> handler and the IIO device are registered.
Fixed. The last two items are request_irq() and iio_device_register().
>
> >+ if (ret) {
> >+ dev_err(dev, "Failed to enable clock\n");
> >+ return ret;
> >+ }
> >+
> >+ platform_set_drvdata(pdev, priv);
> >+
> >+ return 0;
> >+}
> >+
> >+static int mx25_gcq_remove(struct platform_device *pdev)
> >+{
> >+ struct mx25_gcq_priv *priv = platform_get_drvdata(pdev);
> >+
>
> iio_device_unregister().
Fixed.
>
> >+ clk_disable_unprepare(priv->clk);
> >+
> >+ return 0;
> >+}
>
>
Thanks,
Markus
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: Digital signature
