Message-ID: <578DE5C9.8070502@free-electrons.com>
Date:	Tue, 19 Jul 2016 10:33:13 +0200
From:	Quentin Schulz <quentin.schulz@...e-electrons.com>
To:	Jonathan Cameron <jic23@...nel.org>, jdelvare@...e.com,
	linux@...ck-us.net, knaack.h@....de, lars@...afoo.de,
	pmeerw@...erw.net, maxime.ripard@...e-electrons.com, wens@...e.org,
	lee.jones@...aro.org
Cc:	linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
	linux-iio@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	thomas.petazzoni@...e-electrons.com,
	antoine.tenart@...e-electrons.com
Subject: Re: [PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC

On 18/07/2016 15:18, Jonathan Cameron wrote:
> On 15/07/16 10:59, Quentin Schulz 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.
>>
>> This also registers the thermal adc channel in the iio map array so
>> iio_hwmon could use it without modifying the Device Tree.
>>
>> This driver probes on three different platform_device_id to take into
>> account slight differences between Allwinner SoCs ADCs.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@...e-electrons.com>
> 
> Hi Quentin,
> 
> Various bits inline.  In particular the irq handling looks flakey / racey
> to me.  Definitely need some explanatory comments.
> 
> Also another note on the craziness that using extended_name to provide
> the hwmon labels will cause :)
> 
> Jonathan
[...]
>> +struct sunxi_gpadc_dev {
>> +	void __iomem			*regs;
>> +	struct completion		completion;
>> +	int				temp_data;
>> +	u32				adc_data;
>> +	struct regmap			*regmap;
>> +	unsigned int			fifo_data_irq;
>> +	unsigned int			temp_data_irq;
>> +	unsigned int			flags;
> I'd prefer something more explicit than this.  Right now only one
> bit can ever be set - indicating a particular chip family.  Why not
> just have an enum instead?

ACK.

[...]
>> +static const struct iio_chan_spec sunxi_gpadc_channels[] = {
>> +	SUNXI_GPADC_ADC_CHANNEL(0, "adc_chan0"),
>> +	SUNXI_GPADC_ADC_CHANNEL(1, "adc_chan1"),
>> +	SUNXI_GPADC_ADC_CHANNEL(2, "adc_chan2"),
>> +	SUNXI_GPADC_ADC_CHANNEL(3, "adc_chan3"),
>> +	{
>> +		.type = IIO_TEMP,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> +		.datasheet_name = "temp_adc",
>> +		.extend_name = "SoC temperature",
> Just curious, did you look at the resultling sysfs entries?
> Going to be something like
> in_temp_SoC\ Temperature_input... Not good.

Just encountered this after further testing, not good as you said.

> If there is a strong enough reason (and there may be) to add a 'help string'
> type label to struct iio_chan_spec then my only real worries would be that
> we would be adding a whole pile of ABI that would be in the, near impossible
> to change in future, category...

I don't understand your "adding a whole pile of ABI" thing. Same as
datasheet_name variable: const char* in struct iio_chan_spec and used
whenever needed with some NULL checks. This is the easiest way to do it.
Or maybe you're thinking of adding an item to iio_chan_info_enum and all
its underlying modifications? This could allow us to expose a _label
sysfs file in the iio_device per type/channel.

I understand the "near impossible to change in future" concern though.

[...]
>> +	enable_irq(info->temp_data_irq);
> Is this hardware spitting out extra irqs?  If not, much better to just
> leave it enabled all the time and control whether it can occur or not
> by controlling the device state..

The temp_data_irq occurs every SUNXI_GPADC_TEMP_PERIOD(x) periods (in
the current state of the driver: 2s). What do you mean by controlling
the device state? Enabling or disabling the hardware part of the IP
responsible of getting the temperature
(SUNXI_GPADC_TP_TPR_TEMP_ENABLE(x) here)?
Once the interrupt is activated, the IP periodically performs
conversions. We don't really want interrupts to be activated when not
needed.

[...]
>> +
>> +	if (info->flags & SUNXI_GPADC_ARCH_SUN4I)
>> +		*val = info->temp_data * 133 - 257000;
> Why report as processed?  I'd just report them as raw with the scale
> and offset provided.  It's not a big thing, but if we can leave it so
> that the conversion only occurs when desired, why not?
> 
> For in kernel users, this all happen 'automagically' anyway ;)
> 

ACK.

[...]
>> +	mutex_unlock(&indio_dev->mlock);
> mlock has a very specific purpose - locking the state changes of
> between 'buffered' (push) and poll modes. Don't use it for anything else.
> In fact it's almost always a bug to access it directly at all.  We have
> nice wrappers now for checking and locking the access mode.
> Just have a local lock in your iio_priv structure.

ACK. There is still a lot of drivers using iio_dev mlock though.

[...]
>> +static irqreturn_t sunxi_gpadc_fifo_data_irq_handler(int irq, void *dev_id)
>> +{
>> +	struct sunxi_gpadc_dev *info = dev_id;
>> +	int ret;
>> +
>> +	ret = regmap_read(info->regmap, SUNXI_GPADC_TP_DATA, &info->adc_data);
>> +	if (ret == 0)
>> +		complete(&info->completion);
> if (!regmap_read(...))?
> 

ACK.

[...]
>> +
>> +	info->temp_data_irq = irq;
>> +	disable_irq(irq);
> As below, I want a comment explaining why you are disabling the irq here.
> This is clearly racey..

Once the interrupt is activated, the IP performs continuous conversions
(temp_data_irq only periodically). I want these interrupts to be enabled
only when I read the sysfs file or we get useless interrupts.
In the current state of this driver's irq handlers, I only set values in
structures and all the needed structures are already initialized before
requesting irqs. So it does not look like a race. I can prevent races in
future versions by adding an atomic flag if wanted.

>> +
>> +	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev,
>> +			"no FIFO_DATA_PENDING interrupt registered\n");
>> +		ret = irq;
>> +		goto err;
>> +	}
>> +
>> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
>> +					   sunxi_gpadc_fifo_data_irq_handler,
>> +					   0, "fifo_data", info);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev,
>> +			"could not request FIFO_DATA_PENDING interrupt: %d\n",
>> +			ret);
>> +		goto err;
>> +	}
>> +
>> +	info->fifo_data_irq = irq;
> There's a race here - this may be the only way of doing it though.
> 
> However, I want a comment explaining why here... Are we looking at a hardware
> bug?
>> +	disable_irq(irq);
>> +
>> +	ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "failed to register iio map array\n");
>> +		goto err;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, indio_dev);
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "could not register the device\n");
>> +		iio_map_array_unregister(indio_dev);
> Once you have an unwinding path (which is sensible here) put everything in
> that block - it makes it easier to review (initially I compared that block
> with the remove and thought you'd missed this error path).
> 

ACK. As suggested by Maxime, I will put that in a different goto label.

>> +		goto err;
>> +	}
>> +
>> +	return 0;
>> +
>> +err:
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);
>> +
>> +	return ret;
>> +}
>> +
>> +static int sunxi_gpadc_remove(struct platform_device *pdev)
>> +{
>> +	struct sunxi_gpadc_dev *info;
>> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +
>> +	info = iio_priv(indio_dev);
>> +	iio_device_unregister(indio_dev);
>> +	iio_map_array_unregister(indio_dev);
> Slight disrepancy here between the cleanup in the error path in
> probe vs what we have in remove (next line doesn't exist in probe cleanup)
> Perhaps a comment here would make it clear why...
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);

SUNXI_GPADC_TP_INT_FIFO is the register in charge of activating hardware
interrupts. I disable all interrupts. In the probe, the interrupts are
already disabled (with disable_irq(irq)). This is actually just a
shortcut compared to "disable_irq(info->fifo_data_irq);
disable_irq(inf->temp_data_irq);".

[...]

Powered by blists - more mailing lists