Hi Andy,

Thank you for reviewing the patches.
 

quoted

quoted

+static int ams_init_module(struct iio_dev *indio_dev, struct
+fwnode_handle *np,

np --> fwnode

Will do. 

quoted

quoted

+                        struct iio_chan_spec *channels) {
+     struct ams *ams = iio_priv(indio_dev);
+     struct device *dev = indio_dev->dev.parent;

quoted

quoted

+     struct device_node *node;
+     int num_channels = 0;
+     int ret;
+
+     node = to_of_node(np);
+     if (fwnode_property_match_string(np, "compatible",
+                                      "xlnx,zynqmp-ams-ps") == 0) {
+             ams->ps_base = of_iomap(node, 0);
+             if (!ams->ps_base)
+                     return -ENXIO;
+             ret = devm_add_action_or_reset(dev, ams_iounmap, ams-

ps_base);

quoted

+             if (ret < 0)
+                     return ret;
+
+             /* add PS channels to iio device channels */
+             memcpy(channels + num_channels, ams_ps_channels,
+                    sizeof(ams_ps_channels));
+             num_channels += ARRAY_SIZE(ams_ps_channels);
+     } else if (fwnode_property_match_string(np, "compatible",
+                                             "xlnx,zynqmp-ams-pl") == 0) {
+             ams->pl_base = of_iomap(node, 0);

Hmm. So of_iomap() leaves us dependent on dt specific calls. Whilst it
doesn't exactly look hard to create a generic version covering at
least dt and acpi I don' think there is an equivalent acpi function currently

defined.

quoted

Andy, do you think this is a good thing to add to the generic firmware
node handling?  It's a bit specialist as most of the time this will be
wrapped up in the platform device handling or similar rather than being in a

child node like this.

I saw this issue previously somewhere else and we can do something about
it.
But first (before going to ACPI guts) we may indeed introduce a basic
skeleton under fwnode API for this.

Something like

void __iomem *fwnode_iomap(...)
{
  if (is_of_node(fwnode))
    return of_iomap();
  return NULL;
}
EXPORT_SYMBOL_GPL(fwnode_iomap);

At least it will allow drivers to make them property provider agnostic.

Okay, I checked the current version of the ACPI specification and the
proposed DTS here. With above API and something like

Device (AMS0) {
  Name (_CRS, ...)
  ...
  Device (PLMN) {
    Name (_CRS, ...)
  }
  Device (PSMN) {
    Name (_CRS, ...)
  }
}

we may get the resource from the corresponding fwnode's _CRS object

Just to be sure, do I need to do anything for this in this iteration?

quoted

quoted

+             if (!ams->pl_base)
+                     return -ENXIO;

...

quoted

quoted

+     /* Initialize buffer for channel specification */
+     ams_channels = kzalloc(sizeof(ams_ps_channels) +
+                            sizeof(ams_pl_channels) +
+                            sizeof(ams_ctrl_channels), GFP_KERNEL);

Make a use of something from overflow.h.

If my understanding is right, I need to use check_mul_overflow.

quoted

quoted

+     if (!ams_channels)
+             return -ENOMEM;

--
With Best Regards,
Andy Shevchenko

Thanks,
Anand