Re: [PATCH RFC v2 3/4] mux: Add support for reading mux enable state from DT

[Aswath Govindraju <a-govindraju@ti.com>]

Hi Peter,

On 23/11/21 12:29 am, Peter Rosin wrote:
> 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@ti.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.
> 

Got it. Will not touch idle_state.

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

Sorry, forgot the distinction that multiple consumers can get the mux
and only one can select the mux.

> 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

I am sorry but I did not understand why mux_control_get is not
appropriate. We should be able to call the function and get the mux right?

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

The problem that I see with the optional apis as wrappers around
mux_control_get is the following print in mux_control_get,

dev_err(dev, "%pOF: failed to get mux-control %s(%i)\n",

This gets printed whenever it can't find mux-controls device tree property.


Thank you providing your comments and reference implementation.

Regards,
Aswath

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