Re: [RFC PATCH] mux: core: add exclusive mux controls support

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

On 03/04/2025 21:58, Peter Rosin wrote:
>> @@ -479,6 +487,10 @@ int mux_control_deselect(struct mux_control *mux)
>>   {
>>   	int ret = 0;
>>   
>> +	/* exclusive mux control do not deselection */
>> +	if (mux->exclusive)
>> +		return -EINVAL;
> This is unfortunate. I think there is value in being able to deselect
> muxes in exclusive mode. Otherwise you might need to keep track of
> some idle-state in the code, when it would be more flexible to have
> it specified in e.g. the DT node. The best idle-state may very well
> be hardware dependent, and is often not some central thing for the
> consumer driver.

Does it mean exclusive mux can deselect a mux however its not mandatory 
to deslect between each mux selects?


> 
>> +
>>   	if (mux->idle_state != MUX_IDLE_AS_IS &&
>>   	    mux->idle_state != mux->cached_state)
>>   		ret = mux_control_set(mux, mux->idle_state);
>> @@ -523,13 +535,15 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
>>    * @mux_name: The name identifying the mux-control.
>>    * @state: Pointer to where the requested state is returned, or NULL when
>>    *         the required multiplexer states are handled by other means.
>> + * @get_type: Type of mux get, shared or exclusive
>>    *
>>    * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
>>    */
>>   static struct mux_control *mux_get(struct device *dev, const char *mux_name,
>> -				   unsigned int *state)
>> +				   unsigned int *state, enum mux_control_get_type get_type)
>>   {
>>   	struct device_node *np = dev->of_node;
>> +	struct mux_control *mux_ctrl;
>>   	struct of_phandle_args args;
>>   	struct mux_chip *mux_chip;
>>   	unsigned int controller;
>> @@ -606,7 +620,25 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
>>   		return ERR_PTR(-EINVAL);
>>   	}
>>   
>> -	return &mux_chip->mux[controller];
>> +	mux_ctrl = &mux_chip->mux[controller];
>> +
>> +	if (mux_ctrl->exclusive) {
>> +		mux_ctrl = ERR_PTR(-EPERM);
>> +		put_device(&mux_chip->dev);
>> +		return mux_ctrl;
>> +	}
>> +
>> +	if (get_type == EXCLUSIVE_GET && mux_ctrl->open_count) {
>> +		mux_ctrl = ERR_PTR(-EBUSY);
>> +		put_device(&mux_chip->dev);
>> +		return mux_ctrl;
>> +	}
>> +
>> +	mux_ctrl->open_count++;
>> +	if (get_type == EXCLUSIVE_GET)
>> +		mux_ctrl->exclusive = true;
>> +
>> +	return mux_ctrl;
> This is racy with no guarantee that you are the only consumer after you

Yes, there is a chance of race here. locking around mux_chip access 
should help fix this race.


> have gotten an exclusive mux. My sketchy vision was to have an API
> function that requests an ordinary shared mux to be exclusive, and then
> another to make the mux shared again. Those would take/release the same

hm,  dynamically going between shared to exclusive is going bring more 
challenges and consumer side its going to be more complicated.


My idea to do this way was to allow more flags like optional to mux, in 
same likes  regulators or resets.. etc.

> lock as when selecting/deselecting, but also mark the mux as exclusive
> and trigger_not_  taking/releasing the lock in select/deselect.
> 
> But then we have the little thing that conditional locking is not
> exactly ideal. Which is why I haven't done this before. I simply never
> got it to a point where I felt it was good enough...
> 
> Another reason for me not having done it is that I also feel that it
> might not be ideal to reuse mux_control_select and mux_control_deselect
> at all since the rules for using those when the mux is shared are ...
> a bit difficult, and will remain that way. Thus, having those functions
> *sometimes*  behave like they are easy and sometimes requiring great
> detail will make the already bad shared case even more error prone.
> 
> I wish I could see how to do this sanely.

How about going with current approach with locking as suggested?

thanks,
Srini
> 
> Cheers,
> Peter
> 
>>   }