From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755027AbcIEGs6 (ORCPT ); Mon, 5 Sep 2016 02:48:58 -0400 Received: from down.free-electrons.com ([37.187.137.238]:58399 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753023AbcIEGsy (ORCPT ); Mon, 5 Sep 2016 02:48:54 -0400 Subject: Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC To: Peter Meerwald-Stadler References: <1472738705-20145-1-git-send-email-quentin.schulz@free-electrons.com> <1472738705-20145-4-git-send-email-quentin.schulz@free-electrons.com> Cc: jic23@kernel.org, knaack.h@gmx.de, lars@metafoo.de, maxime.ripard@free-electrons.com, thomas.petazzoni@free-electrons.com, antoine.tenart@free-electrons.com, linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org From: Quentin Schulz Message-ID: Date: Mon, 5 Sep 2016 08:48:51 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/09/2016 18:12, Peter Meerwald-Stadler wrote: > >> The Allwinner SoCs all have an ADC that can also act as a touchscreen >> controller and a thermal sensor. This patch adds the ADC driver which is >> based on the MFD for the same SoCs ADC. > > nitpicking ahead > [...] >> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c >> new file mode 100644 >> index 0000000..a93d36d >> --- /dev/null >> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c >> @@ -0,0 +1,525 @@ >> +/* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC >> + * >> + * Copyright (c) 2016 Quentin Schulz > > email address is incomplete > As in my other patches, thanks! >> + * >> + * This program is free software; you can redistribute it and/or modify it under >> + * the terms of the GNU General Public License version 2 as published by the >> + * Free Software Foundation. >> + * >> + * The Allwinner SoCs all have an ADC that can also act as a touchscreen >> + * controller and a thermal sensor. >> + * The thermal sensor works only when the ADC acts as a touchscreen controller >> + * and is configured to throw an interrupt every fixed periods of time (let say >> + * every X seconds). >> + * One would be tempted to disable the IP on the hardware side rather than >> + * disabling interrupts to save some power but that reset the internal clock of > > resets > >> + * the IP, resulting in having to wait X seconds every time we want to read the >> + * value of the thermal sensor. >> + * This is also the reason of using autosuspend in pm_runtime. If there were no > > there was no > [...] >> + >> +const unsigned int sun4i_gpadc_chan_select(unsigned int chan) > > static instead of const? > static const then? >> +{ >> + return SUN4I_GPADC_CTRL1_ADC_CHAN_SELECT(chan); >> +} >> + >> +const unsigned int sun6i_gpadc_chan_select(unsigned int chan) > > static instead of const? > static const then? >> +{ >> + return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan); >> +} >> + >> +struct soc_specific { >> + const int temp_offset; > > wondering why you constify every member? > Because they're supposed to be fixed values? It won't change in runtime. Is there any reason why I shouldn't do that? >> + const int temp_scale; >> + const unsigned int tp_mode_en; >> + const unsigned int tp_adc_select; >> + const unsigned int (*adc_chan_select)(unsigned int chan); >> +}; [...] >> +static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int *val, >> + unsigned int irq) >> +{ >> + struct sun4i_gpadc_dev *info = iio_priv(indio_dev); >> + int ret = 0; >> + >> + pm_runtime_get_sync(indio_dev->dev.parent); >> + mutex_lock(&info->mutex); >> + >> + reinit_completion(&info->completion); >> + >> + regmap_write(info->regmap, SUN4I_GPADC_INT_FIFOC, >> + SUN4I_GPADC_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) | >> + SUN4I_GPADC_INT_FIFOC_TP_FIFO_FLUSH); > > check retval? here and elsewhere for regmap_write() > ACK. What should I do with the retval then? There are some in: - sun4i_gpadc_read called for read_raws => return which error code (or -ret only?)? - sun4i_gpadc_runtime_suspend => return value of ret, but that would cancel the suspend right? - sun4i_gpadc_runtime_resume => return value of ret, but that would cancel resume right? - sun4i_gpadc_probe in the error gotos => probe already failing so do we actually care if regmap_update_bits fails? If yes, what's the expected behaviour? - sun4i_gpadc_remove => return value of ret, but that would mean the remove failed right? [...] >> +static int sun4i_gpadc_read_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, int *val, >> + int *val2, long mask) >> +{ >> + int ret; >> + >> + switch (mask) { >> + case IIO_CHAN_INFO_OFFSET: >> + ret = sun4i_gpadc_temp_offset(indio_dev, val); >> + if (ret) >> + return ret; >> + >> + return IIO_VAL_INT; >> + case IIO_CHAN_INFO_RAW: >> + if (chan->type == IIO_VOLTAGE) { >> + ret = sun4i_gpadc_adc_read(indio_dev, chan->channel, >> + val); >> + if (ret) >> + return ret; > > could share the "if (ret) return ret;" between code path > ACK. [...] >> +static int sun4i_irq_init(struct platform_device *pdev, const char *name, >> + irq_handler_t handler, const char *devname, >> + unsigned int *irq, atomic_t *atomic) >> +{ >> + int ret; >> + struct sun4i_gpadc_mfd_dev *mfd_dev = dev_get_drvdata(pdev->dev.parent); >> + struct sun4i_gpadc_dev *info = iio_priv(dev_get_drvdata(&pdev->dev)); >> + >> + /* >> + * Once the interrupt is activated, the IP continuously performs >> + * conversions thus throws interrupts. The interrupt is activated right >> + * after being requested but we want to control when these interrupts >> + * occurs thus we disable it right after being requested. However, an > > occur > ACK for all typos. Thanks! Quentin