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

[Andy Shevchenko <andy@xxxxxxxxxx>]

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@xxxxxxxxxx> 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