Hi Peter,
On 30/11/21 11:14 am, Aswath Govindraju wrote:
> Hi Peter,
>
> On 25/11/21 7:22 pm, Peter Rosin wrote:
>> Hi!
>>
>> On 2021-11-23 09:12, 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>
>>> ---
>>> drivers/mux/core.c | 146 ++++++++++++++++++++++++++++++++++-
>>> include/linux/mux/consumer.h | 19 ++++-
>>> include/linux/mux/driver.h | 13 ++++
>>> 3 files changed, 173 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>>> index 22f4709768d1..9622b98f9818 100644
>>> --- a/drivers/mux/core.c
>>> +++ b/drivers/mux/core.c
>>> @@ -370,6 +370,29 @@ int mux_control_select_delay(struct mux_control *mux, unsigned int state,
>>> }
>>> EXPORT_SYMBOL_GPL(mux_control_select_delay);
>>>
>
> [...]
>
>>> }
>>>
>>> /**
>>> - * mux_control_get() - Get the mux-control for a device.
>>> + * mux_get() - Get the mux-control for a device.
>>> * @dev: The device that needs a mux-control.
>>> * @mux_name: The name identifying the mux-control.
>>> + * @enable_state: The variable to store the enable state for the requested device
>>> *
>>> * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
>>> */
>>> -struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>> +static struct mux_control *mux_get(struct device *dev, const char *mux_name,
>>> + unsigned int *enable_state)
>>
>> s/enable_state/state/ (goes for all of the patch).
>>
>>> {
>>> struct device_node *np = dev->of_node;
>>> struct of_phandle_args args;
>>> @@ -481,8 +545,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 ||
>>
>> It is inconsistent to allow more than 2 args, but then only allow
>> digging out the state from the 2nd arg if the count is exactly 2.
>>
>>> - (!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,8 +563,25 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>> return ERR_PTR(-EINVAL);
>>> }
>>>
>>> + if (args.args_count == 2)
>>> + *enable_state = args.args[1];
>>> +
>>
>> With the suggested binding in my comment for patch 1/4, you'd need to do
>> either
>>
>> ret = of_parse_phandle_with_args(np,
>> "mux-controls", "#mux-control-cells",
>> index, &args);
>>
>> or
>>
>> ret = of_parse_phandle_with_args(np,
>> "mux-states", "#mux-state-cells",
>> index, &args);
>>
>> depending on if the mux_get helper gets a NULL (enable_)state pointer or a "real"
>> address, and then...
>>
>
>
> Sorry, while trying to implement the above method, I came across one
> more question. So, in a given consumer DT node we will be either having
> only mux-states or mux-controls right? How would we take care of the
> condition when we would want to set the state of a given line in the
> controller. Especially when a single mux chip is used by multiple
> consumers each using a different line. Wouldn't we require both
> mux-controls and mux-states in that case? So, shouldn't the
> implementation be such that we need to first read mux-controls and then
> based whether the enable_state is NULL, we read mux-states?
>
> Just to add more clarity, if we go about this method then we would have
> both mux-controls and mux-states in the consumer DT node when we want to
> specify the state.
>
I now understood the implementation that you described. mux-states will
be similar to the mux-controls after this patch is applied. mux-states
can have 2 arguments(mux line and state) whereas mux-controls can have
only one argument(mux line).
Sorry, for the inconvenience.
Thanks,
Aswath
> Thanks,
> Aswath
>
>>> return &mux_chip->mux[controller];
>>> }
>>> +
>>> +/**
>>> + * mux_control_get() - Get the mux-control for a device.
>>> + * @dev: The device that needs a mux-control.
>>> + * @mux_name: The name identifying the mux-control.
>>> + *
>>> + * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
>>> + */
>>> +struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>> +{
>
> [...]
>
