On 2019-04-25 17:53, Serge Semin wrote:
> On Wed, Apr 24, 2019 at 09:26:22PM +0000, Peter Rosin wrote:
>> On 2019-04-24 14:34, Serge Semin wrote:
>>> In case if the idle state has been specified in the data structure,
>>> the idle variable is left untouched as before, so to keep a default
>>> channel number enabled in the mux idle state. But if a platform doesn't
>>> specify which channel is going to be enabled by default, we as before
>>> don't setup the deselect callback, but the initial state is saved in the
>>> idle variable for further initialization. We can safely do this here
>>> since that variable is used for initial state setting only, when no
>>> idling lane is specified.
>>
>> While this subtlety is *maybe* ok, a comment about it belongs in the
>> *code* where it will be seen when the next person makes changes.
>>
>> But why not extend the struct with the initial state? How many of
>> these muxes do you expect to exist in a system? Multiplied by a
>> couple of bytes. Who cares?
>>
>
> I actually thought about this when started working on the patchset.
> That time saving the initial value in the idle variable seemed like a
> good idea. I even put a small comment in the code about this.
Oh, I missed that. Crap, and sorry!
> Anyway lets add a new field to "struct gpiomux" structure and use
> it as a container for initial value. It is also a good alternative.
> I'll do this in a v2 patchset.
Good. Thanks!
Cheers,
Peter
> -Sergey
>
>> Cheers,
>> Peter
>>
>>>
>>> The reason of this change is to prepare the code for future GPIOs request
>>> path being split up into of- and plat- based methods. The idle variable
>>> here is used as a container of the initial state for both of the paths in
>>> case of idle-channel isn't specified.
>>>
>>> Signed-off-by: Serge Semin <fancer.lancer@xxxxxxxxx>
>>> ---
>>> drivers/i2c/muxes/i2c-mux-gpio.c | 15 ++++++++-------
>>> 1 file changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
>>> index a14fe132b0c3..535c83c43371 100644
>>> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
>>> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
>>> @@ -171,7 +171,6 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>>> struct gpiomux *mux;
>>> struct i2c_adapter *parent;
>>> struct i2c_adapter *root;
>>> - unsigned initial_state;
>>> int i, ret;
>>>
>>> mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
>>> @@ -204,12 +203,14 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>>>
>>> muxc->mux_locked = true;
>>>
>>> - if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE) {
>>> - initial_state = mux->data.idle;
>>> + /*
>>> + * Set descelect callback if idle state has been setup otherwise just
Spelling "deselect"
>>> + * use the idle variable to store the initial muxer value.
>>> + */
>>> + if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE)
>>> muxc->deselect = i2c_mux_gpio_deselect;
>>> - } else {
>>> - initial_state = mux->data.values[0];
>>> - }
>>> + else
>>> + mux->data.idle = mux->data.values[0];
>>>
>>> for (i = 0; i < mux->data.n_gpios; i++) {
>>> struct device *gpio_dev;
>>> @@ -224,7 +225,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>>> }
>>>
>>> ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
>>> - initial_state & (1 << i));
>>> + mux->data.idle & (1 << i));
>>> if (ret) {
>>> dev_err(&pdev->dev,
>>> "Failed to set direction of GPIO %d to output\n",
>>>
>>
