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

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.

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