Hi Sudan & especially Krzysztof,
On Wed, Apr 17, 2024 at 12:43 PM Babis Chalios <[email protected]> wrote:
> struct vmgenid_state {
> u8 *next_id;
> u8 this_id[VMGENID_SIZE];
> + int irq;
This is only ever used inside of one function. Why not just keep it on
the stack?
> };
>
> static void vmgenid_notify(struct device *device)
> @@ -43,6 +45,14 @@ vmgenid_acpi_handler(acpi_handle __always_unused handle,
> vmgenid_notify(dev);
> }
>
> +static irqreturn_t
> +vmgenid_of_irq_handler(int __always_unused irq, void *dev)
> +{
> + vmgenid_notify(dev);
> +
> + return IRQ_HANDLED;
> +}
Is there a reason the of code isn't conditional on CONFIG_OF? I'm not
super familiar with these drivers, but this seems like it would be a
thing to do, and then we could do `depends on OF || ACPI` in the
Kconfig.
After the whole Babis authorship debacle, I'm just fixing various
things up in my own tree and I'll send out a v+1. But Krzysztof, I
would really appreciate your review of this before I apply it to
random-next.
Jason
On 17/04/2024 17:16, Jason A. Donenfeld wrote:
>> +static irqreturn_t
>> +vmgenid_of_irq_handler(int __always_unused irq, void *dev)
>> +{
>> + vmgenid_notify(dev);
>> +
>> + return IRQ_HANDLED;
>> +}
>
> Is there a reason the of code isn't conditional on CONFIG_OF? I'm not
> super familiar with these drivers, but this seems like it would be a
> thing to do, and then we could do `depends on OF || ACPI` in the
> Kconfig.
>
Usually we do not recommend hiding code behind !CONFIG_OF because this
limits possible usage on ACPI systems via PRP0001. Not sure if it is
applicable here, because there is already ACPI matching.
I would suggest choose whatever makes code simpler... Or just mark some
pieces with __maybe_unused if they are really not used? That would avoid
ifdeffery.
Best regards,
Krzysztof