Hi,
Jonathan Cameron <jonathan.cameron@xxxxxxxxxx> writes:
>> diff --git a/drivers/iio/adc/intel-adc.c b/drivers/iio/adc/intel-adc.c
>> new file mode 100644
>> index 000000000000..381958668563
>> --- /dev/null
>> +++ b/drivers/iio/adc/intel-adc.c
>> @@ -0,0 +1,482 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/**
>> + * intel-adc.c - Intel ADC Driver
>> + *
>> + * Copyright (C) 2018 Intel Corporation
>> + *
>> + * Author: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
>> + */
>> +
>> +#include <linux/completion.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/iio/buffer.h>
>
> You aren't currently supporting the buffered interface
> or triggers so a few headers to clean out.
removed
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/slab.h>
>> +#include <linux/sysfs.h>
>> +
>> +#define PCI_DEVICE_ID_INTEL_EHLLP 0x4bb8
>
> Perhaps just put this inline as it's obvious what it is from
> context so doesn't really need a 'name'.
removed
>> +/* ADC Interrupt Mask Register */
>> +#define ADC_INTR_LOOP_DONE_INTR BIT(22)
>> +#define ADC_INTR_FIFO_EMPTY_INTR BIT(21)
>> +#define ADC_INTR_DMA_DONE_INTR BIT(20)
>> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_7 BIT(19)
>> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_7 BIT(18)
>> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_6 BIT(17)
>> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_6 BIT(16)
>> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_5 BIT(15)
>> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_5 BIT(14)
>> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_4 BIT(13)
>> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_4 BIT(12)
>> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_3 BIT(11)
>> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_3 BIT(10)
>> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_2 BIT(9)
>> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_2 BIT(8)
>> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_1 BIT(7)
>> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_1 BIT(6)
>> +#define ADC_INTR_DATA_THRSHLD_LOW_INTR_0 BIT(5)
>> +#define ADC_INTR_DATA_THRSHLD_HIGH_INTR_0 BIT(4)
>> +#define ADC_INTR_PWR_DWN_EXIT_INTR BIT(3)
>> +#define ADC_INTR_FIFO_FULL_INTR BIT(2)
>> +#define ADC_INTR_SMPL_DONE_INTR BIT(0)
>
> Seems to be a mixture of aligned spacing and non aligned.
> I don't mind which, but consistency is good.
I did it like this because otherwise I would need another tab for all
defines and some of them would cross 80-columns. I can change, no
worries, just let me know.
>> +#define ADC_INTR_ALL_MASK (ADC_INTR_LOOP_DONE_INTR | \
>> + ADC_INTR_FIFO_EMPTY_INTR | \
>> + ADC_INTR_DMA_DONE_INTR | \
>> + ADC_INTR_DATA_THRSHLD_LOW_INTR_7 | \
>> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_7 | \
>> + ADC_INTR_DATA_THRSHLD_LOW_INTR_6 | \
>> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_6 | \
>> + ADC_INTR_DATA_THRSHLD_LOW_INTR_5 | \
>> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_5 | \
>> + ADC_INTR_DATA_THRSHLD_LOW_INTR_4 | \
>> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_4 | \
>> + ADC_INTR_DATA_THRSHLD_LOW_INTR_3 | \
>> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_3 | \
>> + ADC_INTR_DATA_THRSHLD_LOW_INTR_2 | \
>> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_2 | \
>> + ADC_INTR_DATA_THRSHLD_LOW_INTR_1 | \
>> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_1 | \
>> + ADC_INTR_DATA_THRSHLD_LOW_INTR_0 | \
>> + ADC_INTR_DATA_THRSHLD_HIGH_INTR_0 | \
>> + ADC_INTR_PWR_DWN_EXIT_INTR | \
>> + ADC_INTR_FIFO_FULL_INTR | \
>> + ADC_INTR_SMPL_DONE_INTR)
>> +
>> +#define ADC_VREF_UV 1600000 /* uV */
>
> Units are in the define name (which is nice btw) so probably no need for
> the comment.
>
>> +#define ADC_DEFAULT_CONVERSION_TIMEOUT 5000 /* ms */
>
> Give this one explicit units in it's naming as well.
done
> The ADC prefix is a bit generic, but I suppose it's unlikely to get
> used in standard headers etc...
okay
>> +
>> +struct intel_adc {
>> + struct completion completion;
>> + struct pci_dev *pci;
>> + struct iio_dev *iio;
>
> As noted below, this pointer appears unused. I'm not sure the
> pci one is used either...
removed both
>> +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;
}
}
??
>> +#define INTEL_ADC_DIFF_CHAN(c1, c2) \
>> +{ \
>> + .type = IIO_VOLTAGE, \
>> + .differential = true, \
>> + .indexed = 1, \
>> + .channel = (c1), \
>> + .channel2 = (c2), \
>> + .scan_index = (c1), \
>
> I think we get overlapping index values between these and
> the SINGLE_CHAN ones. These should be unique.
>
> Also, without buffered interface support they don't actually
> do anything so drop them for now. Same with scan_type.
removed
>> +#define INTEL_ADC_SINGLE_CHAN(c) \
>> +{ \
>> + .type = IIO_VOLTAGE, \
>> + .indexed = 1, \
>> + .channel = (c), \
>> + .scan_index = (c), \
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> + .scan_type = { \
>> + .sign = 's', \
>> + .realbits = 14, \
>> + .storagebits = 32, \
>> + .shift = 0, \
>
> No need to specify shift of 0 as that's the 'obviousish' default.
removed
>> +static int intel_adc_probe(struct pci_dev *pci, const struct pci_device_id *id)
>> +{
>> + struct intel_adc *adc;
>> + struct iio_dev *iio;
>> + int ret;
>> + int irq;
>> +
>> + iio = devm_iio_device_alloc(&pci->dev, sizeof(*adc));
>> + if (!iio)
>> + return -ENOMEM;
>> +
>> + adc = iio_priv(iio);
>> + adc->pci = pci;
>> + adc->iio = iio;
>
> This pointer look usually means that the driver could be slightly
> adjusted to remove the need to go from iio_dev -> private
> and private-> iio_dev.
>
> In this case I can't find a user of adc->iio so get rid of it.
removed
>> + 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?
>> + 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.
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int intel_adc_suspend(struct device *dev)
>> +{
>
> Why provide empty sleep and resume functions?
no reason, removed.
>> + return 0;
>> +}
>> +
>> +static int intel_adc_resume(struct device *dev)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(intel_adc_pm_ops, intel_adc_suspend, intel_adc_resume);
then removed this
>> +static const struct pci_device_id intel_adc_id_table[] = {
>> + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_EHLLP), },
>> + { } /* Terminating Entry */
>> +};
>> +MODULE_DEVICE_TABLE(pci, intel_adc_id_table);
>> +
>> +static struct pci_driver intel_adc_driver = {
>> + .name = "intel-adc",
>> + .probe = intel_adc_probe,
>> + .remove = intel_adc_remove,
>> + .id_table = intel_adc_id_table,
>> + .driver = {
>> + .pm = &intel_adc_pm_ops,
>
> .pm should be indented one more level.
and this
--
balbi
