Hi Andy,
Thank you for reviewing the patches.
> > > +static int ams_init_module(struct iio_dev *indio_dev, struct
> > > +fwnode_handle *np,
>
> np --> fwnode
Will do.
>
> > > + 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
Just to be sure, do I need to do anything for this in this iteration?
>
> > > + 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 my understanding is right, I need to use check_mul_overflow.
>
>
> > > + if (!ams_channels)
> > > + return -ENOMEM;
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks,
Anand
