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

["Marek Behún" <kabel@kernel.org>]

On Wed, 8 May 2024 14:16:26 +0300
Andy Shevchenko <andy@kernel.org> wrote:

> On Wed, May 08, 2024 at 12:31:12PM +0200, Marek Behún wrote:

...

> > +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.

OK

> > +	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().
 
Changing this propagated to many other variables and even required
some refactoring of the omnia_gpio structure, since the bit, ctl_bit
and int_bit members are stored as a masks, but __assign_bit() /
__set_bit() / __clear_bit() requires bit numbers.

For example
  if (gpio->int_bit && (mcu->is_cached & gpio->int_bit))
    return !!(mcu->cached & gpio->int_bit);
needed to change to
  if (gpio->has_int && (mcu->is_cached & BIT(gpio->int_bit)))
    return !!(mcu->cached & BIT(gpio->int_bit));
and so on.

Moreover, I agree that the if-else statement which you commented on,
when changed to __assign_bit(), looks much nicer, but some changes that
sprouted from this are in my opinion less readable.

I have prepared the fixup patch, but I am not confident enough that
everything is done correctly. I would prefer leaving this one for
later, if it is okay with you.

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

OK

...

> > +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;

OK, also for the rest similar.

...

> > +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.

As you say, for indentation purposes I would prefer keeping it this way.

> 
> > +		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().

Those define the variable as static, but I need to access it from
turris-omnia-mcu-base.c compilation unit.