On Mon, Jan 20, 2025 at 11:59:03AM +0100, Fiona Behrens wrote:
> >
> > This looks as if you are doing a Rust binding for struct led_trigger.
> > But you keep calling it Hardware trigger, which makes me thing that
> > you are confused about what is a LED trigger and what is a hardware
> > trigger.
> >
> > Why do you keep putting "Hardware" into the names of these symbols?
>
> The idea was to create a abstraction specific to writing a hardware trigger (or my understanding of what that is) and deal with the other
> trigger types later, to more separate the things on the rust side with e.g. the vtables for those.
> But my understanding might be wrong.
>
> (My broad understanding is what I did in the SE10 driver later, to tell the hardware to not present the LED to the kernel, but some other hardware wiring to a hardware thing that then drives the LED)
Looking at patch 5, you do:
#[vtable]
#[cfg(CONFIG_LEDS_TRIGGERS)]
impl triggers::HardwareOperations for LedSE10 {
fn activate(this: &mut Self::This) -> Result {
this.data.send_cmd(LedCommand::Trigger)
}
}
I think this naming "triggers::HardwareOperations" may cause confusion
in the future. I think that what you implement here should be called
LED Private Trigger, or something derived from that.
Using the work "hardware" may lead people to think that it means the
other mechanism, wherein we offload software LED triggers to hardware
(which is implemented via the `hw_control_*` members of
`struct led_classdev`).
>
> >
> > I fear that you may be confused about some stuff here. In order to
> > determine whether this is the case, could you answer the following
> > questions please?
>
> That might be right, thanks for trying to clear it up if that is the case.
>
> > - What is the purpose of `struct led_hw_trigger_type`?
> Marking a led that it has a private trigger that gives control of the LED to some hardware driver.
> > - What is the purpose of the `dummy` member of this struct? What
> > value should be assigned to it?
> From my understanding this is just to give the struct a size, so that it has a unique address in memory so the pointer value can be compared.
> > - If a LED class device (LED cdev) has the `trigger_type` member set
> > to NULL, which LED triggers will be listed in the sysfs `trigger`
> > file for this LED cdev? And which triggers will be listed if the
> > `trigger_type` member is not NULL?
> For null all generic triggers will be listed, and for some value all generic plus the specific trigger.
> > - Why does both `struct led_classdev` and `struct led_trigger` have
> > the `trigger_type` member?
> led_classdev has it to declare that it does have a led private trigger mode, and the led_trigger has it so the activate/deactive functions can be found.
>
> My research so far into how triggers work was mostly so that I can use the wwan module trigger on the SE10 board I have here, and therefore I did not look into how to write a generic led trigger usable on more then a specific led.
>
> Thanks a lot for clearing up possible misunderstandings,
> Fiona
OK it seems that you do indeed understand these.
