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