Message-ID: <e442d239-2604-54d8-9d50-97a726747c46@ti.com>
Date:   Tue, 23 Nov 2021 09:59:46 +0530
From:   Aswath Govindraju <a-govindraju@...com>
To:     Peter Rosin <peda@...ntia.se>
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 Peter,

On 23/11/21 9:44 am, Aswath Govindraju wrote:
> 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@...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?
> 

Understood why mux_control_get() is not appropriate. There would be no
way for us to get the information about the state from the DT.

Thanks,
Aswath

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

Powered by blists - more mailing lists