Re: [PATCH v19 06/10] power: reset: Add psci-reboot-mode driver

[Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>]

On 1/2/2026 5:27 PM, Bartosz Golaszewski wrote:
> On Sun, Dec 28, 2025 at 6:21 PM Shivendra Pratap
> <shivendra.pratap@oss.qualcomm.com> wrote:
>>
> 
> [snip]
> 
>> +
>> +static int psci_reboot_mode_probe(struct faux_device *fdev)
>> +{
>> +       struct reboot_mode_driver *reboot;
>> +       struct device_node *psci_np;
>> +       struct device_node *np;
>> +       int ret;
>> +
>> +       psci_np = of_find_compatible_node(NULL, NULL, "arm,psci-1.0");
>> +       if (!psci_np)
>> +               return -ENODEV;
>> +
>> +       /*
>> +        * Find the psci:reboot-mode node.
>> +        * If NULL, continue to register predefined modes.
>> +        * np refcount to be handled by dev;
>> +        * psci_np refcount is decremented by of_find_node_by_name;
>> +        */
> 
> Can you make this comment into full sentences, I had trouble parsing
> the meaning for a minute.

Ack.

> 
>> +       np = of_find_node_by_name(psci_np, "reboot-mode");
>> +       fdev->dev.of_node = np;
> 
> The logic of the device assigning its own node is a bit sketchy,
> ideally this should be done before the device probes.

Will move it to init. thanks.

> 
>> +
>> +       reboot = devm_kzalloc(&fdev->dev, sizeof(*reboot), GFP_KERNEL);
>> +       if (!reboot)
>> +               return -ENOMEM;
>> +
>> +       psci_reboot_mode_set_predefined_modes(reboot);
>> +       reboot->write = psci_reboot_mode_write;
>> +       reboot->dev = &fdev->dev;
>> +
>> +       ret = devm_reboot_mode_register(&fdev->dev, reboot);
>> +       if (ret) {
>> +               dev_err(&fdev->dev, "devm_reboot_mode_register failed %d\n", ret);
>> +               return ret;
> 
> Use dev_err_probe().

Ack.

> 
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static struct faux_device_ops psci_reboot_mode_ops = {
>> +       .probe = psci_reboot_mode_probe,
>> +};
>> +
>> +static int __init psci_reboot_mode_init(void)
>> +{
>> +       struct faux_device *fdev;
>> +
>> +       fdev = faux_device_create("psci-reboot-mode", NULL, &psci_reboot_mode_ops);
>> +       if (!fdev)
>> +               return -ENODEV;
> 
> This will always create this device for everyone who includes this
> module. Move the of_find_compatible_node(NULL, NULL, "arm,psci-1.0")
> call from probe() here instead and don't create the device if it
> fails.

Ack. 
Will move both calls to init before creating the faux device. 

psci_np = of_find_compatible_node(NULL, NULL, "arm,psci-1.0");
and 
np = of_find_node_by_name(psci_np, "reboot-mode");
--

thanks,
Shivendra