Re: [PATCH v3 7/7] leds: add synology microp led driver

[Markus Probst <markus.probst@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 4893 bytes --]

On Mon, 2026-03-16 at 14:58 +0100, Greg Kroah-Hartman wrote:
> On Mon, Mar 16, 2026 at 01:43:44PM +0000, Markus Probst wrote:
> > On Mon, 2026-03-16 at 07:33 +0100, Greg Kroah-Hartman wrote:
> > > On Sun, Mar 15, 2026 at 08:41:06PM +0100, Danilo Krummrich wrote:
> > > > I.e. if we can't (easily) use mfd cells and would need a custom API, then why
> > > > even split it up at all, given that splitting it up would probably the most
> > > > complicated part of the whole driver.
> > > > 
> > > > Greg, what do you think?
> > > 
> > > I think this has yet to be proven to be a kernel driver at all at this
> > > point, and not just a userspace daemon that listens to the serial port
> > > and then does what is needed from there :)
> > > 
> > > Or, if someone can prove that the operations on this serial data stream
> > > actually do require it to be in the kernel (which I have yet to see a
> > > list of what this connection does, did I miss it?) then a single driver,
> > > under the drivers/platform section of the kernel tree makes sense.
> > The sysoff component is strictly necessary for poweroff and reboot.
> > 
> > On ARM64 Synology NAS devices it is needed so the device actually
> > powers off after calling
> > `syscall(SYS_reboot, LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2,
> > LINUX_REBOOT_CMD_POWER_OFF, NULL);`
> > . Otherwise it would stay on.
> > Same applies to reboot.
> 
> So that means a write of a set of bytes to the serial port will cause
> the machine to reboot or shutdown?
Yes.
> 
> > On x86 it isn't clearly documented what sending the poweroff and reboot
> > command to the microp device exactly does, so this is based on
> > observations. It should be sent before issuing a poweroff or reboot via
> > ACPI Sleep. On reboot it resets various device states, so fan speeds go
> > to default, leds turn off etc., so it behaves like a coldboot.
> > On poweroff it will mark it as graceful shutdown (i. e. the device
> > won't turn automatically on, because it thinks a power-loss happend).
> > 
> > For the other components:
> > - leds
> > - hwmon
> > - input
> 
> For "input" what exactly does the input device show up as?  A power
> button?  Something else?

Well there are a few other things besides a power button:

- beeper (short and long variant)
- factory reset button
- "USB Copy" button. Not present on all devices (including not present
on my testing device). It is meant for copying data from or to a usb
stick.

Both of these buttons don't have a keycode in include/uapi/linux/input-
event-codes.h, which is suprising given how many devices have a factory
reset button. So I am not sure if I can handle them in this driver.

From what I can tell, there isn't actually a input device type I could
specify on registering a input device. But it would be considered a
misc input device.

> 
> For hwmon, that makes sense to have a kernel driver.
> 
> For leds, that depends on what you want to do with the led, as in the
> end you are just controlling it from userspace anyway :)


> 
> > It could theoratically be implemented in userspace. A userspace daemon
> > could theoratically control the fan speeds directly, issue a systemd
> > shutdown on power button press, control the leds directly etc.
> > 
> > But honestly, I don't understand why this is an argument.
> > With that argument drivers/leds, drivers/hwmon and drivers/input would
> > not even exist, because everything could be implemented in userspace
> > via
> > - I2C: /dev/i2c-* (drivers/i2c/i2c-dev.c)
> > - MMIO: /dev/ioport and /dev/mem (drivers/char/mem.c)
> > - GPIO: /sys/class/gpio (drivers/gpio/gpiolib-sysfs.c)
> > - SPI: /dev/spidev* (drivers/spi/spidev.c)
> > - PCI: /sys/class/pci_bus/ (drivers/pci/pci-sysfs.c)
> > - Serial: /dev/ttyS*
> > and likely almost any other bus device too.
> > 
> > Generally speaking, the kernel and its drivers is the layer between
> > hardware and software. It provides the hardware abstractions as
> > userspace interfaces. So any software on the same cpu architecture can
> > work with any hardware, as long as there is a kernel driver.
> 
> Yes, I kind of know what drivers and classes do and why they are needed,
> that's not the point here. :)
> 
> > In the case of this driver, it means
> > - *any* led daemon can control the leds
> > - *any* fan control daemon can control the fan speed and frequency
> > - *any* monitoring software can view the provided sensors
> > - *any* init system can react to the power button
> > - *any* process can request a reboot or shutdown
> > .
> > I think this is the expected behaviour.
> 
> Ok great, then make a single driver that handles all of this, like other
> drivers/platform/ drivers do today, and all should be fine.
Great, I will.

Thanks
- Markus Probst

> 
> thanks,
> 
> greg k-h

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]