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.
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)
>> +{
[...]
