Message-ID: <69f73f64-6424-4e3f-9068-195e959b9762@axentia.se>
Date:   Mon, 22 Nov 2021 19:59:48 +0100
From:   Peter Rosin <peda@...ntia.se>
To:     Aswath Govindraju <a-govindraju@...com>
Cc:     Nishanth Menon <nm@...com>, Vignesh Raghavendra <vigneshr@...com>,
        Rob Herring <robh+dt@...nel.org>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        Kishon Vijay Abraham I <kishon@...com>,
        Vinod Koul <vkoul@...nel.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-can@...r.kernel.org,
        linux-phy@...ts.infradead.org
Subject: Re: [PATCH RFC v2 3/4] mux: Add support for reading mux enable state
 from DT

Hi!

On 2021-11-22 13:56, Aswath Govindraju wrote:
> In some cases, we might need to provide the state of the mux to be set for
> the operation of a given peripheral. Therefore, pass this information using
> the second argument of the mux-controls property.
> 
> Signed-off-by: Aswath Govindraju <a-govindraju@...com>
> ---
> 
> Notes:
> - The function mux_control_get() always return the mux_control for a single
>   line. So, control for mutiple lines cannot be represented in the
>   mux-controls property.
> - For representing multiple lines of control, multiple entries need to be
>   used along with mux-names for reading them.
> - If a device uses both the states of the mux line then #mux-control-cells
>   can be set to 1 and enable_state will not be set in this case.
> 
>  drivers/mux/core.c           | 20 ++++++++++++++++++--
>  include/linux/mux/consumer.h |  1 +
>  include/linux/mux/driver.h   |  1 +
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
> index 22f4709768d1..51140748d2d6 100644
> --- a/drivers/mux/core.c
> +++ b/drivers/mux/core.c
> @@ -294,6 +294,18 @@ unsigned int mux_control_states(struct mux_control *mux)
>  }
>  EXPORT_SYMBOL_GPL(mux_control_states);
>  
> +/**
> + * mux_control_enable_state() - Query for the enable state.
> + * @mux: The mux-control to query.
> + *
> + * Return: State to be set in the mux to enable a given device
> + */
> +unsigned int mux_control_enable_state(struct mux_control *mux)
> +{
> +	return mux->enable_state;
> +}
> +EXPORT_SYMBOL_GPL(mux_control_enable_state);
> +
>  /*
>   * The mux->lock must be down when calling this function.
>   */
> @@ -481,8 +493,7 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>  	if (!mux_chip)
>  		return ERR_PTR(-EPROBE_DEFER);
>  
> -	if (args.args_count > 1 ||
> -	    (!args.args_count && (mux_chip->controllers > 1))) {
> +	if (!args.args_count && mux_chip->controllers > 1) {
>  		dev_err(dev, "%pOF: wrong #mux-control-cells for %pOF\n",
>  			np, args.np);
>  		put_device(&mux_chip->dev);
> @@ -500,6 +511,11 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	if (args.args_count == 2) {
> +		mux_chip->mux[controller].enable_state = args.args[1];
> +		mux_chip->mux[controller].idle_state = !args.args[1];

Please leave the idle state alone. The idle state is a property of
the mux-control itself, and not the business of any particular
consumer. Consumers can only say what state the mux control should
have when they have selected the mux-control, and have no say about
what state the mux-control has when they do not have it selected.
There is no conflict with having the same idle state as the state the
mux would normally have. That could even be seen as an optimization,
since then there might be no need to operate the mux for most
accesses.

> +	}
> +
>  	return &mux_chip->mux[controller];
>  }
>  EXPORT_SYMBOL_GPL(mux_control_get);
> diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
> index 7a09b040ac39..cb861eab8aad 100644
> --- a/include/linux/mux/consumer.h
> +++ b/include/linux/mux/consumer.h
> @@ -16,6 +16,7 @@ struct device;
>  struct mux_control;
>  
>  unsigned int mux_control_states(struct mux_control *mux);
> +unsigned int mux_control_enable_state(struct mux_control *mux);
>  int __must_check mux_control_select_delay(struct mux_control *mux,
>  					  unsigned int state,
>  					  unsigned int delay_us);
> diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
> index 18824064f8c0..7db378dabdb2 100644
> --- a/include/linux/mux/driver.h
> +++ b/include/linux/mux/driver.h
> @@ -48,6 +48,7 @@ struct mux_control {
>  	int cached_state;
>  
>  	unsigned int states;
> +	unsigned int enable_state;

This is the wrong place to store the state you need. The mux_control
is a shared resource and can have many consumers. Storing the needed
value for exactly one consumer in the mux control is therefore
broken. It would get overwritten when consumer #2 (and 3 etc etc)
wants to use some other state from the same shared mux control.

Doing this properly means that you need a new struct tying together
a mux-control and a state. With an API looking something like this:

struct mux_state {
	struct mux_control *mux;
	unsigned int state;
};

struct mux_state *mux_state_get(struct device *dev, const char *mux_name)
{
	struct mux_state *mux_state = kzalloc(sizeof(*mux_state), GFP_KERNEL);

	if (!mux_state)
		return ERR_PTR(-ENOMEM);

	mux_state->mux = ...; /* mux_control_get(...) perhaps? */
	/* error checking and recovery, etc etc etc */
	mux_state->state = ...;

	return mux_state;
}

void mux_state_put(struct mux_state *mux_state)
{
	mux_control_put(mux_state->mux);
	free(mux_state);
}

int mux_state_select_delay(struct mux_state *mux_state,
 			   unsigned int delay_us)
{
	return mux_control_select_delay(mux_state->mux, mux_state->state,
					delay_us);
}

int mux_state_select(struct mux_state *mux_state)
{
	return mux_state_select_delay(mux_state, 0);
}

int mux_state_try_select_delay(struct mux_state *mux_state)
			       unsigned int delay_us);
{
	return mux_control_try_select_delay(mux_state->mux, mux_state->state,
					    delay_us);
}

int mux_state_try_select(struct mux_state *mux_state)
{
	return mux_state_try_select_delay(mux_state, 0);
}

int mux_state_deselect(struct mux_control *mux)
{
	return mux_control_deselect(mux_state->mux);
}

(written directly in the mail client, never compiled, here be dragons)

mux_state_get is obviously the difficult function to write, and
the above call to mux_control_get is not appropriate as-is. I
think mux_control_get perhaps needs to be refactored into a
flexible helper that takes a couple of extra arguments that
indicate if you want an optional get and/or a particular state.
And then mux_control_get can just be a wrapper around that helper.
Adding mux_control_get_optional would be a matter of adding a new
wrapper. And the mux_state->mux assignment above would need yet
another wrapper for the flexible helper, one that also make the
flexible helper return the requested state from the mux-control
property.

I realize that this might be a big piece to chew, but you want to
do something new here, and I think it is best to do it right from
the start instead of having weird code that only makes it harder
to do it right later. Ad it's not that complicated.

Cheers,
Peter

>  	int idle_state;
>  
>  	ktime_t last_change;
> 

Powered by blists - more mailing lists