Re: [PATCH v9 4/9] mfd: add base driver for qnap-mcu devices

[Kees Bakker <kees@ijzerbout.nl>]

Op 07-11-2024 om 12:47 schreef Heiko Stuebner:
> These microcontroller units are used in network-attached-storage devices
> made by QNAP and provide additional functionality to the system.
>
> This adds the base driver that implements the serial protocol via
> serdev and additionally hooks into the poweroff handlers to turn
> off the parts of the system not supplied by the general PMIC.
>
> Turning off (at least the TSx33 devices using Rockchip SoCs) consists of
> two separate actions. Turning off the MCU alone does not turn off the main
> SoC and turning off only the SoC/PMIC does not turn off the hard-drives.
> Also if the MCU is not turned off, the system also won't start again until
> it is unplugged from power.
>
> So on shutdown the MCU needs to be turned off separately before the
> main PMIC.
>
> The protocol spoken by the MCU is sadly not documented, but was
> obtained by listening to the chatter on the serial port, as thankfully
> the "hal_app" program from QNAPs firmware allows triggering all/most
> MCU actions from the command line.
>
> The implementation of how to talk to the serial device got some
> inspiration from the rave-sp servdev driver.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>   MAINTAINERS                  |   6 +
>   drivers/mfd/Kconfig          |  13 ++
>   drivers/mfd/Makefile         |   2 +
>   drivers/mfd/qnap-mcu.c       | 338 +++++++++++++++++++++++++++++++++++
>   include/linux/mfd/qnap-mcu.h |  26 +++
>   5 files changed, 385 insertions(+)
>   create mode 100644 drivers/mfd/qnap-mcu.c
>   create mode 100644 include/linux/mfd/qnap-mcu.h
>
> [...]
> diff --git a/drivers/mfd/qnap-mcu.c b/drivers/mfd/qnap-mcu.c
> new file mode 100644
> index 000000000000..4be39d8b2905
> --- /dev/null
> +++ b/drivers/mfd/qnap-mcu.c
> [...]
> +int qnap_mcu_exec(struct qnap_mcu *mcu,
> +		  const u8 *cmd_data, size_t cmd_data_size,
> +		  u8 *reply_data, size_t reply_data_size)
> +{
> +	unsigned char rx[QNAP_MCU_RX_BUFFER_SIZE];
> +	size_t length = reply_data_size + QNAP_MCU_CHECKSUM_SIZE;
> +	struct qnap_mcu_reply *reply = &mcu->reply;
> +	int ret = 0;
> +
> +	if (length > sizeof(rx)) {
> +		dev_err(&mcu->serdev->dev, "expected data too big for receive buffer");
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&mcu->bus_lock);
> +
> +	reply->data = rx,
> +	reply->length = length,
> +	reply->received = 0,
> +	reinit_completion(&reply->done);
> +
> +	qnap_mcu_write(mcu, cmd_data, cmd_data_size);
> +
> +	serdev_device_wait_until_sent(mcu->serdev, msecs_to_jiffies(QNAP_MCU_TIMEOUT_MS));
> +
> +	if (!wait_for_completion_timeout(&reply->done, msecs_to_jiffies(QNAP_MCU_TIMEOUT_MS))) {
> +		dev_err(&mcu->serdev->dev, "Command timeout\n");
> +		ret = -ETIMEDOUT;
> +	} else {
> +		u8 crc = qnap_mcu_csum(rx, reply_data_size);
Here `rx` is still not initialized.
> +
> +		if (crc != rx[reply_data_size]) {
> +			dev_err(&mcu->serdev->dev,
> +				"Invalid Checksum received\n");
> +			ret = -EIO;
> +		} else {
> +			memcpy(reply_data, rx, reply_data_size);
> +		}
> +	}
> +
> +	mutex_unlock(&mcu->bus_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(qnap_mcu_exec);
>