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

[Andy Shevchenko <andy@kernel.org>]

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