Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU

Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU

[Andy Shevchenko]

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

Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU

[Andy Shevchenko]

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

Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU

[Andy Shevchenko]

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

Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Andy Shevchenko]

On Tue, Sep 19, 2023 at 12:38:11PM +0200, Marek Behún wrote:
> Add support for GPIOs connected to the MCU on the Turris Omnia board.
> 
> This include:

includes

> - front button pin
> - enable pins for USB regulators
> - MiniPCIe / mSATA card presence pins in MiniPCIe port 0
> - LED output pins from WAN ethernet PHY, LAN switch and MiniPCIe ports
> - on board revisions 32+ also various peripheral resets and another
>   voltage regulator enable pin

Can the main driver provide a regmap and all other use it?

...

> +Date:		August 2023
> +KernelVersion:	6.6

Same as per previous patch review.

...

> +	ret = omnia_mcu_register_gpiochip(mcu);
> +	if (ret)
> +		return ret;
> +
>  	return 0;

	return ..._gpiochip(...);

?

...

> +	switch (cmd) {
> +	case CMD_GENERAL_CONTROL:
> +		buf[1] = val;
> +		buf[2] = mask;
> +		len = 3;
> +		break;
> +
> +	case CMD_EXT_CONTROL:
> +		put_unaligned_le16(val, &buf[1]);
> +		put_unaligned_le16(mask, &buf[3]);
> +		len = 5;
> +		break;
> +
> +	default:
> +		unreachable();

You meant BUG_ON()?

> +	}

...

> +static int omnia_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct omnia_mcu *mcu = gpiochip_get_data(gc);
> +
> +	if (offset == OMNIA_GPIO_PHY_SFP_OFFSET) {
> +		int val;
> +
> +		mutex_lock(&mcu->lock);
> +		val = omnia_cmd_read_bit(mcu->client,
> +					 CMD_GET_EXT_CONTROL_STATUS,
> +					 EXT_CTL_PHY_SFP_AUTO);
> +		mutex_unlock(&mcu->lock);
> +
> +		if (val < 0)
> +			return val;
> +
> +		if (val)
> +			return GPIO_LINE_DIRECTION_IN;

> +		else

Redundant 'else'.

> +			return GPIO_LINE_DIRECTION_OUT;
> +	}
> +
> +	if (omnia_gpios[offset].ctl_cmd)
> +		return GPIO_LINE_DIRECTION_OUT;

> +	else

Ditto.

> +		return GPIO_LINE_DIRECTION_IN;
> +}

...

> +	if (offset == OMNIA_GPIO_PHY_SFP_OFFSET)
> +		return omnia_ctl_cmd(mcu, CMD_EXT_CONTROL, EXT_CTL_PHY_SFP_AUTO,
> +				     EXT_CTL_PHY_SFP_AUTO);
> +
> +	if (gpio->ctl_cmd)
> +		return -EOPNOTSUPP;

I believe internally we use ENOTSUPP.
Ditto for all cases like this.

...

> +	uint16_t val, mask;

So, why uintXX_t out of a sudden?

...

> +	if (gpio->ctl_cmd)
> +		return omnia_ctl_cmd(mcu, gpio->ctl_cmd, val, mask);
> +	else

Redundant 'else'.

> +		return -EOPNOTSUPP;
> +}

...

> +static void omnia_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
> +{
> +	const struct omnia_gpio *gpio = &omnia_gpios[offset];
> +	struct omnia_mcu *mcu = gpiochip_get_data(gc);
> +	u16 val, mask;

> +	int err = 0;

Redundant assignment.

> +	if (!gpio->ctl_cmd)
> +		return;
> +
> +	mask = gpio->ctl_bit;
> +	val = value ? mask : 0;
> +
> +	err = omnia_ctl_cmd(mcu, gpio->ctl_cmd, val, mask);
> +	if (err)
> +		dev_err(&mcu->client->dev, "Cannot set GPIO %u: %d\n",
> +			offset, err);
> +}

...

> +	mutex_lock(&mcu->lock);
> +
> +	if (ctl_mask)
> +		err = omnia_ctl_cmd_unlocked(mcu, CMD_GENERAL_CONTROL, ctl,
> +					     ctl_mask);

> +	if (!err && ext_ctl_mask)
> +		err = omnia_ctl_cmd_unlocked(mcu, CMD_EXT_CONTROL, ext_ctl,
> +					     ext_ctl_mask);

Can it be

	if (err)
		goto out_unlock;

	if (_mask)
		...

?

> +	mutex_unlock(&mcu->lock);

> +	if (err)
> +		dev_err(&mcu->client->dev, "Cannot set GPIOs: %d\n", err);

How is useful this one?

...

> +static bool omnia_gpio_available(struct omnia_mcu *mcu,
> +				 const struct omnia_gpio *gpio)
> +{
> +	if (gpio->feat_mask)
> +		return (mcu->features & gpio->feat_mask) == gpio->feat;
> +	else if (gpio->feat)
> +		return mcu->features & gpio->feat;
> +	else

Redundant 'else':s.

> +		return true;
> +}

...

> +static int omnia_gpio_init_valid_mask(struct gpio_chip *gc,
> +				      unsigned long *valid_mask,
> +				      unsigned int ngpios)
> +{
> +	struct omnia_mcu *mcu = gpiochip_get_data(gc);

> +	bitmap_zero(valid_mask, ngpios);

No need.

Also do you have bitops.h included?

> +	for (int i = 0; i < ngpios; ++i) {
> +		const struct omnia_gpio *gpio = &omnia_gpios[i];
> +
> +		if (!gpio->cmd && !gpio->int_bit)
> +			continue;

Use clear_bit() here...

> +		if (omnia_gpio_available(mcu, gpio))
> +			set_bit(i, valid_mask);

...and assign_bit() here.

> +	}
> +
> +	return 0;
> +}

...

> +	dev_dbg(&mcu->client->dev,
> +		"set int mask %02x %02x %02x %02x %02x %02x %02x %02x\n",
> +		cmd[1], cmd[2], cmd[3], cmd[4], cmd[5], cmd[6], cmd[7], cmd[8]);

%8ph

...

> +	/*
> +	 * Remember which GPIOs have both rising and falling interrupts enabled.
> +	 * For those we will cache their value so that .get() method is faster.
> +	 * We also need to forget cached values of GPIOs that aren't cached
> +	 * anymore.
> +	 */
> +	if (!err) {

	if (err)
		goto out_unlock;

> +		mcu->both = rising & falling;
> +		mcu->is_cached &= mcu->both;
> +	}
> +
> +	mutex_unlock(&mcu->lock);

...

> +static void omnia_irq_init_valid_mask(struct gpio_chip *gc,
> +				      unsigned long *valid_mask,
> +				      unsigned int ngpios)
> +{
> +	struct omnia_mcu *mcu = gpiochip_get_data(gc);

> +	bitmap_zero(valid_mask, ngpios);

No need (see above how).

> +	for (int i = 0; i < ngpios; ++i) {
> +		const struct omnia_gpio *gpio = &omnia_gpios[i];
> +
> +		if (!gpio->int_bit)
> +			continue;
> +
> +		if (omnia_gpio_available(mcu, gpio))
> +			set_bit(i, valid_mask);
> +	}
> +}

...

> +	u8 cmd[9] = {};

Magic number in a few places. Please, use self-explanatory pre-defined constant
instead.

...


> +		dev_err(&mcu->client->dev, "Cannot read pending IRQs: %d\n",
> +			err);

In all functions with help of

	struct device *dev = &mcu->client->dev;

you can make code shorter.

...

> +	rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
> +		 (reply[6] << 24);
> +	falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
> +		  (reply[7] << 24);

With a help of two masks, you can access to the both edges as to 64-bit value
and simplify the code.

...

> +	int ret = omnia_cmd_read_u16(mcu->client, CMD_GET_STATUS_WORD);

Please, split this for better maintenance.

> +	if (ret < 0)
> +		return ret;

...

> +static irqreturn_t omnia_irq_thread_handler(int irq, void *dev_id)
> +{
> +	struct omnia_mcu *mcu = dev_id;
> +	irqreturn_t res = IRQ_NONE;
> +	unsigned long pending;
> +	int i;
> +
> +	if (!omnia_irq_read_pending(mcu, &pending))
> +		return IRQ_NONE;
> +
> +	for_each_set_bit(i, &pending, 32) {
> +		unsigned int nested_irq =
> +			irq_find_mapping(mcu->gc.irq.domain,
> +					 omnia_int_to_gpio_idx[i]);

It's much better to read in a form

		unsigned int nested_irq;
		domain = ...

		nested_irq = irq_find_mapping(domain, omnia_int_to_gpio_idx[i]);

(exactly 80 characters, btw).

> +		handle_nested_irq(nested_irq);

> +		res = IRQ_HANDLED;

No need.

> +	}

> +	return res;

	return IRQ_RETVAL(pending);

> +}

...

> +static ssize_t front_button_mode_show(struct device *dev,
> +				      struct device_attribute *a, char *buf)
> +{
> +	struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));
> +	int val;
> +
> +	if (mcu->features & FEAT_NEW_INT_API)
> +		val = omnia_cmd_read_bit(mcu->client, CMD_GET_STATUS_WORD,
> +					 STS_BUTTON_MODE);
> +	else
> +		val = !!(mcu->last_status & STS_BUTTON_MODE);

> +	if (val < 0)

Can be true only in one case, why to check for the second oner?/

> +		return val;

> +	return sysfs_emit(buf, "%s\n", val ? "cpu" : "mcu");

With a static const array of string literals...

> +}

...

> +	if (sysfs_streq(buf, "mcu"))
> +		val = 0;
> +	else if (sysfs_streq(buf, "cpu"))
> +		val = mask;
> +	else
> +		return -EINVAL;

...and help of sysfs_match_string() you can simplify the code.

...

> +static struct attribute *omnia_mcu_gpio_attrs[] = {
> +	&dev_attr_front_button_mode.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group omnia_mcu_gpio_group = {
> +	.attrs = omnia_mcu_gpio_attrs,
> +};

ATTRIBUTE_GROUPS() ?

...

> +	err = devm_gpiochip_add_data(dev, &mcu->gc, mcu);
> +	if (err) {
> +		dev_err(dev, "Cannot add GPIO chip: %d\n", err);
> +		return err;

		return dev_err_probe(...);

Ditto for other places like this in the probe stage.

> +	}

...

> +	err = devm_device_add_group(dev, &omnia_mcu_gpio_group);

No way, no-one should use the API scheduled for removal.
What's wrong with .dev_groups ?

...

> +void omnia_mcu_unregister_gpiochip(struct omnia_mcu *mcu)
> +{
> +	if (!(mcu->features & FEAT_NEW_INT_API))
> +		cancel_delayed_work_sync(&mcu->button_release_emul_work);
> +
> +	mutex_destroy(&mcu->lock);

Wrong order?

> +}

...

>  struct omnia_mcu {
>  	struct i2c_client *client;
>  	const char *type;
>  	u16 features;
> +
> +	/* GPIO chip */
> +	struct gpio_chip gc;

Making this a first member may lead to the better code. Check with bloat-o-meter.

> +	struct mutex lock;
> +	u32 mask, rising, falling, both, cached, is_cached;

> +	/* Old MCU firmware handling needs the following */
> +	u16 last_status;
> +	struct delayed_work button_release_emul_work;

Swapping these two may free a few bytes. Check with pahole tool.

> +	bool button_pressed_emul;
>  };

...

> +	if (!bits) {
> +		*reply = 0;
> +		return 0;
> +	}
> +
> +	ret = omnia_cmd_read(client, cmd, reply, (ilog2(bits) >> 3) + 1);

> +

Redundant blank line.

> +	if (ret >= 0)

> +		*reply = le32_to_cpu(*reply) & bits;

Huh?! Please, check your code with sparse like

	make W=1 C=1 CF=-D__CHECK_ENDIAN__ ...

> +	return ret < 0 ? ret : 0;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Andy Shevchenko]

On Wed, Sep 20, 2023 at 07:08:18PM +0200, Marek Behún wrote:
> On Tue, 19 Sep 2023 16:00:39 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
> > On Tue, Sep 19, 2023 at 12:38:11PM +0200, Marek Behún wrote:

...

> > Ditto for all cases like this.
> 
> checkpatch warns: 
>   ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP

So, fix checkpatch then. It has false positives, because it doesn't know
if the actual error code will sneak outside the kernel or not. But in
the subsystem we internally use ENOTSUPP.

...

> Linus suggested using guards, so I will refactor this away.

Even better.

...

> > > +		dev_err(&mcu->client->dev, "Cannot set GPIOs:
> > >   %d\n", err);  
> > 
> > How is useful this one?
> 
> The function does not return, this way we will know something has gone
> wrong. I am used to do this from the networking subsystem, where people
> at least from mv88e6xxx wanted such behavior. Do you want me to drop
> this?

You are the developer and owner of the specific hardware, if you have a good
justification for this message _in production_, then leave it, otherwise drop.

...

> > > +	bitmap_zero(valid_mask, ngpios);  
> > 
> > No need.
> > 
> > Also do you have bitops.h included?
> 
> bitmap.h actually for this

Right, but I already thought a step forward, when you drop bitmap APIs,
the bitops are still in place.

...

> > > +	rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
> > > +		 (reply[6] << 24);
> > > +	falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
> > > +		  (reply[7] << 24);  
> > 
> > With a help of two masks, you can access to the both edges as to
> >   64-bit value and simplify the code.
> 
> Huh? As in
>   rising = reply & 0x00ff00ff00ff00ff;
>   falling = reply & 0xff00ff00ff00ff00;
> ?
> But then I can't or the rising bit with the corresponding falling bit
> to get pending...
> Or I guess i can with:
>   pending = rising & (pending >> 8);
> 
> Am I understanding you correctly?
> 
> But then I would need to store the mask in driver data as a 64-bit
> value with half the data not used. Also the CPU is 32-bit.

If you use proper bitmaps, perhaps this will be easier. You can use one for
each and merge them whenever you want (with bitmap_or() call) or split (with
bitmap_and() respectively):

	bitmap_or(full, raising, failing); // merge
	bitmap_and(raising, full, rasing_mask); // split

...

> > ATTRIBUTE_GROUPS() ?
> 
> I only want to create ane attribute_group. (I am using
> devm_device_add_group(), not devm_device_add_groups()).

...and you should not use that APIs.

> > > +	err = devm_device_add_group(dev, &omnia_mcu_gpio_group);  
> > 
> > No way, no-one should use the API scheduled for removal.
> > What's wrong with .dev_groups ?
> 
> Can I add them conditionally? GPIO chip is always created, but the
> patch 4/7 only adds the attributes conditionally. Is it possible via
> .dev_groups?

Yes. You use __ATTRIBUTE_GROUPS() and .is_visible callback.

...

> > > +void omnia_mcu_unregister_gpiochip(struct omnia_mcu *mcu)
> > > +{
> > > +	if (!(mcu->features & FEAT_NEW_INT_API))
> > > +
> > >   cancel_delayed_work_sync(&mcu->button_release_emul_work); +
> > > +	mutex_destroy(&mcu->lock);  
> > 
> > Wrong order?
> 
> No, the mutex may be used in the work. Can't destroy it first. Or am I
> misunderstanding something?

I mean you are using a lot of devm(), can mutex be used in IRQ or whatever
that can be triggered after this call?

...

> > >  	struct i2c_client *client;
> > >  	const char *type;
> > >  	u16 features;
> > > +
> > > +	/* GPIO chip */
> > > +	struct gpio_chip gc;  
> > 
> > Making this a first member may lead to the better code. Check with
> >   bloat-o-meter.
> 
> kabel@dellmb ~/linux $ ./scripts/bloat-o-meter \
>   turris-omnia-mcu.o turris-omnia-mcu-gpiochip-first.o
> add/remove: 0/0 grow/shrink: 9/0 up/down: 52/0 (52)
> Function                                     old     new   delta
> omnia_mcu_register_gpiochip                  840     852     +12
> omnia_mcu_probe                              696     708     +12
> omnia_mcu_unregister_gpiochip                 20      24      +4
> omnia_irq_bus_sync_unlock                    256     260      +4
> omnia_irq_bus_lock                            36      40      +4
> omnia_gpio_get_multiple                      872     876      +4
> omnia_gpio_get                               372     376      +4
> fw_features_show                              28      32      +4
> front_button_mode_show                       260     264      +4
> Total: Before=10468, After=10520, chg +0.50%
> 
> Seems the code grew when I swapped it.

Thanks for checking! It means that access to client pointer is needed
more often.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Andy Shevchenko]

On Thu, Sep 21, 2023 at 10:25:01PM +0200, Marek Behún wrote:
> On Thu, 21 Sep 2023 12:55:08 +0300
> Andy Shevchenko <andy@kernel.org> wrote:

...

> > > > > +	rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
> > > > > +		 (reply[6] << 24);
> > > > > +	falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
> > > > > +		  (reply[7] << 24);    
> > > > 
> > > > With a help of two masks, you can access to the both edges as to
> > > >   64-bit value and simplify the code.  
> > > 
> > > Huh? As in
> > >   rising = reply & 0x00ff00ff00ff00ff;
> > >   falling = reply & 0xff00ff00ff00ff00;
> > > ?
> > > But then I can't or the rising bit with the corresponding falling bit
> > > to get pending...
> > > Or I guess i can with:
> > >   pending = rising & (pending >> 8);
> > > 
> > > Am I understanding you correctly?
> > > 
> > > But then I would need to store the mask in driver data as a 64-bit
> > > value with half the data not used. Also the CPU is 32-bit.  
> > 
> > If you use proper bitmaps, perhaps this will be easier. You can use one for
> > each and merge them whenever you want (with bitmap_or() call) or split (with
> > bitmap_and() respectively):
> > 
> > 	bitmap_or(full, raising, failing); // merge
> > 	bitmap_and(raising, full, rasing_mask); // split
> 
> Hmm. But then what? I or the result and use it as pending interrupt
> bitmap, to be iterated over. The indexes of the bits correspond to the
> constants in the MCU API.
> 
> So after your suggestion I have rising and falling containgin
>   rising = 00rr00rr00rr00rr; /* r means rising bits */
>   falling = 00ff00ff00ff00ff; /* f means falling bits */
>   pending = rising | falling;
> which means:
>   pending = pp00pp00pp00pp; /* p means pending bits */
> But these bit positions do not correspond to the interrupt number
> anymore.
> 
> I still think the de-interleaving of the buffer from
>   rr ff rr ff rr ff rr ff
> into two words:
>   rising = rrrrrrrr;
>   falling = ffffffff;
> is simpler...

There are two sides of this: OS and hardware. See Xilinx GPIO driver how it's
made there. But before going that way, check on
https://lore.kernel.org/all/ZOMmuZuhdjA6mdIG@smile.fi.intel.com/
That APIs you would need I am pretty sure.

...

> > > > > +	if (!(mcu->features & FEAT_NEW_INT_API))
> > > > > +
> > > > >   cancel_delayed_work_sync(&mcu->button_release_emul_work); +
> > > > > +	mutex_destroy(&mcu->lock);    
> > > > 
> > > > Wrong order?  
> > > 
> > > No, the mutex may be used in the work. Can't destroy it first. Or am I
> > > misunderstanding something?  
> > 
> > I mean you are using a lot of devm(), can mutex be used in IRQ or whatever
> > that can be triggered after this call?
> 
> OK, I think I need to free the irq before canceling the work. Thank you!

Can you rather switch everything to be devm managed?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Andy Shevchenko]

On Mon, Sep 25, 2023 at 12:03:56PM +0200, Marek Behún wrote:
> On Fri, 22 Sep 2023 17:18:56 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
> > On Thu, Sep 21, 2023 at 10:25:01PM +0200, Marek Behún wrote:
> > > On Thu, 21 Sep 2023 12:55:08 +0300
> > > Andy Shevchenko <andy@kernel.org> wrote:  

...

> > > > > > > +	rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
> > > > > > > +		 (reply[6] << 24);
> > > > > > > +	falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
> > > > > > > +		  (reply[7] << 24);      
> > > > > > 
> > > > > > With a help of two masks, you can access to the both edges as to
> > > > > >   64-bit value and simplify the code.    
> > > > > 
> > > > > Huh? As in
> > > > >   rising = reply & 0x00ff00ff00ff00ff;
> > > > >   falling = reply & 0xff00ff00ff00ff00;
> > > > > ?
> > > > > But then I can't or the rising bit with the corresponding falling bit
> > > > > to get pending...
> > > > > Or I guess i can with:
> > > > >   pending = rising & (pending >> 8);
> > > > > 
> > > > > Am I understanding you correctly?
> > > > > 
> > > > > But then I would need to store the mask in driver data as a 64-bit
> > > > > value with half the data not used. Also the CPU is 32-bit.    
> > > > 
> > > > If you use proper bitmaps, perhaps this will be easier. You can use one for
> > > > each and merge them whenever you want (with bitmap_or() call) or split (with
> > > > bitmap_and() respectively):
> > > > 
> > > > 	bitmap_or(full, raising, failing); // merge
> > > > 	bitmap_and(raising, full, rasing_mask); // split  
> > > 
> > > Hmm. But then what? I or the result and use it as pending interrupt
> > > bitmap, to be iterated over. The indexes of the bits correspond to the
> > > constants in the MCU API.
> > > 
> > > So after your suggestion I have rising and falling containgin
> > >   rising = 00rr00rr00rr00rr; /* r means rising bits */
> > >   falling = 00ff00ff00ff00ff; /* f means falling bits */
> > >   pending = rising | falling;
> > > which means:
> > >   pending = pp00pp00pp00pp; /* p means pending bits */
> > > But these bit positions do not correspond to the interrupt number
> > > anymore.
> > > 
> > > I still think the de-interleaving of the buffer from
> > >   rr ff rr ff rr ff rr ff
> > > into two words:
> > >   rising = rrrrrrrr;
> > >   falling = ffffffff;
> > > is simpler...  
> > 
> > There are two sides of this: OS and hardware. See Xilinx GPIO driver how it's
> > made there. But before going that way, check on
> > https://lore.kernel.org/all/ZOMmuZuhdjA6mdIG@smile.fi.intel.com/
> > That APIs you would need I am pretty sure.
> 
> Andy, thank you for patience in reviewing this.
> 
> Hmm. I like the names, scatter and gather. In the firmware, I used
> interleave and deinterleave, see
>   https://gitlab.nic.cz/turris/hw/omnia_hw_ctrl/-/blob/master/src/drivers/i2c_iface.c#L360
> 
> But those functions work bit-wise. I realize that the I2C transfers in
> the driver are so slow that such bit-wise cycling over a bitmap won't
> matter much, but I still find my original proposal more simple and
> straight-forward. But I will cave if you insist. Please let me know
> (and can I then send your local patch in the series?)

You can. but I need to add test cases there.

Yes, I think the best is to have hardware values and Linux cached ones
to be separated. Let me try my best and send it out this week.

...

> > > > > > > +	if (!(mcu->features & FEAT_NEW_INT_API))
> > > > > > > +
> > > > > > >   cancel_delayed_work_sync(&mcu->button_release_emul_work); +
> > > > > > > +	mutex_destroy(&mcu->lock);      
> > > > > > 
> > > > > > Wrong order?    
> > > > > 
> > > > > No, the mutex may be used in the work. Can't destroy it first. Or am I
> > > > > misunderstanding something?    
> > > > 
> > > > I mean you are using a lot of devm(), can mutex be used in IRQ or whatever
> > > > that can be triggered after this call?  
> > > 
> > > OK, I think I need to free the irq before canceling the work. Thank you!  
> > 
> > Can you rather switch everything to be devm managed?
> 
> There are no devm_ calls for mutex and work initialization. Are you
> suggesting that I should write a release function for the gpio
> sub-driver? Something like
> 
> static void omnia_gpiochip_release(dev, res)
> {
>   cancel_work();
>   mutex_destroy();
> }

Not together, but
- for mutex use devm_add_action_or_reset() as done in many other drivers
  for the same reason;
- for the work we have devm_work_autocancel()
  (you need to include devm-helpers.h)

> int omnia_mcu_register_gpiochip(mcu)
> {
>   ...
>   x = devres_alloc(omnia_gpiochip_release);
>   devres_add(dev, x);
>   ...
> }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Andy Shevchenko]

On Thu, Sep 21, 2023 at 08:42:43PM +0200, Marek Behún wrote:
> On Tue, 19 Sep 2023 16:00:39 +0300
> Andy Shevchenko <andy@kernel.org> wrote:

...

> > > +	mutex_lock(&mcu->lock);
> > > +
> > > +	if (ctl_mask)
> > > +		err = omnia_ctl_cmd_unlocked(mcu, CMD_GENERAL_CONTROL, ctl,
> > > +					     ctl_mask);  
> > 
> > > +	if (!err && ext_ctl_mask)
> > > +		err = omnia_ctl_cmd_unlocked(mcu, CMD_EXT_CONTROL, ext_ctl,
> > > +					     ext_ctl_mask);  
> > 
> > Can it be
> > 
> > 	if (err)
> > 		goto out_unlock;
> > 
> > 	if (_mask)
> > 		...
> > 
> > ?
> 
> Hi Andy,
> 
> so I am refactoring this to use guard(mutex), but now I have this:
> 
> 	guard(mutex, &mcu->lock);
> 
> 	if (ctl_mask) {
> 		err = ...;
> 		if (err)
> 			goto out_err;
> 	}
> 
> 	if (ext_ctl_mask) {
> 		err = ...;
> 		if (err)
> 			goto out_err;
> 	}
> 
> 	return;
> out_err:
> 	dev_err(dev, "Cannot set GPIOs: %d\n", err);
> 
> which clearly is not any better... or at least the original

...which rather means that the design of above is not so good, i.e.
why do you need the same message in the different situations?

>   if (!err && ext_ctl_mask)
> is better IMO.

I disagree.

> Compare with:
> 
> 	guard(mutex, &mcu->lock);
> 
> 	if (ctl_mask)
> 		err = ...;
> 
> 	if (!err && ext_ctl_mask)
> 		err = ...;
> 
> 	if (err)
> 		dev_err(dev, "Cannot set GPIOs: %d\n", err);
> 
> 
> Do you have a better suggestion?

Use different messages (if even needed) for different situations.

With cleanup.h in place you shouldn't supposed to have goto:s
(in simple cases like yours).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Linus Walleij]

Hi Marek,

thanks for your patch! This is a very interesting hardware.

On Tue, Sep 19, 2023 at 12:38 PM Marek Behún <kabel@kernel.org> wrote:

> Add support for GPIOs connected to the MCU on the Turris Omnia board.
>
> This include:
> - front button pin
> - enable pins for USB regulators
> - MiniPCIe / mSATA card presence pins in MiniPCIe port 0
> - LED output pins from WAN ethernet PHY, LAN switch and MiniPCIe ports
> - on board revisions 32+ also various peripheral resets and another
>   voltage regulator enable pin
>
> Signed-off-by: Marek Behún <kabel@kernel.org>

OK all pretty straight-forward devices built on top of GPIO
in something like device tree or ACPI etc.

> --- a/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
> +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-turris-omnia-mcu
> @@ -1,3 +1,19 @@
> +What:          /sys/bus/i2c/devices/<mcu_device>/front_button_mode
> +Date:          August 2023
> +KernelVersion: 6.6
> +Contact:       Marek Behún <kabel@kernel.org>
> +Description:   (RW) The front button on the Turris Omnia router can be
> +               configured either to change the intensity of all the LEDs on the
> +               front panel, or to send the press event to the CPU as an
> +               interrupt.
> +
> +               This file switches between these two modes:
> +               - "mcu" makes the button press event be handled by the MCU to
> +                 change the LEDs panel intensity.
> +               - "cpu" makes the button press event be handled by the CPU.
> +
> +               Format: %s.

I understand what this does, but why does this have to be configured
at runtime from userspace sysfs? Why can't this just be a property in
device tree or ACPI (etc)? I don't imagine a user is going to change
this back and forth are they? They likely want one or another.

> --- a/drivers/platform/cznic/turris-omnia-mcu-base.c
> +++ b/drivers/platform/cznic/turris-omnia-mcu-base.c
> @@ -222,9 +222,20 @@ static int omnia_mcu_probe(struct i2c_client *client)
>                 return -ENODATA;
>         }
>
> +       ret = omnia_mcu_register_gpiochip(mcu);
> +       if (ret)
> +               return ret;

I guess it's OK to have a GPIOchip with the rest of the stuff, there
are a few precedents, such as drivers/bcma.

> +static const char * const omnia_mcu_gpio_names[64] = {

I would name these _templates since it is not the final names
that will be used. Very nice with all debug names though!

> +#define _DEF_GPIO(_cmd, _ctl_cmd, _bit, _ctl_bit, _int_bit, _feat, _feat_mask) \
> +       {                                                               \
> +               .cmd = _cmd,                                            \
> +               .ctl_cmd = _ctl_cmd,                                    \
> +               .bit = _bit,                                            \
> +               .ctl_bit = _ctl_bit,                                    \
> +               .int_bit = _int_bit,                                    \
> +               .feat = _feat,                                          \
> +               .feat_mask = _feat_mask,                                \
> +       }
> +#define _DEF_GPIO_STS(_name) \
> +       _DEF_GPIO(CMD_GET_STATUS_WORD, 0, STS_ ## _name, 0, INT_ ## _name, 0, 0)
> +#define _DEF_GPIO_CTL(_name) \
> +       _DEF_GPIO(CMD_GET_STATUS_WORD, CMD_GENERAL_CONTROL, STS_ ## _name, \
> +                 CTL_ ## _name, 0, 0, 0)
> +#define _DEF_GPIO_EXT_STS(_name, _feat) \
> +       _DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \
> +                 INT_ ## _name, FEAT_ ## _feat | FEAT_EXT_CMDS, \
> +                 FEAT_ ## _feat | FEAT_EXT_CMDS)
> +#define _DEF_GPIO_EXT_STS_LED(_name, _ledext) \
> +       _DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \
> +                 INT_ ## _name, FEAT_LED_STATE_ ## _ledext, \
> +                 FEAT_LED_STATE_EXT_MASK)
> +#define _DEF_GPIO_EXT_STS_LEDALL(_name) \
> +       _DEF_GPIO(CMD_GET_EXT_STATUS_DWORD, 0, EXT_STS_ ## _name, 0, \
> +                 INT_ ## _name, FEAT_LED_STATE_EXT_MASK, 0)
> +#define _DEF_GPIO_EXT_CTL(_name, _feat) \
> +       _DEF_GPIO(CMD_GET_EXT_CONTROL_STATUS, CMD_EXT_CONTROL, \
> +                 EXT_CTL_ ## _name, EXT_CTL_ ## _name, 0, \
> +                 FEAT_ ## _feat | FEAT_EXT_CMDS, \
> +                 FEAT_ ## _feat | FEAT_EXT_CMDS)
> +#define _DEF_INT(_name) \
> +       _DEF_GPIO(0, 0, 0, 0, INT_ ## _name, 0, 0)

Ugh that's really hard to read and understand, but I can't think of
anything better
so I guess we live with it.


> +static const struct omnia_gpio {
> +       u8 cmd, ctl_cmd;
> +       u32 bit, ctl_bit;
> +       u32 int_bit;

If they are really just ONE bit I would actually use an int, encode
the bit number rather than the mask, and then use BIT( ..->int_bit)
from <linux/bits.h> everywhere I need a mask. No strong preference
though.

> +/* mapping from interrupts to indexes of GPIOs in the omnia_gpios array */
> +static const unsigned int omnia_int_to_gpio_idx[32] = {
> +       [ilog2(INT_CARD_DET)]           = 4,
> +       [ilog2(INT_MSATA_IND)]          = 5,
> +       [ilog2(INT_USB30_OVC)]          = 6,

ilog2 is a bit unituitive for a programmer, are you a schooled mathematician
Marek? ;)

It is also a consequence of using a bitmask rather than a bit number in
the struct, all ilog2 does is fls-1. So what about just putting the bit number
in the struct and then all of these assignments will look natural as well.

> +/* index of PHY_SFP GPIO in the omnia_gpios array */
> +enum {
> +       OMNIA_GPIO_PHY_SFP_OFFSET = 54,
> +};

I would just use a define for this, the enum isn't very useful for
singular values.

> +static int omnia_ctl_cmd(struct omnia_mcu *mcu, u8 cmd, u16 val, u16 mask)
> +{
> +       int err;
> +
> +       mutex_lock(&mcu->lock);
> +       err = omnia_ctl_cmd_unlocked(mcu, cmd, val, mask);

We just discussed this on an unrelated topic: *_unlocked means it should
be called without need for the appropriate mutex to be locked, since it clearly
requires &mcu->lock to be taken, this should be
omnia_ctl_cmd_locked() as in "call this with the lock held".

I did a quick grep _locked to convince myself about this.
Obvious exampled include wait_event_interruptible_locked_irq()
or wake_up_locked() in the core kernel infrastructure that clearly states
(include/linux/wait.h for wait_event_interruptible_locked():
"It must be called with wq.lock being held."

> +static int omnia_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
(...)
> +       if (gpio->cmd == CMD_GET_STATUS_WORD &&
> +           !(mcu->features & FEAT_NEW_INT_API))
> +               return !!(mcu->last_status & gpio->bit);

So I expect something like return !!(mcu->last_status & BIT(gpio->bit));

> +static int omnia_gpio_get_multiple(struct gpio_chip *gc, unsigned long *mask,
> +                                  unsigned long *bits)
> +{
> +       mutex_lock(&mcu->lock);
(...)
> +               if (err)
> +                       goto err_unlock;
(...)
> +       if (err)
> +               goto err_unlock;
(...)
> +       if (err)
> +               goto err_unlock;
> +
> +       mutex_unlock(&mcu->lock);

This function is kind of a good example of where using scoped guards
from <linux/cleanup.h> can simplify the code. After the initial lock you
can just return on error and let the cleanup handler unlock the mutex,
just guard(mutex)(&mcu->lock);

In the beginning and then it's done, the lock will be held until the
function is exited.

> +static void omnia_gpio_set_multiple(struct gpio_chip *gc, unsigned long *mask,
> +                                   unsigned long *bits)
> +{
(...)

same here.

> +static int omnia_gpio_init_valid_mask(struct gpio_chip *gc,
> +                                     unsigned long *valid_mask,
> +                                     unsigned int ngpios)
> +{
> +       struct omnia_mcu *mcu = gpiochip_get_data(gc);
> +
> +       bitmap_zero(valid_mask, ngpios);
> +
> +       for (int i = 0; i < ngpios; ++i) {
> +               const struct omnia_gpio *gpio = &omnia_gpios[i];
> +
> +               if (!gpio->cmd && !gpio->int_bit)
> +                       continue;
> +
> +               if (omnia_gpio_available(mcu, gpio))
> +                       set_bit(i, valid_mask);

Speaking of locks, isn't this where you should use __set_bit()
rather than set_bit()?

__set_bit() is incomprehensible shorthand for "set_bit_nonatomic".

> +static void omnia_irq_shutdown(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct omnia_mcu *mcu = gpiochip_get_data(gc);
> +       irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +       u32 bit = omnia_gpios[hwirq].int_bit;
> +
> +       mcu->rising &= ~bit;
> +       mcu->falling &= ~bit;

Yeah so here it would be BIT(bit) etc.

> +       if (type & IRQ_TYPE_EDGE_RISING)
> +               mcu->rising |= bit;
> +       else
> +               mcu->rising &= ~bit;

And with bits in place of bitmasks these would be

if ()
  __set_bit(bit, mcu->rising);
else
  __clear_bit(bit, mcu->rising);

etc

> +static void omnia_irq_init_valid_mask(struct gpio_chip *gc,
> +                                     unsigned long *valid_mask,
> +                                     unsigned int ngpios)
> +{
> +       struct omnia_mcu *mcu = gpiochip_get_data(gc);
> +
> +       bitmap_zero(valid_mask, ngpios);
> +
> +       for (int i = 0; i < ngpios; ++i) {
> +               const struct omnia_gpio *gpio = &omnia_gpios[i];
> +
> +               if (!gpio->int_bit)
> +                       continue;
> +
> +               if (omnia_gpio_available(mcu, gpio))
> +                       set_bit(i, valid_mask);

__set_bit()

> +static bool omnia_irq_read_pending_new(struct omnia_mcu *mcu,
> +                                      unsigned long *pending)
> +{
> +       u32 rising, falling;
> +       u8 reply[8] = {};
> +       size_t len;
> +       int err;
> +
> +       /*
> +        * Determine how many bytes we need to read from the reply to the
> +        * CMD_GET_INT_AND_CLEAR command in order to retrieve all unmasked
> +        * interrupts.
> +        */
> +       len = max(((ilog2(mcu->rising & mcu->mask) >> 3) << 1) + 1,
> +                 ((ilog2(mcu->falling & mcu->mask) >> 3) << 1) + 2);

Really hard to read, and again easier if we store the bit number
rather than the bitmask.

> +       mutex_lock(&mcu->lock);

Scoped guard?

> +static void button_release_emul_fn(struct work_struct *work)
> +{
> +       struct omnia_mcu *mcu = container_of(to_delayed_work(work),
> +                                            struct omnia_mcu,
> +                                            button_release_emul_work);
> +
> +       mcu->button_pressed_emul = false;
> +       generic_handle_irq_safe(mcu->client->irq);
> +}
(...)
> +       /*
> +        * The old firmware triggers an interrupt whenever status word changes,
> +        * but does not inform about which bits rose or fell. We need to compute
> +        * this here by comparing with the last status word value.
> +        *
> +        * The STS_BUTTON_PRESSED bit needs special handling, because the old
> +        * firmware clears the STS_BUTTON_PRESSED bit on successful completion
> +        * of the CMD_GET_STATUS_WORD command, resulting in another interrupt:
> +        * - first we get an interrupt, we read the status word where
> +        *   STS_BUTTON_PRESSED is present,
> +        * - MCU clears the STS_BUTTON_PRESSED bit because we read the status
> +        *   word,
> +        * - we get another interrupt because the status word changed again
> +        *   (STS_BUTTON_PRESSED was cleared).
> +        *
> +        * The gpiolib-cdev, gpiolib-sysfs and gpio-keys input driver all call
> +        * the gpiochip's .get() method after an edge event on a requested GPIO
> +        * occurs.
> +        *
> +        * We ensure that the .get() method reads 1 for the button GPIO for some
> +        * time.
> +        */
> +
> +       if (status & STS_BUTTON_PRESSED) {
> +               mcu->button_pressed_emul = true;
> +               mod_delayed_work(system_wq, &mcu->button_release_emul_work,
> +                                msecs_to_jiffies(50));
> +       } else if (mcu->button_pressed_emul) {
> +               status |= STS_BUTTON_PRESSED;
> +       }

I feel a bit icky about this, would be best if Dmitry Torokhov review
such input subsystem-related things.

It looks like a reimplementation of drivers/input/keyboard/gpio_keys.c
so why not just connect a gpio-keys device using either device tree
or a software node? (Dmitry knows all about how to create proper
software nodes for stuff like this, DT is self-evident.)

> +       mcu->gc.names = omnia_mcu_gpio_names;

Do we really support format strings in these names? I don't
think so, or I'm wrong about my own code... (Which wouldn't
be the first time.)

>  #include <asm/byteorder.h>
> +#include <linux/gpio/driver.h>
>  #include <linux/i2c.h>
> +#include <linux/log2.h>

I think we want to get rid of log2.h from this driver.

Yours,
Linus Walleij

Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Andy Shevchenko]

On Wed, Sep 20, 2023 at 01:58:37PM +0200, Linus Walleij wrote:
> On Tue, Sep 19, 2023 at 12:38 PM Marek Behún <kabel@kernel.org> wrote:

...

> > +       if (type & IRQ_TYPE_EDGE_RISING)
> > +               mcu->rising |= bit;
> > +       else
> > +               mcu->rising &= ~bit;
> 
> And with bits in place of bitmasks these would be
> 
> if ()
>   __set_bit(bit, mcu->rising);
> else
>   __clear_bit(bit, mcu->rising);

More precisely __assign_bit() in this case.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Andy Shevchenko]

On Thu, Sep 21, 2023 at 10:14:09PM +0200, Marek Behún wrote:
> On Thu, 21 Sep 2023 21:45:57 +0200
> Marek Behún <kabel@kernel.org> wrote:
> 
> > I could use ffs(x) instead of ilog2(x) + 1.
> 
> Pardon me, I meant fls(). Or maybe get_bitmask_order() from
> linux/bitops.h.

In any case it's bitops.h APIs that you will need and I think it's fine and
Linus will approve that.

-- 
With Best Regards,
Andy Shevchenko