Re: [PATCH v2] i2c/muxes/i2c-mux-ltc4306: LTC4306 and LTC4305 I2C multiplexer/switch

[Peter Rosin]

*snip* *snip*

>>> +static int ltc4306_gpio_get(struct gpio_chip *chip, unsigned int offset)
>>> +{
>>> +	struct ltc4306 *data = gpiochip_get_data(chip);
>>> +	int ret = 0;
>>> +
>>> +	if (gpiochip_line_is_open_drain(chip, offset) ||
>>> +	    (data->regs[LTC_REG_MODE] & BIT(7 - offset))) {
>>
>> I wonder about this open-coded register cache. So, gpio people, is there
>> a guarantee from gpiolib that only one gpio_chip operation is in flight
>> concurrently? Because I don't see any evidence of that. With that in
>> mind, I think some locking is needed?
> 
> I thought there is a per chip mutex in the gpiolib. But I can't find 
> anything like this either. Since these two gpios can be used from 
> different internal or external users. The locking seem to be needed.
> 
> This gets us back to the regmap option. I did a quick grep, and 9 out of 
> 205 drivers using regmap i2c, also use i2c_smbus... concurrently.
> 
> grep -Rl regmap_init_i2c ./drivers  | xargs grep -l i2c_smbus_ | grep "\.c"
> 
> Mostly to work around non uniform transfer layouts.

I see three options.

1. Go with regmap and convert to mux-locked. Then the unlocked i2c-xfer
becomes an ordinary i2c-xfer (or smbus, whatever). This will result in
the cleanest code.

2. Go with regmap and stay parent-locked. Then hook into the regmap
locking as is done in one of the drivers that have worked around similar
problems with regmap and parent-locked i2c-mux interactions:

drivers/media/dvb-frontends/rtl2830.c
drivers/media/dvb-frontends/m88ds3103.c

This will probably work, but you'd need to add a number of extra helper
functions.

3. Exclude register 3 from regmap and only use regmap for the other
registers. This will be a bit ugly and ad-hoc, will need clear comments
on what is going on and why it is safe etc. And I want to see it before
I accept it. And it might not be my call to begin with, because TBH, it
sounds a bit disgusting...

> I'll check with Mark Brown on this topic.

Ok, might be a good idea...

>>> +
>>> +add_adapter_failed:
>>> +	i2c_mux_del_adapters(muxc);
>>> +gpio_default:
>>> +	gpiod_direction_input(data->en_gpio);
>>
>> This was actually not what I had in mind when I asked about it in v1, and
>> this looks a bit strange. You have no way of knowing if the pin was
>> configured as input when probe was called, and I don't see code like this
>> all over the place. Maybe it's is ok to not disable the chip over
>> suspend/resume, I was just asking because it looked a bit strange to grab
>> a pin and then forget about it. Now that I think about it some more, it's
>> probably ok to do just that since it is perhaps not possible to make the
>> chip draw less power by deasserting enable, but what do I know?
> 
> GPIOs are assumed by default inputs. So if you want to undo the actions 
> in probe. The logical consequence is to move them back to inputs, and 
> let the external PULL-UP or PULL-DOWN on the ENABLE decide what happens. 
> I would also prefer to leave it enabled, so that the GPIOs can retain

My point is that I do not see any probe functions undoing gpio configs.
Why bother in this case? Or are other probe functions really doing this?
I actually didn't check, but I haven't stumbled over it previously and
at least think I would have noticed...

> it's last state. Well I think the device draws a bit less power when 
> disabled. But we don't support runtime PM anyways.

It might not be safe to reset the gpio pins over a suspend/resume depending
on what they are used for, so it is probably a bad idea to go there. Sorry
for bringing the whole issue up and muddying the waters...

Cheers,
peda