Re: [PATCH] iio: adc: add support for Intel ADC

[Felipe Balbi <felipe.balbi@linux.intel.com>]

Hi,

Jonathan Cameron <jonathan.cameron@huawei.com> writes:
>> >> +static int intel_adc_read_raw(struct iio_dev *iio,
>> >> +		struct iio_chan_spec const *channel, int *val, int *val2,
>> >> +		long mask)
>> >> +{
>> >> +	struct intel_adc *adc = iio_priv(iio);
>> >> +	int shift;
>> >> +	int ret;
>> >> +
>> >> +	switch (mask) {
>> >> +	case IIO_CHAN_INFO_RAW:
>> >> +		shift = channel->scan_type.shift;
>> >> +
>> >> +		ret = iio_device_claim_direct_mode(iio);
>> >> +		if (ret)
>> >> +			break;
>> >> +
>> >> +		intel_adc_enable(adc);
>> >> +
>> >> +		ret = intel_adc_single_channel_conversion(adc, channel, val);
>> >> +		if (ret) {
>> >> +			intel_adc_disable(adc);
>> >> +			iio_device_release_direct_mode(iio);
>> >> +			break;  
>> >
>> > nitpick (feel free to ignore).
>> > It might be nice to pull this case block as a separate function, then you
>> > could cleanly use goto to do the unwinding.  
>> 
>> you mean something like below:
>> 
>> static int intel_adc_read_info_raw(...)
>> {
>> 	....
>> }
>> 
>> static int intel_adc_read_raw(...)
>> {
>> 	switch (mask) {
>>         case IIO_CHAN_INFO_RAW:
>>         	ret = intel_adc_read_info_raw(...);
>>                 break;
>>         default:
>>         	ret = -EINVAL;
>>         }
>> }
>> 
>> ??
>
> Yes, exactly that.

I'll change it, no worries.

>> >> +	ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
>> >> +	if (ret < 0)
>> >> +		return ret;
>> >> +
>> >> +	irq = pci_irq_vector(pci, 0);
>> >> +	ret = devm_request_irq(&pci->dev, irq, intel_adc_irq,
>> >> +			IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_RISING,
>> >> +			"intel-adc", adc);  
>> >
>> > Requesting the interrupt only after exposing userspace and in kernel
>> > interfaces seems liable to cause problem.  
>> 
>> It goes the other way around, rather. If I request the interrupt before,
>> then I could get interrupts before IIO subsystem knows about the device,
>> no?
>
> Only if your device comes up with interrupts already enabled.  Normally they
> only turn on in response to some userspace interaction, such as enabling
> a threshold. Unless there is a hardware limitation, then at startup no
> such interrupt sources should be enabled.

We have FW that _may_ use the hardware and leave it at unpredictable
state. There is a potential for irq status bits being left over by
FW.


>> >> +	if (ret)
>> >> +		goto err;
>> >> +
>> >> +	pm_runtime_set_autosuspend_delay(&pci->dev, 1000);
>> >> +	pm_runtime_use_autosuspend(&pci->dev);
>> >> +	pm_runtime_put_autosuspend(&pci->dev);
>> >> +	pm_runtime_allow(&pci->dev);
>> >> +
>> >> +	return 0;
>> >> +
>> >> +err:
>> >> +	pci_free_irq_vectors(pci);
>> >> +	return ret;
>> >> +}
>> >> +
>> >> +static void intel_adc_remove(struct pci_dev *pci)
>> >> +{
>> >> +	pm_runtime_forbid(&pci->dev);
>> >> +	pm_runtime_get_noresume(&pci->dev);
>> >> +
>> >> +	pci_free_irq_vectors(pci);  
>> >
>> > There is a theoretical race here.  We have freed the irq vectors
>> > before removing the userspace and in kernel interfaces.  
>> 
>> There's no way to sort this out, though. Is there? Apart from switching
>> away from device managed resources.
>
> There is the rather helpful,
>
> devm_add_action_or_reset() that allows you to define additional cleanup
> actions to be automatically run.  It's either that, or stop using
> device managed resources from the point at which something that isn't
> device managed occurs in probe.

I'll have a look, thanks.

-- 
balbi