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

[Andy Shevchenko <andy@xxxxxxxxxx>]

On Wed, May 08, 2024 at 12:31:12PM +0200, Marek Behún wrote:
> Add support for GPIOs connected to the MCU on the Turris Omnia board.
> 
> This 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

...

>  static const struct attribute_group *omnia_mcu_groups[] = {
>  	&omnia_mcu_base_group,
> +	&omnia_mcu_gpio_group,
>  	NULL
>  };

Ah, __ATTRIBUTE_GROUPS() can't be used.

...

> +static int omnia_ctl_cmd_locked(struct omnia_mcu *mcu, u8 cmd, u16 val,
> +				u16 mask)

Can be one line as it's only 81 characters long.

...

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

If those variables was defined as unsigned long, these can be just

	__assign_bit()
	__assign_bit()

And other non-atomic bitops elsewhere, like __clear_bit().

...

> + * Feel free to remove this function and its inverse, omnia_mask_deinterleave,
> + * and use an appropriate bitmap_* function once such a function exists.

bitmap_*()

...

> +static void omnia_mask_interleave(u8 *dst, u32 rising, u32 falling)
> +{
> +	for (int i = 0; i < sizeof(u32); ++i) {
> +		dst[2 * i] = rising >> (8 * i);
> +		dst[2 * i + 1] = falling >> (8 * i);
> +	}

Yeah, this can actually be converted to the existing bitmap ops, but I think it
will be a bit an overkill for now. Perhaps we will have something better in the
future.

> +}

+ blank line. (Or is it me who accidentally removed it?)

> +/**
> + * omnia_mask_deinterleave - Deinterleaves the bytes into @rising and @falling
> + *	@src: the source u8 array containing the interleaved bytes
> + *	@rising: pointer where to store the rising mask gathered from @src
> + *	@falling: pointer where to store the falling mask gathered from @src
> + *
> + * This is the inverse function to omnia_mask_interleave.
> + */
> +static void omnia_mask_deinterleave(const u8 *src, u32 *rising, u32 *falling)
> +{
> +	*rising = *falling = 0;
> +
> +	for (int i = 0; i < sizeof(u32); ++i) {
> +		*rising |= src[2 * i] << (8 * i);
> +		*falling |= src[2 * i + 1] << (8 * i);
> +	}
> +}

...

> +static int omnia_read_status_word_old_fw(struct omnia_mcu *mcu, u16 *status)
> +{
> +	int err;
> +
> +	err = omnia_cmd_read_u16(mcu->client, OMNIA_CMD_GET_STATUS_WORD,
> +				 status);
> +	if (!err)

Why not traditional pattern?

	if (err)
		return err;

> +		/*
> +		 * Old firmware has a bug wherein it never resets the USB port
> +		 * overcurrent bits back to zero. Ignore them.
> +		 */
> +		*status &= ~(OMNIA_STS_USB30_OVC | OMNIA_STS_USB31_OVC);
> +
> +	return err;

	return 0;

> +}

...

> +static bool omnia_irq_read_pending(struct omnia_mcu *mcu,
> +				   unsigned long *pending)
> +{
> +	if (mcu->features & OMNIA_FEAT_NEW_INT_API)
> +		return omnia_irq_read_pending_new(mcu, pending);
> +	else

'else' is redundant, but it can be still used for indentation purposes here.

> +		return omnia_irq_read_pending_old(mcu, pending);
> +}

...

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

Haven't seen the rest, but here perhaps ATTRIBUTE_GROUPS().

...

> +static inline int omnia_cmd_read_bits(const struct i2c_client *client, u8 cmd,
> +				      u32 bits, u32 *dst)
> +{
> +	__le32 reply;
> +	int err;
> +
> +	if (!bits) {
> +		*dst = 0;
> +		return 0;
> +	}
> +
> +	err = omnia_cmd_read(client, cmd, &reply,
> +			     omnia_compute_reply_length(bits, false, 0));

> +	if (!err)

	if (err)
		return err;

> +		*dst = le32_to_cpu(reply) & bits;
> +
> +	return err;

	return 0;

> +}

-- 
With Best Regards,
Andy Shevchenko