Hi Lee,
thanks a lot for your review,
Am Freitag, 16. August 2024, 19:13:36 CEST schrieb Lee Jones:
> On Sat, 10 Aug 2024, Heiko Stuebner wrote:
> > +/*
> > + * MFD core driver for the MCU in Qnap NAS devices that is connected
>
> No such thing as an "MFD". Please describe the device.
>
> Is it QNAP or Qnap? Please be consistent.
Looks like QNAP spells itself in all upper case on their website, so I did
go with that
> > + * via a dedicated UART port
> > + *
> > + * Copyright (C) 2024 Heiko Stuebner <heiko@xxxxxxxxx>
> > + */
> > +
> > +#include <linux/export.h>
>
> What is this used for?
the #define EXPORT_SYMBOL_GPL that gets used below lives in that file.
> > + * struct qnap_mcu - QNAP NAS embedded controller
> > + *
> > + * @serdev: Pointer to underlying serdev
> > + * @bus_lock: Lock to serialize access to the device
> > + * @reply_lock: Lock protecting @reply
> > + * @reply: Pointer to memory to store reply payload
> > + * @variant: Device variant specific information
> > + * @version: MCU firmware version
> > + */
> > +struct qnap_mcu {
> > + struct serdev_device *serdev;
> > + /* Serialize access to the device */
>
> Comments and K-doc is OOT.
I don't really know what OOT means, but I guess you mean that only
one or the other is needed?
Though somehow checkpatch was very unhappy without those comments directly
above the mutex declaration. But I can of course remove these additional
comments
> > + mutex_lock(&mcu->reply_lock);
> > + if (!reply) {
> > + dev_warn(dev, "received %zu bytes, we were not waiting for\n",
> > + size);
> > + mutex_unlock(&mcu->reply_lock);
>
> guard(mutex)?
thanks a lot for pointing out this neat feature
> > +int qnap_mcu_exec_with_ack(struct qnap_mcu *mcu,
> > + const u8 *cmd_data, size_t cmd_data_size)
> > +{
> > + u8 ack[2];
> > + int ret;
> > +
> > + ret = qnap_mcu_exec(mcu, cmd_data, cmd_data_size, ack, sizeof(ack));
> > + if (ret)
> > + return ret;
> > +
> > + /* Should return @0 */
> > + if (ack[0] != 0x40 || ack[1] != 0x30) {
>
> Why not use the char variants?
Consistency, I guess.
While the basic commands _seem_ to use values equivalent to ascii
characters, I've also seen other commands where the values are not based
on those but use real numbers / hex values for data instead.
So I opted to go with hex values for all and kept the comments, if someone
wants to get started talking to their MCU via a terminal.
> > +
> > +/*
> > + * The MCU controls power to the peripherals but not the CPU.
> > + *
> > + * So using the pmic to power off the system keeps the MCU and hard-drives
> > + * running. This also then prevents the system from turning back on until
> > + * the MCU is turned off by unplugging the power-cable.
> > + * Turning off the MCU alone on the other hand turns off the hard-drives,
> > + * LEDs, etc while the main SoC stays running - including its network ports.
> > + */
> > +static int qnap_mcu_power_off(struct sys_off_data *data)
> > +{
> > + struct qnap_mcu *mcu = data->cb_data;
> > + int ret;
> > + u8 cmd[] = {
> > + [0] = 0x40, /* @ */
> > + [1] = 0x43, /* C */
> > + [2] = 0x30 /* 0 */
> > + };
>
> u8 cmd [] = { '@', 'C', '0' }; ?
see above.
I guess this is a style choice, we could of course go with
u8 cmd[] = { 0x40, 0x43, 0x30 }
if you prefer that.
> > +static int qnap_mcu_probe(struct serdev_device *serdev)
> > +{
> > + struct device *dev = &serdev->dev;
> > + struct qnap_mcu *mcu;
> > + int ret;
> > +
> > + mcu = devm_kzalloc(dev, sizeof(*mcu), GFP_KERNEL);
> > + if (!mcu)
> > + return -ENOMEM;
> > +
> > + mcu->serdev = serdev;
> > + dev_set_drvdata(dev, mcu);
> > +
> > + mcu->variant = of_device_get_match_data(dev);
> > + if (!mcu->variant)
> > + return -ENODEV;
> > +
> > + mutex_init(&mcu->bus_lock);
> > + mutex_init(&mcu->reply_lock);
>
> Can you not get away with a single lock?
Of course all behaviour information of the device come from observation
alone right now, but it does look like the MCU does not cause
transmissions of its own, but only as a reply to a sent command.
So yes, I can probably do away with the whole reply assignment and just
use one dataset and thus just the main lock.
Heiko
