Re: [PATCH v3 4/7] platform: cznic: turris-omnia-mcu: Add support for poweroff and wakeup

[Andy Shevchenko <andy@kernel.org>]

On Mon, Oct 23, 2023 at 04:31:27PM +0200, Marek Behún wrote:
> Add support for true board poweroff (MCU can disable all unnecessary
> voltage regulators) and wakeup at a specified time, implemented via a
> RTC driver so that the rtcwake utility can be used to configure it.

...

> +static int omnia_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct omnia_mcu *mcu = i2c_get_clientdata(client);
> +
> +	mcu->rtc_alarm = rtc_tm_to_time64(&alrm->time);
> +
> +	if (alrm->enabled)
> +		return omnia_cmd_write_u32(client, CMD_SET_WAKEUP,
> +					   mcu->rtc_alarm);

> +	else

Redundant 'else'.

> +		return 0;
> +}

...

> +static int omnia_power_off(struct sys_off_data *data)
> +
> +	struct omnia_mcu *mcu = data->cb_data;
> +	__be32 tmp;
> +	u8 cmd[9];
> +	u16 arg;
> +	int err;
> +
> +	if (mcu->front_button_poweron)
> +		arg = CMD_POWER_OFF_POWERON_BUTTON;
> +	else
> +		arg = 0;
> +
> +	cmd[0] = CMD_POWER_OFF;
> +	put_unaligned_le16(CMD_POWER_OFF_MAGIC, &cmd[1]);
> +	put_unaligned_le16(arg, &cmd[3]);

> +	tmp = cpu_to_be32(get_unaligned_le32(&cmd[1]));
> +	put_unaligned_le32(crc32_be(0xffffffff, (void *)&tmp, sizeof(tmp)),
> +			   &cmd[5]);

Hmm... Why crc32_le*() can't be used?

> +	err = omnia_cmd_write(mcu->client, cmd, sizeof(cmd));
> +	if (err)
> +		dev_err(&mcu->client->dev,
> +			"Unable to send the poweroff command: %d\n", err);
> +
> +	return NOTIFY_DONE;
> +}

...

> +	if (kstrtobool(buf, &val) < 0)
> +		return -EINVAL;

Why is the error code shadowed?

...

> +	struct device *dev = container_of(kobj, typeof(*dev), kobj);

kobj_to_dev()?

...

> +	/*
> +	 * poweroff and wakeup are available only if POWEROFF_WAKEUP feature is
> +	 * present

Missing period at the end.

> +	 */

...

> +	err = devm_rtc_register_device(mcu->rtcdev);
> +	if (err)
> +		return dev_err_probe(dev, err, "Cannot register RTC device\n");
> +
> +	mcu->front_button_poweron = true;

> +	return err;

Can it be anything than 0?

-- 
With Best Regards,
Andy Shevchenko