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

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

Hi Andy,

I'm sending reply to some of your comments. For those to which I do not
reply I will simply incorporate your suggestions.

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:
> > - 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?

It can't. These are not registers accessible via I2C at specific
addresses, but commands with different-length arguments. This was
already discussed 4 years ago with Jacek, see
  https://www.spinics.net/lists/linux-leds/msg11587.html
There, Jacek suggested writing regmap API for the leds-turris-omnia
driver (the MCU is also a LED controller and has same command
interface, only at different I2C address).

> > +	if (gpio->ctl_cmd)
> > +		return -EOPNOTSUPP;  
> 
> I believe internally we use ENOTSUPP.
> Ditto for all cases like this.

checkpatch warns: 
  ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP

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

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

> > +	mutex_unlock(&mcu->lock);  
> 
> > +	if (err)
> > +		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?

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

bitmap.h actually for this

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

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

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

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

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


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

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.


Marek