linux-kernel - Re: [PATCH 3/5] i2c-mux-gpio: Save initial channel number to the idle data field

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190425155325.voqlrmbdblo7xhtx@mobilestation>
Date:   Thu, 25 Apr 2019 18:53:26 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Peter Rosin <peda@...ntia.se>
Cc:     Peter Korsgaard <peter.korsgaard@...co.com>,
        Serge Semin <Sergey.Semin@...latforms.ru>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/5] i2c-mux-gpio: Save initial channel number to the
 idle data field

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.

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.

-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@...il.com>
> > ---
> >  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
> > +	 * 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",
> > 
>