by Lars-Peter Clausen

[permalink] [raw]

Subject: Re: [PATCH v7 2/4] iio: adc: Add Xilinx AMS driver

On 10/19/21 5:20 PM, Anand Ashok Dumbre wrote:
> [....]
> +struct ams {
> + void __iomem *base;
> + void __iomem *ps_base;
> + void __iomem *pl_base;
> + struct clk *clk;
> + struct device *dev;
> + /* check kernel doc above */
> + struct mutex lock;
> + /* check kernel doc above */
Do we need this comment?
> + spinlock_t intr_lock;
> + unsigned int alarm_mask;
> + unsigned int masked_alarm;
> + u64 intr_mask;
> + int irq;
The irq field is only ever used in the probe function. Could be made a
local variable.
> + struct delayed_work ams_unmask_work;
> +};
> [...]
>
> +static void ams_enable_channel_sequence(struct iio_dev *indio_dev)
> +{
> + int i;
> + unsigned long long scan_mask;
> + struct ams *ams = iio_priv(indio_dev);
> +
> + /*
> + * Enable channel sequence. First 22 bits of scan_mask represent
> + * PS channels, and next remaining bits represent PL channels.
> + */
> +
> + /* Run calibration of PS & PL as part of the sequence */
> + scan_mask = 0x1 | BIT(AMS_PS_SEQ_MAX);
> + for (i = 0; i < indio_dev->num_channels; i++)
> + scan_mask |= BIT_ULL(indio_dev->channels[i].scan_index);

There are channels with a scan index > 63. These need to be skipped here
to avoid undefined behavior. UBSAN is reporting an error here.
> [...]
> + if (ams->pl_base) {
> + /* put sysmon in a soft reset to change the sequence */
> + ams_pl_update_reg(ams, AMS_REG_CONFIG1, AMS_CONF1_SEQ_MASK,
> + AMS_CONF1_SEQ_DEFAULT);
> +
> + /* configure basic channels */
> + scan_mask = FIELD_GET(AMS_PL_SEQ_MASK, scan_mask);
> + writel(FIELD_GET(AMS_REG_SEQ0_MASK, scan_mask),
> + ams->pl_base + AMS_REG_SEQ_CH0);
> + writel(FIELD_GET(AMS_REG_SEQ2_MASK, scan_mask),
> + ams->pl_base + AMS_REG_SEQ_CH2);
> + writel(FIELD_GET(AMS_REG_SEQ1_MASK, scan_mask),
> + ams->pl_base + AMS_REG_SEQ_CH1);
Hm, the ordering is a bit strange here: 0, 2, 1.
> +
> + /* set continuous sequence mode */
> + ams_pl_update_reg(ams, AMS_REG_CONFIG1, AMS_CONF1_SEQ_MASK,
> + AMS_CONF1_SEQ_CONTINUOUS);
> + }
> +}
> +
>
> +
> [...]
> +
> +static int ams_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct ams *ams = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&ams->lock);
> + if (chan->scan_index >= (AMS_PS_SEQ_MAX * 3)) {
This AMS_PS_SEQ_MAX * 3 really deserves a define. AMS_CTRL_SEQ_BASE or
something like that.
> + ret = ams_read_vcc_reg(ams, chan->address, val);
> + if (ret) {
> + mutex_unlock(&ams->lock);
> + return -EINVAL;
> + }
> + ams_enable_channel_sequence(indio_dev);
> + } else if (chan->scan_index >= AMS_PS_SEQ_MAX)
> + *val = readl(ams->pl_base + chan->address);
> + else
> + *val = readl(ams->ps_base + chan->address);
> + mutex_unlock(&ams->lock);
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + switch (chan->address) {
> + case AMS_SUPPLY1:
> + case AMS_SUPPLY2:
> + case AMS_SUPPLY3:
> + case AMS_SUPPLY4:
> + *val = AMS_SUPPLY_SCALE_3VOLT;
> + break;
> + case AMS_SUPPLY5:
> + case AMS_SUPPLY6:
> + if (chan->scan_index < AMS_PS_SEQ_MAX)
> + *val = AMS_SUPPLY_SCALE_6VOLT;
> + else
> + *val = AMS_SUPPLY_SCALE_3VOLT;
> + break;
> + case AMS_SUPPLY7:
> + case AMS_SUPPLY8:
> + *val = AMS_SUPPLY_SCALE_6VOLT;
> + break;
> + case AMS_SUPPLY9:
> + case AMS_SUPPLY10:
> + if (chan->scan_index < AMS_PS_SEQ_MAX)
> + *val = AMS_SUPPLY_SCALE_3VOLT;
> + else
> + *val = AMS_SUPPLY_SCALE_6VOLT;
> + break;
For the PL the range of supply 7 to 10 depends on the CONFIG_REG4 and
can be either 3V or 6V. Might be worth checking that register, rather
than hardcoding the value.
> + case AMS_VCC_PSPLL0:
> + case AMS_VCC_PSPLL3:
> + case AMS_VCCINT:
> + case AMS_VCCBRAM:
> + case AMS_VCCAUX:
> + case AMS_PSDDRPLL:
> + case AMS_PSINTFPDDR:

This is missing AMS_VCCAMS, AMS_VREFP, AMS_VREFN. They all use the 3V scale.

The only ones on a 1V scale are the external channels.

Maye it is easier to write this as

    case AMS_VP_VN:
    case AMS_REG_VAUX ... AMS_REG_VAUX(15):
        *val = AMS_SUPPLY_SCALE_1VOLT;
        break;
    default:
        *val = AMS_SUPPLY_SCALE_3VOLT;
        break;

> + *val = AMS_SUPPLY_SCALE_3VOLT;
> + break;
> + default:
> + *val = AMS_SUPPLY_SCALE_1VOLT;
> + break;
> + }
> + *val2 = AMS_SUPPLY_SCALE_DIV_BIT;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + case IIO_TEMP:
> + *val = AMS_TEMP_SCALE;
> + *val2 = AMS_TEMP_SCALE_DIV_BIT;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_OFFSET:
> + /* Only the temperature channel has an offset */
> + *val = AMS_TEMP_OFFSET;
> + return IIO_VAL_INT;
> + }
> +
> + return -EINVAL;
> +}
> +
> [...]
>
> +static int ams_init_module(struct iio_dev *indio_dev, struct device_node *np,
> + struct iio_chan_spec *channels)
> +{
> + struct ams *ams = iio_priv(indio_dev);
> + int num_channels = 0;
> +
> + if (of_device_is_compatible(np, "xlnx,zynqmp-ams-ps")) {
> + ams->ps_base = of_iomap(np, 0);
No unmap anywhere in the driver. Same for the other base addresses.
> [...]
> +
> + return num_channels;
> +}
> [...]
> +static int ams_probe(struct platform_device *pdev)
> +{
> [...]
> + ams->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(ams->clk))
> + return PTR_ERR(ams->clk);
> + clk_prepare_enable(ams->clk);
> + devm_add_action_or_reset(&pdev->dev, ams_clk_disable_unprepare,
> + ams->clk);
Needs to check for errors. Same for other users of
devm_add_action_or_reset().
> +
> + INIT_DELAYED_WORK(&ams->ams_unmask_work, ams_unmask_worker);
> + devm_add_action_or_reset(&pdev->dev, ams_cancel_delayed_work,
> + &ams->ams_unmask_work);
> +
> + ret = ams_init_device(ams);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to initialize AMS\n");
> + return ret;
> + }
> +
> + ret = ams_parse_dt(indio_dev, pdev);
> + if (ret) {
> + dev_err(&pdev->dev, "failure in parsing DT\n");
> + return ret;
> + }
> +
> + ams_enable_channel_sequence(indio_dev);
> +
> + ams->irq = platform_get_irq(pdev, 0);
> + if (ams->irq == -EPROBE_DEFER) {
What about other errors? E.g. if the interrupt is not specified int he
devicetree we'll pass the invalid irq number to request_irq().
> + ret = -EPROBE_DEFER;
> + return ret;
> + }
> +
> + ret = devm_request_irq(&pdev->dev, ams->irq, &ams_irq, 0, "ams-irq",
> + indio_dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to register interrupt\n");
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, indio_dev);
> +
> + ret = devm_iio_device_register(&pdev->dev, indio_dev);
> +
> + return ret;
> +}

2021-10-24 14:31:31

Hi Andy,

Thanks for the review.

> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Monday 25 October 2021 11:11 AM
> To: Anand Ashok Dumbre <[email protected]>
> Cc: Linux Kernel Mailing List <[email protected]>; Jonathan
> Cameron <[email protected]>; Lars-Peter Clausen <[email protected]>; linux-
> iio <[email protected]>; git <[email protected]>; Michal Simek
> <[email protected]>; Peter Meerwald <[email protected]>;
> devicetree <[email protected]>; Manish Narani
> <[email protected]>
> Subject: Re: [PATCH v7 2/4] iio: adc: Add Xilinx AMS driver
>
> On Tue, Oct 19, 2021 at 6:22 PM Anand Ashok Dumbre
> <[email protected]> wrote:
> >
> > The AMS includes an ADC as well as on-chip sensors that can be used to
> > sample external voltages and monitor on-die operating conditions, such
> > as temperature and supply voltage levels. The AMS has two SYSMON
> blocks.
> > PL-SYSMON block is capable of monitoring off chip voltage and
> > temperature.
> > PL-SYSMON block has DRP, JTAG and I2C interface to enable monitoring
> > from external master. Out of these interface currently only DRP is
>
> from an external
> interfaces

Will fix the grammar.

>
> > supported.
> > Other block PS-SYSMON is memory mapped to PS.
> > The AMS can use internal channels to monitor voltage and temperature
> > as well as one primary and up to 16 auxiliary channels for measuring
> > external voltages.
> > The voltage and temperature monitoring channels also have event
> > capability which allows to generate an interrupt when their value
> > falls below or raises above a set threshold.
> >
> > Signed-off-by: Manish Narani <[email protected]>
>
> What does this SoB mean here? Have you read Submitting Patches?

Will add the co-developed by tag here.

>
> > Signed-off-by: Anand Ashok Dumbre <[email protected]>
>
> ...
>
> > +config XILINX_AMS
> > + tristate "Xilinx AMS driver"
> > + depends on ARCH_ZYNQMP || COMPILE_TEST
> > + depends on HAS_IOMEM
> > + help
>
> > + Say yes here to have support for the Xilinx AMS.
>
> It's not important for most of the users. Please, strat help with more useful
> information like below.

Will do.

>
> > + The driver supports Voltage and Temperature monitoring on Xilinx
> Ultrascale
> > + devices.
> > +
> > + The driver can also be built as a module. If so, the module will be
> called
> > + xilinx-ams.
>
> ...
>
> > + * Manish Narani <[email protected]>
>
> A-ha! You probably forgot the Co-developed-by tag above.

Correct will add.

>
> > + * Rajnikant Bhojani <[email protected]>
>
> ...
>
> Missed headers, like bits.h.

Will add.

>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
>
> > +#include <linux/of_address.h>
>
> Do you need this? Maybe mod_devicetable.h?
>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
>
> ...
>
> > +static const unsigned int AMS_UNMASK_TIMEOUT_MS = 500;
>
> Why not define (esp. taking into account another similar define below?
>

Makes sense

> ...
>
> > +#define AMS_REGCFG1_ALARM_MASK 0xF0F
> > +#define AMS_REGCFG3_ALARM_MASK 0x3F
>
> > +#define AMS_PL_ALARM_MASK 0xFFFF0000U
> > +#define AMS_ISR0_ALARM_MASK 0xFFFFFFFFU
> > +#define AMS_ISR1_ALARM_MASK 0xE000001FU
> > +#define AMS_ISR1_EOC_MASK 0x00000008U
>
> What is so special about these that they are not using combinations of
> GENMASK() / BIT()?
>
Will add for those.

> ...
>
> > +enum ams_alarm_bit {
> > + AMS_ALARM_BIT_TEMP,
> > + AMS_ALARM_BIT_SUPPLY1,
> > + AMS_ALARM_BIT_SUPPLY2,
> > + AMS_ALARM_BIT_SUPPLY3,
> > + AMS_ALARM_BIT_SUPPLY4,
> > + AMS_ALARM_BIT_SUPPLY5,
> > + AMS_ALARM_BIT_SUPPLY6,
> > + AMS_ALARM_BIT_RESERVED,
> > + AMS_ALARM_BIT_SUPPLY7,
> > + AMS_ALARM_BIT_SUPPLY8,
> > + AMS_ALARM_BIT_SUPPLY9,
> > + AMS_ALARM_BIT_SUPPLY10,
> > + AMS_ALARM_BIT_VCCAMS,
> > + AMS_ALARM_BIT_TEMP_REMOTE
>
> Is it terminator line? Doesn't sound like it to me. So, please add a comma.
> Same for the rest.
>
> > +};
>
> ...
>
> > + AMS_SEQ_MAX
>
> This seems correct, no comma is needed :-)
>
> ...
>
> > +struct ams {
> > + void __iomem *base;
> > + void __iomem *ps_base;
> > + void __iomem *pl_base;
> > + struct clk *clk;
> > + struct device *dev;
>
> > + /* check kernel doc above */
>
> Useless
>
> > + struct mutex lock;
>
> > + /* check kernel doc above */
>
> Ditto.
>

Will remove the comments

> > + spinlock_t intr_lock;
> > + unsigned int alarm_mask;
> > + unsigned int masked_alarm;
> > + u64 intr_mask;
> > + int irq;
> > + struct delayed_work ams_unmask_work; };
>
> ...
>
> > + writel((val & ~mask) | (data & mask), ams->ps_base + offset);
>
> Split to assignment and simple writel() call. Same to the rest.
>
Will do.

> ...
>
> > + ams->intr_mask &= ~mask;
> > + ams->intr_mask |= (val & mask);
>
> This may be combined to one line as it's a standard pattern.
>
Will do.

> ...
>
> > + if (ams->ps_base) {
> > + /* Configuring PS alarm enable */
> > + cfg = ~((alarm_mask & AMS_ISR0_ALARM_2_TO_0_MASK) <<
> > + AMS_CONF1_ALARM_2_TO_0_SHIFT);
> > + cfg &= ~((alarm_mask & AMS_ISR0_ALARM_6_TO_3_MASK) <<
> > + AMS_CONF1_ALARM_6_TO_3_SHIFT);
> > + ams_ps_update_reg(ams, AMS_REG_CONFIG1,
> AMS_REGCFG1_ALARM_MASK,
> > + cfg);
> > +
> > + cfg = ~((alarm_mask >> AMS_CONF3_ALARM_12_TO_7_SHIFT) &
> > + AMS_ISR0_ALARM_12_TO_7_MASK);
> > + ams_ps_update_reg(ams, AMS_REG_CONFIG3,
> AMS_REGCFG3_ALARM_MASK,
> > + cfg);
> > + }
>
> By factoring out the body of the conditional to a helper function you may:
> - decrease indentation
> - make code better to read
> - reduce LOCs

Will do.

>
> > + if (ams->pl_base) {
> > + pl_alarm_mask = (alarm_mask >> AMS_PL_ALARM_START);
> > + pl_alarm_mask = FIELD_GET(AMS_PL_ALARM_MASK,
> alarm_mask);
> > + /* Configuring PL alarm enable */
> > + cfg = ~((pl_alarm_mask & AMS_ISR0_ALARM_2_TO_0_MASK) <<
> > + AMS_CONF1_ALARM_2_TO_0_SHIFT);
> > + cfg &= ~((pl_alarm_mask & AMS_ISR0_ALARM_6_TO_3_MASK)
> <<
> > + AMS_CONF1_ALARM_6_TO_3_SHIFT);
> > + ams_pl_update_reg(ams, AMS_REG_CONFIG1,
> > + AMS_REGCFG1_ALARM_MASK, cfg);
> > +
> > + cfg = ~((pl_alarm_mask >> AMS_CONF3_ALARM_12_TO_7_SHIFT)
> &
> > + AMS_ISR0_ALARM_12_TO_7_MASK);
> > + ams_pl_update_reg(ams, AMS_REG_CONFIG3,
> > + AMS_REGCFG3_ALARM_MASK, cfg);
> > + }
>
> Ditto. And the same applies to all the rest where it gains something from the
> above list of improvements.
>
Will do.

> ...
>
> > + int i;
> > + unsigned long long scan_mask;
> > + struct ams *ams = iio_priv(indio_dev);
>
> Reversed xmas tree order, please.
> Same for the rest.
>
Will do.

> ...
>
> > + /* Run calibration of PS & PL as part of the sequence */
> > + scan_mask = 0x1 | BIT(AMS_PS_SEQ_MAX);
>
> BIT(0) ?
>

Will fix.
> ...
>
> > + ams_update_intrmask(ams, ~0, ~0);
>
> Replace ~0 to proper GENMASK()./BIT() combination which takes into
> account real bits used by hardware.
>
Will fix.

> ...
>
> > + case IIO_CHAN_INFO_RAW:
> > + mutex_lock(&ams->lock);
> > + if (chan->scan_index >= (AMS_PS_SEQ_MAX * 3)) {
>
> Too many parens.
>
> > + ret = ams_read_vcc_reg(ams, chan->address, val);
> > + if (ret) {
> > + mutex_unlock(&ams->lock);
> > + return -EINVAL;
> > + }
> > + ams_enable_channel_sequence(indio_dev);
> > + } else if (chan->scan_index >= AMS_PS_SEQ_MAX)
> > + *val = readl(ams->pl_base + chan->address);
> > + else
> > + *val = readl(ams->ps_base + chan->address);
> > + mutex_unlock(&ams->lock);
> > +
> > + return IIO_VAL_INT;
>
> ...
>
> > + return -EINVAL;
>
> Use corresponding defaul cases in each of the switches.
>

Will do.

> ...
>
> > + int offset = 0;
>
> Make the assignment as an else branch, so all offset assignments will be
> grouped together.
>
> > + if (dir == IIO_EV_DIR_FALLING) {
> > + if (scan_index < AMS_SEQ_SUPPLY7)
> > + offset = AMS_ALARM_THRESHOLD_OFF_10;
> > + else
> > + offset = AMS_ALARM_THRESHOLD_OFF_20;
> > + }
>
> ...
>
> > + return 0;
>
> default case.

Will do.

>
> > +}
>
> ...
>
> > +static const struct iio_chan_spec
> > +*ams_event_to_channel(struct iio_dev *indio_dev, u32 event)
>
> Unusual indentation.

Will fix.

>
> ...
>
> > + case AMS_ALARM_BIT_TEMP_REMOTE:
> > + scan_index += AMS_SEQ_TEMP_REMOTE;
> > + break;
>
> default?
> Same for the rest of the cases like this.

Will add.

>
> ...
>
> > + return (ams->alarm_mask &
> > + ams_get_alarm_mask(chan->scan_index)) ? 1 : 0;
>
> !! would work as well.
>
> ...
>
> > + /*
> > + * The temperature channel only supports over-temperature
> > + * events
>
> Missed period.
>
> > + */
>
> ...
>
> > + /* only process alarms that are not masked */
>
> Inconsistent style (here capitalization is missed). Make all comments in the
> code consistent.
>
> > + isr0 &= ~((ams->intr_mask & AMS_ISR0_ALARM_MASK) |
> > + ams->masked_alarm);
>
> > +
>
> Redundant blank line.
>
> > + if (!isr0)
>
> How did you test this branch? (Hint: something very important should be
> done here)
Missing spin_unlock.

>
> > + return IRQ_NONE;
>
> ...
>
> > + for_each_child_of_node(chan_node, child) {
> > + ret = of_property_read_u32(child, "reg", &reg);
> > + if (ret || reg > (AMS_PL_MAX_EXT_CHANNEL + 30))
> > + continue;
> > +
> > + memcpy(&channels[num_channels], &ams_pl_channels[reg +
> > + AMS_PL_MAX_FIXED_CHANNEL - 30],
> > + sizeof(*channels));
> > +
> > + if (of_property_read_bool(child, "xlnx,bipolar"))
> > + channels[num_channels].scan_type.sign = 's';
> > +
> > + num_channels++;
> > + }
>
> Use device property API here instead of *of_*() calls.

>
> ...
>
> > + /* Initialize buffer for channel specification */
> > + ams_channels = kzalloc(sizeof(ams_ps_channels) +
> > + sizeof(ams_pl_channels) +
> > + sizeof(ams_ctrl_channels), GFP_KERNEL);
>
> Use the corresponding macro from overflow.h.
>
> > + if (!ams_channels)
> > + return -ENOMEM;
>
> ...
>
> > + if (of_device_is_available(np)) {
>
> fwnode_device_is_available()

Currently acpi is not supported with this driver. But I will add support in the next series of patches.
I don’t have a full understanding of ACPI and its interfaces. So would it be okay once the first iteration
gets checked in, I will add ACPI support on top.

>
> > + ret = ams_init_module(indio_dev, np, ams_channels);
> > + if (ret < 0)
> > + goto err;
> > +
> > + num_channels += ret;
> > + }
>
> ...
>
> > + for_each_child_of_node(np, child_node) {
> > + if (of_device_is_available(child_node)) {
> > + ret = ams_init_module(indio_dev, child_node,
> > + ams_channels + num_channels);
> > + if (ret < 0)
> > + goto err;
> > +
> > + num_channels += ret;
> > + }
> > + }
>
> As per above.
>
> ...
>
> > + if (!pdev->dev.of_node)
> > + return -ENODEV;
>
> Drop this, please. It will allow reuse of the driver in ACPI environments.
>
> ...
>
> > + ams->irq = platform_get_irq(pdev, 0);
> > + if (ams->irq == -EPROBE_DEFER) {
>
> Is IRQ optional or not?

Its not. I will add a generic handling.

>
> > + ret = -EPROBE_DEFER;
> > + return ret;
> > + }
>
> ...
>
> > + ret = devm_iio_device_register(&pdev->dev, indio_dev);
> > +
> > + return ret;
>
> return devm_...

Will do.

>
> ...
>
> > + clk_prepare_enable(ams->clk);
>
> It might fail.

Will add return

>
>
> --
> With Best Regards,
> Andy Shevchenko

2021-11-03 17:16:32

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v7 2/4] iio: adc: Add Xilinx AMS driver

On Tue, 2 Nov 2021 21:31:21 +0000
Anand Ashok Dumbre <[email protected]> wrote:

> Hi Andy,
>
> Thanks for the review.
...

> > > + return IRQ_NONE;
> >
> > ...
> >
> > > + for_each_child_of_node(chan_node, child) {
> > > + ret = of_property_read_u32(child, "reg", &reg);
> > > + if (ret || reg > (AMS_PL_MAX_EXT_CHANNEL + 30))
> > > + continue;
> > > +
> > > + memcpy(&channels[num_channels], &ams_pl_channels[reg +
> > > + AMS_PL_MAX_FIXED_CHANNEL - 30],
> > > + sizeof(*channels));
> > > +
> > > + if (of_property_read_bool(child, "xlnx,bipolar"))
> > > + channels[num_channels].scan_type.sign = 's';
> > > +
> > > + num_channels++;
> > > + }
> >
> > Use device property API here instead of *of_*() calls.
>
>
> >
> > ...
> >
> > > + /* Initialize buffer for channel specification */
> > > + ams_channels = kzalloc(sizeof(ams_ps_channels) +
> > > + sizeof(ams_pl_channels) +
> > > + sizeof(ams_ctrl_channels), GFP_KERNEL);
> >
> > Use the corresponding macro from overflow.h.
> >
> > > + if (!ams_channels)
> > > + return -ENOMEM;
> >
> > ...
> >
> > > + if (of_device_is_available(np)) {
> >
> > fwnode_device_is_available()
>
> Currently acpi is not supported with this driver. But I will add support in the next series of patches.
> I don’t have a full understanding of ACPI and its interfaces. So would it be okay once the first iteration
> gets checked in, I will add ACPI support on top.

If you use the generic fwnode etc from property.h then dt
will just work and it may well be enough to enable the weirdness that
is ACPI ID PRP0001 which actually uses dt properties but reads them
from an ACPI DSDT.

So basically nothing to do except use the generic accessors rather than
the of_ variants.

Jonathan