On Sat, Nov 13, 2021 at 6:12 PM Jonathan Cameron [off-list ref] wrote:

On Mon, 8 Nov 2021 21:05:07 +0000
Anand Ashok Dumbre [off-list ref] wrote:

...

+Cc Andy for a question on of_iomap() and whether we should add something similar
to the generic firmware handling.  Personally I'd be happy with leaving it for
now on basis of it being something to come back and tidy up later, but would
like your opinion as you are more familiar with that stuff.

Thanks, let me look into it...

...

quoted

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

np --> fwnode

quoted

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

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);
+             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.

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.

quoted

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

...

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.

quoted

+     if (!ams_channels)
+             return -ENOMEM;

--
With Best Regards,
Andy Shevchenko