Subject: Re: [PATCH 3/8] base: soc: export soc_device_get_machine()

From: "Danilo Krummrich" <[email-masked]>
Date: Mon, 19 Jan 2026 19:41:26 +0100

On Mon Jan 19, 2026 at 12:36 PM CET, Danilo Krummrich wrote:
> On Mon Jan 19, 2026 at 11:40 AM CET, Bartosz Golaszewski wrote:
>> -static void soc_device_get_machine(struct soc_device_attribute *soc_dev_attr)
>> +int soc_device_get_machine(struct soc_device_attribute *soc_dev_attr)
>>  {
>> -	struct device_node *np;
>> -
>>  	if (soc_dev_attr->machine)
>> -		return;
>> +		return -EBUSY;
>> +
>> +	struct device_node *np __free(device_node) = of_find_node_by_path("/");
>> +	if (!np)
>> +		return -ENOENT;
>
> This should never fail at this point, no? Also, can't we just use of_root?

Regarding of_root, please disregard my earlier comment. I mistakenly assumed
that it would also be guarded by CONFIG_OF.

But I still think we do not need the NULL check.

>> -	np = of_find_node_by_path("/");
>> -	of_property_read_string(np, "model", &soc_dev_attr->machine);
>> -	of_node_put(np);
>> +	return of_property_read_string(np, "model", &soc_dev_attr->machine);
>>  }
>> +EXPORT_SYMBOL_GPL(soc_device_get_machine);
>
> If we want to export this, we shouldn't reuse the existing name, which is
> misleading.
>
> soc_device_get_machine() reads as if we return a reference count of something.
> Additionally, it operates on struct soc_device_attribute instead of struct
> soc_device, where the name suggests the latter.
>
> Instead this should be soc_device_attribute_read_machine() or if we want a
> shorter name, just soc_attr_read_machine().