On Sat, Nov 13, 2021 at 6:12 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On Mon, 8 Nov 2021 21:05:07 +0000
> Anand Ashok Dumbre <anand.ashok.dumbre@xxxxxxxxxx> 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...
...
> > +static int ams_init_module(struct iio_dev *indio_dev, struct fwnode_handle *np,
np --> fwnode
> > + struct iio_chan_spec *channels)
> > +{
> > + struct ams *ams = iio_priv(indio_dev);
> > + struct device *dev = indio_dev->dev.parent;
> > + 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.
> > + if (!ams->pl_base)
> > + return -ENXIO;
...
> > + /* 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 (!ams_channels)
> > + return -ENOMEM;
--
With Best Regards,
Andy Shevchenko
