linux-rtc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU
       [not found] ` <20230919103815.16818-3-kabel@kernel.org>
@ 2023-09-19 12:29   ` Andy Shevchenko
       [not found]     ` <20230919171638.19bc1619@dellmb>
  0 siblings, 1 reply; 4+ messages in thread
From: Andy Shevchenko @ 2023-09-19 12:29 UTC (permalink / raw)
  To: Marek Behún
  Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
	Bartosz Golaszewski, linux-gpio, Alessandro Zummo,
	Alexandre Belloni, linux-rtc, Wim Van Sebroeck, Guenter Roeck,
	linux-watchdog

On Tue, Sep 19, 2023 at 12:38:10PM +0200, Marek Behún wrote:
> Add a new platform driver that exposes the features provided by the
> microcontroller on the Turris Omnia board.

> This commit adds the basic skeleton for the driver.

Read "Submitting Patches" documentation (find "This patch" in it) and amend
your commit message accordingly.

...

> +Date:		August 2023
> +KernelVersion:	6.6

Wrong and outdated. Use phb-crystall-ball to find the closest possible values
for both parameters here.

Ditto for all others with the same issue.

Note, it likely might be part of v6.7, not earlier.

...

> @@ -11,3 +11,4 @@ obj-$(CONFIG_OLPC_EC)		+= olpc/
>  obj-$(CONFIG_GOLDFISH)		+= goldfish/
>  obj-$(CONFIG_CHROME_PLATFORMS)	+= chrome/
>  obj-$(CONFIG_SURFACE_PLATFORMS)	+= surface/
> +obj-$(CONFIG_CZNIC_PLATFORMS)	+= cznic/

Why not ordered (to some extent) here (as you did in the other file)?

...

> +turris-omnia-mcu-objs		:= turris-omnia-mcu-base.o

objs is for user space tools. Kernel code should use m,y.

...

> +#include <linux/hex.h>
> +#include <linux/module.h>

Missing types.h, sysfs.h, mod_devicetable.h, i2c.h, ...

...

> +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
> +				  u8 *version)
> +{
> +	u8 reply[20];
> +	int ret;
> +
> +	ret = omnia_cmd_read(mcu->client, bootloader ? CMD_GET_FW_VERSION_BOOT :
> +						       CMD_GET_FW_VERSION_APP,
> +			     reply, sizeof(reply));

> +	if (ret < 0)

Can it return positive value? What would be the meaning of it?

> +		return ret;

> +	version[40] = '\0';

How do you know the version has enough space?

> +	bin2hex(version, reply, 20);
> +
> +	return 0;
> +}
> +
> +static ssize_t fw_version_hash_show(struct device *dev, char *buf,
> +				    bool bootloader)
> +{
> +	struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));
> +	u8 version[41];

> +	int err;

In two near functions you already inconsistent in the naming of the return code
variable. Be consistent across all your code, i.e. choose one name and use it
everywhere.

> +	err = omnia_get_version_hash(mcu, bootloader, version);
> +	if (err)
> +		return err;
> +
> +	return sysfs_emit(buf, "%s\n", version);
> +}

...

> +	return sysfs_emit(buf, "%#x\n", mcu->features);

"0" will be printed differently, is this on purpose?

...

> +	int ret = omnia_cmd_read_u8(to_i2c_client(dev), CMD_GET_RESET);
> +
> +	if (ret < 0)
> +		return ret;

Better from maintenance perspective to have

	int ret;

	ret = omnia_cmd_read_u8(to_i2c_client(dev), CMD_GET_RESET);
	if (ret < 0)
		return ret;

...

> +static struct attribute *omnia_mcu_attrs[] = {
> +	&dev_attr_fw_version_hash_application.attr,
> +	&dev_attr_fw_version_hash_bootloader.attr,
> +	&dev_attr_fw_features.attr,
> +	&dev_attr_mcu_type.attr,
> +	&dev_attr_reset_selector.attr,

> +	NULL,

No comma for the terminator line. It will make your code robust at compile time
against some misplaced entries.

> +};

> +

Unneeded blank line.

> +ATTRIBUTE_GROUPS(omnia_mcu);

...

> +	u8 version[41];

This magic sizes should go away. Use predefined constant, or allocate on the
heap, depending on the case(s) you have.

...

> +	int status;

My gosh, it's a _third_ name for the same!

> +	status = omnia_cmd_read_u16(mcu->client, CMD_GET_STATUS_WORD);
> +	if (status < 0)
> +		return status;

...

> +	for (int i = 0; i < ARRAY_SIZE(features); ++i) {

Why signed?
Why pre-increment?

> +		if (!(mcu->features & features[i].mask)) {

Wouldn't be better

		if (mcu->features & features[i].mask)
			continue;

?

> +			omnia_info_missing_feature(dev, features[i].name);
> +			suggest_fw_upgrade = true;
> +		}
> +	}

...

> +		dev_info(dev,
> +			 "Consider upgrading MCU firmware with the omnia-mcutool utility.\n");

You have so-o many dev_info() calls, are you sure you not abusing use of that?

...

> +	if (ret) {
> +		dev_err(dev, "Cannot determine MCU supported features: %d\n",
> +			ret);
> +		return ret;

		return dev_err_probe(...);

> +	}

...

> +	if (!client->irq) {
> +		dev_err(dev, "IRQ resource not found\n");
> +		return -ENODATA;
> +	}

Why you need to allocate memory, go through the initial checks, etc and then
fail? What's wrong with checking this first?

Why -ENODATA?!

...

> +static const struct of_device_id of_omnia_mcu_match[] = {
> +	{ .compatible = "cznic,turris-omnia-mcu", },

Inner comma is not needed.

> +	{},

No comma for the terminator entry.

> +};

...

> +static const struct i2c_device_id omnia_mcu_id[] = {
> +	{ "turris-omnia-mcu", 0 },

', 0' part is redundant.

> +	{ }
> +};

...

> +static struct i2c_driver omnia_mcu_driver = {
> +	.probe		= omnia_mcu_probe,
> +	.id_table	= omnia_mcu_id,
> +	.driver		= {
> +		.name	= "turris-omnia-mcu",
> +		.of_match_table = of_omnia_mcu_match,
> +		.dev_groups = omnia_mcu_groups,
> +	},
> +};

> +

Redundant blank line.

> +module_i2c_driver(omnia_mcu_driver);

...

> +#ifndef __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H
> +#define __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H

WE_LIKE_VERY_LONG_AND_OVERFLOWED_DEFINITIONS_H!

...

> +#include <asm/byteorder.h>

This usually goes after linux/*.h.

> +#include <linux/i2c.h>

Missing types.h, errno.h, ...

+ blank line?

> +#include <linux/turris-omnia-mcu-interface.h>

...

> +	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (likely(ret == ARRAY_SIZE(msgs)))

Why likely()? Please, justify.

> +		return len;

> +	else if (ret < 0)
> +		return ret;
> +	else
> +		return -EIO;

In both cases the 'else' is redundant. Moreover, the usual pattern is to check
for the errors first.

> +}

...

> +#ifndef __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H
> +#define __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H

My gosh!

...

> +#include <linux/bits.h>

> +enum commands_e {

Are you sure the name is unique enough / properly namespaced?
Same Q to all enums.

...

> +	/*CTL_RESERVED		= BIT(2),*/

Missing spaces?
Also, needs a comment why this is commented out.

...

> +	CTL_BOOTLOADER		= BIT(7)

Add trailing comma.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU
       [not found]     ` <20230919171638.19bc1619@dellmb>
@ 2023-09-19 18:27       ` Andy Shevchenko
       [not found]         ` <20230920161953.6d952392@dellmb>
  0 siblings, 1 reply; 4+ messages in thread
From: Andy Shevchenko @ 2023-09-19 18:27 UTC (permalink / raw)
  To: Marek Behún
  Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
	Bartosz Golaszewski, linux-gpio, Alessandro Zummo,
	Alexandre Belloni, linux-rtc, Wim Van Sebroeck, Guenter Roeck,
	linux-watchdog

On Tue, Sep 19, 2023 at 05:16:38PM +0200, Marek Behún wrote:
> On Tue, 19 Sep 2023 15:29:08 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
> > On Tue, Sep 19, 2023 at 12:38:10PM +0200, Marek Behún wrote:

...

> > >  obj-$(CONFIG_GOLDFISH)		+= goldfish/
> > >  obj-$(CONFIG_CHROME_PLATFORMS)	+= chrome/
> > >  obj-$(CONFIG_SURFACE_PLATFORMS)	+= surface/
> > > +obj-$(CONFIG_CZNIC_PLATFORMS)	+= cznic/  
> > 
> > Why not ordered (to some extent) here (as you did in the other file)?
> 
> Where should I put it? The other entries are not ordered, see
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/Makefile?h=v6.6-rc2

With the visible context above, at least after chrome already much better, no?

...

> > > +turris-omnia-mcu-objs		:= turris-omnia-mcu-base.o  
> > 
> > objs is for user space tools. Kernel code should use m,y.
> 
> I don't understand. There are plenty driver using -objs. The driver
> consists of multiple C source files. I thought this should be done as
>   obj-$(CONFIG_XXX) += xxx.o
>   xxx-objs := xxx-a.o xxx-b.o xxx-c.o
> 
> If you choose CONFIG_XXX = y, then the driver will be compiled into
> kernel, if you choose CONFIG_XXX = m, then the driver will be a module
> xxx.ko.

Hmm... Maybe I mixed something... Let me check.
For now than it's okay.

...

> > > +#include <linux/hex.h>
> > > +#include <linux/module.h>  
> > 
> > Missing types.h, sysfs.h, mod_devicetable.h, i2c.h, ...
> 
> i2c.h comes from turris-omnia-mcu.h. The others I will fix. Does the
> the .c file also include all those headers if it includes a local
> header already?

For generic ones, yes.

...

> > > +	if (ret < 0)  
> > 
> > Can it return positive value? What would be the meaning of it?
> 
> Originally I had it return 0 on success, but recently I've had a
> discussion about this same thing with LED subsystem maintainer about
> some LED patches, Lee Jones, who requested the _read function to return
> number of bytes read on success. See at
>   https://lore.kernel.org/linux-leds/20230821122644.GJ1380343@google.com/
> where he says:
>   Read and write APIs, even abstracted ones such as these generally
>   return the number of bytes successfully read and written respectively.
>   If you are going to normalise, then please do so against this
>   standard.
> I do not agree with him, since this is a local command accessor which
> only succeeds if all the bytes are read/written as requested. But I
> adjusted my code as per his request since he is the maintainer.

This is strange. For example, regmap APIs never returns amount of data written
or read. I think it's solely depends on the API. It might be useful for i²c
APIs, in case you can do something about it. but if you have wrappers on top
of that already (meaning not using directly the i2c_*() calls, I dunno the
positive return is anyhow useful.

> I will change the code back so that it returns 0 on success instead of
> the number of bytes written.

> > > +		return ret;  

...

> > > +	version[40] = '\0';  
> > 
> > How do you know the version has enough space?
> 
> The function is only use within the file, and both users know how much
> space it needs. But ok, I shall enforce it with the static keyword as
>   static int
>   omnia_get_version_hash(...,
> 			 u8 version[static 2*OMNIA_FW_VERSION_LEN + 1]);
> Would that be okay?

Perhaps, it's a thing in a category "TIL", and it seems we have so few callers
of that.

...

> > > +	int err;  
> > 
> > In two near functions you already inconsistent in the naming of the
> > return code variable. Be consistent across all your code, i.e. choose
> > one name and use it everywhere.
> 
> I use
>   ret - when the return value can also be positive and mean something
>   err - when the return value is either 0 (no error) or negative errno
> Is this inconsistent?

TBH sounds good, but I close to never had heard about such a distinction in
a Linux kernel source file(s).

...

> > > +	int status;  
> > 
> > My gosh, it's a _third_ name for the same!
> 
> The variable holds value of the status register, which is needed at
> another point in this function. I thought naming it just "ret" would
> cause confusions... Do you really propose to rename it such?

It's not bad per se, just put yourself on the place of somebody who would like
to understand the semantics of all these variables. Maybe you can add a comment
on top explaining that (in short)?

...

> > > +	for (int i = 0; i < ARRAY_SIZE(features); ++i) {  
> > 
> > Why signed?
> 
> Because it is shorter and makes no difference and there are tons of
>   for (int i = 0; i < ARRAY_SIZE(x); i++)
> in the kernel already. But if you really want, I can change it. Please
> let me know.

It just makes harder to get the code, because of possible unneeded questions
like "if it's a signed type, maybe it's on purpose?"

> > Why pre-increment?
> 
> Again, it makes no difference, is widely used in the kernel and I
> personally prefer it. But I will change it if you want.

Pre-increment usually also a sign of something special about the iterator
variable. In non-special cases we use post-increment. And if you count
the use of each you will see the difference.

...

> > > +		dev_info(dev,
> > > +			 "Consider upgrading MCU firmware with the
> > > omnia-mcutool utility.\n");  
> > 
> > You have so-o many dev_info() calls, are you sure you not abusing use
> > of that?
> 
> I want the users to upgrade the MCU firmware. These infos are only
> printed during probe time, and won't be printed at all if the firmware
> is upgraded. Is this a problem?

Depends how really useful they are. If you think it's way to go, go for it.

...

> > > +	if (likely(ret == ARRAY_SIZE(msgs)))  
> > 
> > Why likely()? Please, justify.
> 
> Becuase it is unlikely the I2C transaction will fail. In most cases, it
> does not.

Yes, but why likely() is needed? So, i.o.w. what's the benefit in _this_ case?

> > > +		return len;  

...

> > > +enum commands_e {  
> > 
> > Are you sure the name is unique enough / properly namespaced?
> > Same Q to all enums.
> 
> I can change it. I wanted it to be compatible with the header in the
> firmware repository.

Are these being generated, or also statically written in that repo?
If the latter, I think better to use more unique naming schema in
the kernel.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU
       [not found]         ` <20230920161953.6d952392@dellmb>
@ 2023-09-20 14:47           ` Andy Shevchenko
  0 siblings, 0 replies; 4+ messages in thread
From: Andy Shevchenko @ 2023-09-20 14:47 UTC (permalink / raw)
  To: Marek Behún
  Cc: Gregory CLEMENT, Arnd Bergmann, soc, arm, Linus Walleij,
	Bartosz Golaszewski, linux-gpio, Alessandro Zummo,
	Alexandre Belloni, linux-rtc, Wim Van Sebroeck, Guenter Roeck,
	linux-watchdog

On Wed, Sep 20, 2023 at 04:19:53PM +0200, Marek Behún wrote:
> On Tue, 19 Sep 2023 21:27:04 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
> > On Tue, Sep 19, 2023 at 05:16:38PM +0200, Marek Behún wrote:
> > > On Tue, 19 Sep 2023 15:29:08 +0300
> > > Andy Shevchenko <andy@kernel.org> wrote:  
> > > > On Tue, Sep 19, 2023 at 12:38:10PM +0200, Marek Behún wrote:  

...

> > > > > +	if (likely(ret == ARRAY_SIZE(msgs)))    
> > > > 
> > > > Why likely()? Please, justify.  
> > > 
> > > Becuase it is unlikely the I2C transaction will fail. In most cases, it
> > > does not.  
> > 
> > Yes, but why likely() is needed? So, i.o.w. what's the benefit in _this_ case?
> 
> Compiler optimization (one branch avoided). But I guess this isn't a
> hot path, since I2C is insanely slow anyway. OK, I shall remove the
> likely() usage.

Have you seen the difference in the generated code, btw?

I don't think it will get you one independently on the hot/slow
path.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2 4/7] platform: cznic: turris-omnia-mcu: Add support for poweroff and wakeup
       [not found] ` <20230919103815.16818-5-kabel@kernel.org>
@ 2023-09-22 16:28   ` Marek Behún
  0 siblings, 0 replies; 4+ messages in thread
From: Marek Behún @ 2023-09-22 16:28 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc

Allesandro, Alexandre, would you be able to find some time to review
this patch?

Marek

On Tue, 19 Sep 2023 12:38:12 +0200
Marek Behún <kabel@kernel.org> 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.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  .../sysfs-bus-i2c-devices-turris-omnia-mcu    |  16 ++
>  drivers/platform/cznic/Kconfig                |   4 +
>  drivers/platform/cznic/Makefile               |   1 +
>  .../platform/cznic/turris-omnia-mcu-base.c    |   4 +
>  .../platform/cznic/turris-omnia-mcu-gpio.c    |   2 -
>  .../cznic/turris-omnia-mcu-sys-off-wakeup.c   | 245 ++++++++++++++++++
>  drivers/platform/cznic/turris-omnia-mcu.h     |  19 ++
>  7 files changed, 289 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
> index c0efdf1b3803..d668b8e999e6 100644
> --- a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
> +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
> @@ -14,6 +14,22 @@ Description:	(RW) The front button on the Turris Omnia router can be
>  
>  		Format: %s.
>  
> +What:		/sys/bus/i2c/devices/<mcu_device>/front_button_poweron
> +Date:		August 2023
> +KernelVersion:	6.6
> +Contact:	Marek Behún <kabel@kernel.org>
> +Description:	(RW) Newer versions of the microcontroller firmware of the
> +		Turris Omnia router support powering off the router into true
> +		low power mode. The router can be powered on by pressing the
> +		front button.
> +
> +		This file configures whether front button power on is enabled.
> +
> +		This file is present only if the power off feature is supported
> +		by the firmware.
> +
> +		Format: %i.
> +
>  What:		/sys/bus/i2c/devices/<mcu_device>/fw_features
>  Date:		August 2023
>  KernelVersion:	6.6
> diff --git a/drivers/platform/cznic/Kconfig b/drivers/platform/cznic/Kconfig
> index 3a8c3edcd7e6..0a752aa654fa 100644
> --- a/drivers/platform/cznic/Kconfig
> +++ b/drivers/platform/cznic/Kconfig
> @@ -19,10 +19,14 @@ config TURRIS_OMNIA_MCU
>  	depends on I2C
>  	select GPIOLIB
>  	select GPIOLIB_IRQCHIP
> +	select RTC_CLASS
>  	help
>  	  Say Y here to add support for the features implemented by the
>  	  microcontroller on the CZ.NIC's Turris Omnia SOHO router.
>  	  The features include:
> +	  - board poweroff into true low power mode (with voltage regulators
> +	    disabled) and the ability to configure wake up from this mode (via
> +	    rtcwake)
>  	  - GPIO pins
>  	    - to get front button press events (the front button can be
>  	      configured either to generate press events to the CPU or to change
> diff --git a/drivers/platform/cznic/Makefile b/drivers/platform/cznic/Makefile
> index a6177f5b4fff..6f1470d1f673 100644
> --- a/drivers/platform/cznic/Makefile
> +++ b/drivers/platform/cznic/Makefile
> @@ -3,3 +3,4 @@
>  obj-$(CONFIG_TURRIS_OMNIA_MCU)	+= turris-omnia-mcu.o
>  turris-omnia-mcu-objs		:= turris-omnia-mcu-base.o
>  turris-omnia-mcu-objs		+= turris-omnia-mcu-gpio.o
> +turris-omnia-mcu-objs		+= turris-omnia-mcu-sys-off-wakeup.o
> diff --git a/drivers/platform/cznic/turris-omnia-mcu-base.c b/drivers/platform/cznic/turris-omnia-mcu-base.c
> index 2954151468e5..b507f81ebe0a 100644
> --- a/drivers/platform/cznic/turris-omnia-mcu-base.c
> +++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
> @@ -226,6 +226,10 @@ static int omnia_mcu_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	ret = omnia_mcu_register_sys_off_and_wakeup(mcu);
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/platform/cznic/turris-omnia-mcu-gpio.c b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
> index e8d5eeb4eb31..3d34a32bc7b0 100644
> --- a/drivers/platform/cznic/turris-omnia-mcu-gpio.c
> +++ b/drivers/platform/cznic/turris-omnia-mcu-gpio.c
> @@ -5,8 +5,6 @@
>   * 2023 by Marek Behún <kabel@kernel.org>
>   */
>  
> -#include <asm/unaligned.h>
> -
>  #include "turris-omnia-mcu.h"
>  
>  static const char * const omnia_mcu_gpio_names[64] = {
> diff --git a/drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.c b/drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.c
> new file mode 100644
> index 000000000000..07ae92d819e3
> --- /dev/null
> +++ b/drivers/platform/cznic/turris-omnia-mcu-sys-off-wakeup.c
> @@ -0,0 +1,245 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CZ.NIC's Turris Omnia MCU system off and RTC wakeup driver
> + *
> + * This is not a true RTC driver (in the sense that it does not provide a
> + * real-time clock), rather the MCU implements a wakeup from powered off state
> + * at a specified time relative to MCU boot, and we expose this feature via RTC
> + * alarm, so that it can be used via the rtcwake command, which is the standard
> + * Linux command for this.
> + *
> + * 2023 by Marek Behún <kabel@kernel.org>
> + */
> +
> +#include <linux/crc32.h>
> +#include <linux/delay.h>
> +#include <linux/reboot.h>
> +
> +#include "turris-omnia-mcu.h"
> +
> +static int omnia_get_uptime_wakeup(const struct i2c_client *client, u32 *uptime,
> +				   u32 *wakeup)
> +{
> +	u32 reply[2];
> +	int err;
> +
> +	err = omnia_cmd_read(client, CMD_GET_UPTIME_AND_WAKEUP, reply,
> +			     sizeof(reply));
> +	if (err)
> +		return err;
> +
> +	if (uptime)
> +		*uptime = le32_to_cpu(reply[0]);
> +
> +	if (wakeup)
> +		*wakeup = le32_to_cpu(reply[1]);
> +
> +	return 0;
> +}
> +
> +static int omnia_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	u32 uptime;
> +	int err;
> +
> +	err = omnia_get_uptime_wakeup(to_i2c_client(dev), &uptime, NULL);
> +	if (err)
> +		return err;
> +
> +	rtc_time64_to_tm(uptime, tm);
> +
> +	return 0;
> +}
> +
> +static int omnia_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct omnia_mcu *mcu = i2c_get_clientdata(client);
> +	u32 wakeup;
> +	int err;
> +
> +	err = omnia_get_uptime_wakeup(client, NULL, &wakeup);
> +	if (err)
> +		return err;
> +
> +	alrm->enabled = !!wakeup;
> +	rtc_time64_to_tm(wakeup ?: mcu->rtc_alarm, &alrm->time);
> +
> +	return 0;
> +}
> +
> +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
> +		return 0;
> +}
> +
> +static int omnia_alarm_irq_enable(struct device *dev, unsigned int enabled)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct omnia_mcu *mcu = i2c_get_clientdata(client);
> +
> +	return omnia_cmd_write_u32(client, CMD_SET_WAKEUP,
> +				   enabled ? mcu->rtc_alarm : 0);
> +}
> +
> +static const struct rtc_class_ops omnia_rtc_ops = {
> +	.read_time		= omnia_read_time,
> +	.read_alarm		= omnia_read_alarm,
> +	.set_alarm		= omnia_set_alarm,
> +	.alarm_irq_enable	= omnia_alarm_irq_enable,
> +};
> +
> +static int omnia_power_off(struct sys_off_data *data)
> +{
> +	struct omnia_mcu *mcu = data->cb_data;
> +	u8 cmd[9];
> +	u32 tmp;
> +	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]);
> +
> +	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;
> +}
> +
> +static int omnia_restart(struct sys_off_data *data)
> +{
> +	struct omnia_mcu *mcu = data->cb_data;
> +	u8 cmd[3];
> +	int err;
> +
> +	cmd[0] = CMD_GENERAL_CONTROL;
> +
> +	if (reboot_mode == REBOOT_HARD)
> +		cmd[1] = cmd[2] = CTL_HARD_RST;
> +	else
> +		cmd[1] = cmd[2] = CTL_LIGHT_RST;
> +
> +	err = omnia_cmd_write(mcu->client, cmd, sizeof(cmd));
> +	if (err)
> +		dev_err(&mcu->client->dev,
> +			"Unable to send the restart command: %d\n", err);
> +
> +	/*
> +	 * MCU needs a little bit to process the I2C command, otherwise it will
> +	 * do a light reset based on SOC SYSRES_OUT pin.
> +	 */
> +	mdelay(1);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static ssize_t front_button_poweron_show(struct device *dev,
> +					 struct device_attribute *a, char *buf)
> +{
> +	struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));
> +
> +	return sysfs_emit(buf, "%d\n", mcu->front_button_poweron);
> +}
> +
> +static ssize_t front_button_poweron_store(struct device *dev,
> +					  struct device_attribute *a,
> +					  const char *buf, size_t count)
> +{
> +	struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));
> +	bool val;
> +
> +	if (kstrtobool(buf, &val) < 0)
> +		return -EINVAL;
> +
> +	mcu->front_button_poweron = val;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(front_button_poweron);
> +
> +static struct attribute *omnia_mcu_poweroff_attrs[] = {
> +	&dev_attr_front_button_poweron.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group omnia_mcu_poweroff_group = {
> +	.attrs = omnia_mcu_poweroff_attrs,
> +};
> +
> +int omnia_mcu_register_sys_off_and_wakeup(struct omnia_mcu *mcu)
> +{
> +	struct device *dev = &mcu->client->dev;
> +	int err;
> +
> +	/* MCU restart is always available */
> +	err = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART,
> +					    SYS_OFF_PRIO_FIRMWARE,
> +					    omnia_restart, mcu);
> +	if (err) {
> +		dev_err(dev, "Cannot register system restart handler: %d\n",
> +			err);
> +		return err;
> +	}
> +
> +	/*
> +	 * poweroff and wakeup are available only if POWEROFF_WAKEUP feature is
> +	 * present
> +	 */
> +	if (!(mcu->features & FEAT_POWEROFF_WAKEUP))
> +		return 0;
> +
> +	err = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF,
> +					    SYS_OFF_PRIO_FIRMWARE,
> +					    omnia_power_off, mcu);
> +	if (err) {
> +		dev_err(dev, "Cannot register system power off handler: %d\n",
> +			err);
> +		return err;
> +	}
> +
> +	mcu->rtcdev = devm_rtc_allocate_device(dev);
> +	if (IS_ERR(mcu->rtcdev)) {
> +		err = PTR_ERR(mcu->rtcdev);
> +		dev_err(dev, "Cannot allocate RTC device: %d\n", err);
> +		return err;
> +	}
> +
> +	mcu->rtcdev->ops = &omnia_rtc_ops;
> +	mcu->rtcdev->range_max = U32_MAX;
> +
> +	err = devm_rtc_register_device(mcu->rtcdev);
> +	if (err) {
> +		dev_err(dev, "Cannot register RTC device: %d\n", err);
> +		return err;
> +	}
> +
> +	mcu->front_button_poweron = true;
> +
> +	err = devm_device_add_group(dev, &omnia_mcu_poweroff_group);
> +	if (err)
> +		dev_err(dev, "Cannot add poweroff sysfs attribute group: %d\n",
> +			err);
> +
> +	return err;
> +}
> diff --git a/drivers/platform/cznic/turris-omnia-mcu.h b/drivers/platform/cznic/turris-omnia-mcu.h
> index b5802240aa8d..c40ac07be5f5 100644
> --- a/drivers/platform/cznic/turris-omnia-mcu.h
> +++ b/drivers/platform/cznic/turris-omnia-mcu.h
> @@ -9,10 +9,12 @@
>  #define __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H
>  
>  #include <asm/byteorder.h>
> +#include <asm/unaligned.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/i2c.h>
>  #include <linux/log2.h>
>  #include <linux/mutex.h>
> +#include <linux/rtc.h>
>  #include <linux/turris-omnia-mcu-interface.h>
>  #include <linux/workqueue.h>
>  
> @@ -29,6 +31,11 @@ struct omnia_mcu {
>  	u16 last_status;
>  	struct delayed_work button_release_emul_work;
>  	bool button_pressed_emul;
> +
> +	/* RTC device for configuring wake-up */
> +	struct rtc_device *rtcdev;
> +	u32 rtc_alarm;
> +	bool front_button_poweron;
>  };
>  
>  static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,
> @@ -39,6 +46,17 @@ static inline int omnia_cmd_write(const struct i2c_client *client, void *cmd,
>  	return ret < 0 ? ret : 0;
>  }
>  
> +static inline int omnia_cmd_write_u32(const struct i2c_client *client, u8 cmd,
> +				      u32 val)
> +{
> +	u8 buf[5];
> +
> +	buf[0] = cmd;
> +	put_unaligned_le32(val, &buf[1]);
> +
> +	return omnia_cmd_write(client, buf, sizeof(buf));
> +}
> +
>  static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd, void *reply,
>  				 unsigned int len)
>  {
> @@ -117,5 +135,6 @@ static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
>  
>  int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu);
>  void omnia_mcu_unregister_gpiochip(struct omnia_mcu *mcu);
> +int omnia_mcu_register_sys_off_and_wakeup(struct omnia_mcu *mcu);
>  
>  #endif /* __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H */


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2023-09-22 16:28 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20230919103815.16818-1-kabel@kernel.org>
     [not found] ` <20230919103815.16818-3-kabel@kernel.org>
2023-09-19 12:29   ` [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU Andy Shevchenko
     [not found]     ` <20230919171638.19bc1619@dellmb>
2023-09-19 18:27       ` Andy Shevchenko
     [not found]         ` <20230920161953.6d952392@dellmb>
2023-09-20 14:47           ` Andy Shevchenko
     [not found] ` <20230919103815.16818-5-kabel@kernel.org>
2023-09-22 16:28   ` [PATCH v2 4/7] platform: cznic: turris-omnia-mcu: Add support for poweroff and wakeup Marek Behún

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).