Re: [PATCH v6 12/16] iio: frequency: ad9910: add RAM mode support

On Mon, Jul 06, 2026 at 09:10:21AM -0500, David Lechner wrote:
> On 7/6/26 4:47 AM, Nuno Sá wrote:
> > On Sat, Jul 04, 2026 at 11:50:35AM -0500, David Lechner wrote:
> >> On 7/3/26 9:09 AM, Nuno Sá wrote:
> >>> On Fri, Jul 03, 2026 at 04:05:44AM +0100, Jonathan Cameron wrote:
> >>>> On Thu, 18 Jun 2026 14:27:28 +0100
> >>>> Rodrigo Alencar via B4 Relay <[email protected]> wrote:
> >>>>
> >>>>> From: Rodrigo Alencar <[email protected]>
> >>>>>
> 
> ..
> 
> >>>>> +static inline void ad9910_debugfs_init(struct ad9910_state *st,
> >>>>> +				       struct iio_dev *indio_dev)
> >>>>> +{
> >>>>> +	struct dentry *d = iio_get_debugfs_dentry(indio_dev);
> >>>>> +	char buf[64];
> >>>>> +
> >>>>> +	/*
> >>>>> +	 * symlinks are created here so iio userspace tools can refer to them
> >>>>> +	 * as debug attributes.
> >>>>
> >>>> Maybe worth a reference to appropriate ABI doc here (even if it is introduced
> >>>> in a later patch)
> >>>
> >>> I'm not so sure about these links. I mean, I definitely agree we should
> >>> make it easy for userspace tools like libiio to be able to handle
> >>> these kind of attributes but using debugfs is questionable to me. Pretty
> >>> much because this is not a debug thing. It is a real setting for the
> >>> driver so ideally we would be able to control it (using the existent
> >>> tools) without enforcing one to mount debugfs (I know that most of the
> >>> times it's always mounted but still feels wrong to tie "real
> >>> functionality" to debugfs). 
> >>>
> >>> Having said the above, some suggestions:
> >>>
> >>> 1. Make the iio_dev the parent so that the attr name is just "ram" and
> >>> it will be a subdir /sys/bus/iio/iio:deviceN/ram/.
> >>> 2. Propose a new helper for the firmware_loader code so we can get
> >>> struct device from struct fw_upload then we can easily create a sysfs
> >>> symlink.
> >>> 3. Name the attr as dev_name(iio_dev):attr so that it becomes
> >>> iio:deviceN:attr_name.
> >>>
> >>> Now that I think about it, 2. does not make much sense when compared to
> >>> 1. And If I'm not missing anything both 1. and 3. can be sanely parsable
> >>> from userspace (being 3. maybe a bit more reliable). And yes, both require
> >>> user space tools (in this case libiio) to support a new type of
> >>> attribute (firmware) but that is another problem.
> >>
> >> I would say that udev should be used to find the firmware device from
> >> userspace rather than creating symlinks. And you probably don't even
> >> need udev if you already have the `iio-device:<ID>` value. The path
> > 
> > Just one note. If you mean <ID> as the attr name the above will fail for
> > multiple instances of the same device. That's is why a proposed
> > dev_name(iio_dev) which I think it should reliable be iio:deviceN. 
> 
> My <ID> is the same as your N. It looks like I just had a typo of
> an extra : in there.
> 
> > 
> > I guess N might change so anyone hardcoding it might have surprises. But
> 
> That is why I mentioned udev.

I see! I would still advocate for apps to properly do the matching
between the current device id and the firmware path :)

But the above is obvious something we can't control.

- Nuno Sá

> 
> > properly done and we have a sane way to match the above with anything on
> > /sys/class/firmware/iio:deviceN:<attr>/
> > 
> >> is always going to be `/sys/class/firmware/iio-device:<ID>:ram/`. It
> >> seems like the same amount of work for usespace to computer the path
> >> either way, so might as well keep it simple and leave out the symlink
> >> stuff.
> > 
> > Yes, at least for libiio, `iio:deviceN` is already what we call the
> > device ID so we can match fw attrs against it and everything after the
> > last ':' is treated as the attr_name. 
> > 
> > We just need to keep in mind that if a second user of this stuff pops
> > up, we should add a new helper to enforce the ABI.
> > 
> > - Nuno Sá
> >>
> >>>
> >>> - Nuno Sá
> >>>>
> >>>>> +	 */
> >>>>> +	snprintf(buf, sizeof(buf), "/sys/class/firmware/%s/loading", st->ram_fwu_name);
> >>>>> +	debugfs_create_symlink("ram_loading", d, buf);
> >>>>> +
> >>>>> +	snprintf(buf, sizeof(buf), "/sys/class/firmware/%s/data", st->ram_fwu_name);
> >>>>> +	debugfs_create_symlink("ram_data", d, buf);
> >>>>> +}
> >>>>> +