Re: [PATCH v2 1/3] mux: Add mux_control_get_optional() API

[Stephen Boyd]

Quoting Peter Rosin (2017-07-19 00:15:38)
> On 2017-07-19 04:08, Stephen Boyd wrote:
> > Quoting Peter Rosin (2017-07-17 01:20:14)
> >> On 2017-07-14 23:40, Stephen Boyd wrote:
> >>> @@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device 
> >>> *dev, const char *mux_name)
> >>>   if (mux_name) {
> >>>   index = of_property_match_string(np, "mux-control-names",
> >>>mux_name);
> >>> + if (index == -EINVAL && optional)
> >>> + return NULL;
> >>
> >> What about -ENODATA?
> > 
> > At this point in the code we're finding the index of a mux-control-names
> > property so I was trying to handle the case where there isn't a
> > mux-control-names property at all
> 
> Yes, you indeed need to check for -EINVAL to catch that. No argument
> about that.

Ok.

> 
> >   but we're looking for something
> > optional anyway. If there is a property, then we would see some other
> > error if something went wrong and then pass the error up. There is a
> > hole where there isn't a mux-control-names property and we're looking
> > for something that's optional, but there is a mux-control property. Do
> > we care though? The DT seems broken then.
> 
> I was thinking about the case where mux-control-names names *other* muxes
> but not the one asked for in this call. That's not broken and should be
> handled. The way I read it, you get -ENODATA in that case?

Yes that would return -ENODATA. Similarly, it would be returned if we
had a boolean mux-control-names property (which is completely broken).

> 
> >> And if an optional mux is found here, but lookup
> >> fails later in e.g. the of_parse_phandle_with_args call, then I think
> >> an error should be returned. Because that seems like an indication that
> >> DT specifies that there *should* be a mux, but then there isn't one.
> > 
> > of_parse_phandle_with_args() would return ENOENT when there isn't a
> > mux-control property in DT. So I've trapped that case and returned an
> > "optional mux" pointer of NULL. I think we handle the case you mention,
> > where some index is found but it returns an error, because that would
> > return some error besides -ENOENT.
> > 
> > Sorry, I'm not really following what you're suggesting. Maybe it got
> > mixed up with the NULL vs. non-NULL return value from mux_control_get().
> 
> What I mean is that if you have passed a mux_name and the index of that
> name was indeed found in the of_property_match_string call, then any
> failure from of_parse_phandle_with_args indicates a bad DT and should
> IMO result in an error. I.e., when evaluating the result from
> of_parse_phandle_with_args, you should account for the optional param
> if and only if mux_name is NULL.
> 
> You can do that by e.g. setting optional to false after looking up the
> mux_name index (because at that point the mux is no longer considered
> optional). E.g. as the last statement in the if (!mux_name) block.
> 

Ok got it. I'll rework the logic.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] mux: Add mux_control_get_optional() API

[Peter Rosin]

On 2017-07-19 04:08, Stephen Boyd wrote:
> Quoting Peter Rosin (2017-07-17 01:20:14)
>> On 2017-07-14 23:40, Stephen Boyd wrote:
>>> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
>>> index 90b8995f07cb..a0e5bf16f02f 100644
>>> --- a/drivers/mux/mux-core.c
>>> +++ b/drivers/mux/mux-core.c
>>> @@ -289,6 +289,9 @@ EXPORT_SYMBOL_GPL(devm_mux_chip_register);
>>>   */
>>>  unsigned int mux_control_states(struct mux_control *mux)
>>>  {
>>> + if (!mux)
>>> + return 0;
>>> +
>>
>> I don't think this is appropriate. For this function, it might be ok,
>> but...
>>
>>>   return mux->states;
>>>  }
>>>  EXPORT_SYMBOL_GPL(mux_control_states);
>>> @@ -338,6 +341,9 @@ int mux_control_select(struct mux_control *mux, 
>>> unsigned int state)
>>>  {
>>>   int ret;
>>>  
>>> + if (!mux)
>>> + return 0;
>>> +
>>
>> ...here and for other cases below it's very odd to return "ok", when
>> -EINVAL or something seems much more appropriate. And if -EINVAL is
>> returned here, the benefit of returning fake values for anything
>> pretty much falls apart.
>>
>> I simply don't like it, and prefer if the consumer code is arranged
>> to not call the mux functions when the optional get() does not find
>> the mux.
> 
> Do you want the callers of the mux APIs to know that an optional mux
> isn't there, and then have checks at all callsites on optional muxes to
> make sure consumers don't call the mux functions? Won't that duplicate
> lots of checks in drivers for something the core could treat as a don't
> care case? Sorry, I don't understand why the consumer cares that it was
> there or not when it is optional.

Ok, I had a look around to figure out how others handle this, and e.g.
gpio has (ugly) macros (VALIDATE_DESC) to handle this. I guess you are
right and I'm wrong. So, please keep all the if (!mux) checks.

Thanks for insisting!

>>
>>>   ret = down_killable(>lock);
>>>   if (ret < 0)
>>>   return ret;
>>> @@ -370,6 +376,9 @@ int mux_control_try_select(struct mux_control *mux, 
>>> unsigned int state)
>>>  {
>>>   int ret;
>>>  
>>> + if (!mux)
>>> + return 0;
>>> +
>>>   if (down_trylock(>lock))
>>>   return -EBUSY;
>>>  
>>> @@ -398,6 +407,9 @@ int mux_control_deselect(struct mux_control *mux)
>>>  {
>>>   int ret = 0;
>>>  
>>> + if (!mux)
>>> + return 0;
>>> +
>>>   if (mux->idle_state != MUX_IDLE_AS_IS &&
>>>   mux->idle_state != mux->cached_state)
>>>   ret = mux_control_set(mux, mux->idle_state);
>>> @@ -422,14 +434,8 @@ static struct mux_chip 
>>> *of_find_mux_chip_by_node(struct device_node *np)
>>>   return dev ? to_mux_chip(dev) : NULL;
>>>  }
>>>  
>>> -/**
>>> - * 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)
>>> +struct mux_control *
>>> +__mux_control_get(struct device *dev, const char *mux_name, bool optional)
>>>  {
>>>   struct device_node *np = dev->of_node;
>>>   struct of_phandle_args args;
>>> @@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device *dev, 
>>> const char *mux_name)
>>>   if (mux_name) {
>>>   index = of_property_match_string(np, "mux-control-names",
>>>mux_name);
>>> + if (index == -EINVAL && optional)
>>> + return NULL;
>>
>> What about -ENODATA?
> 
> At this point in the code we're finding the index of a mux-control-names
> property so I was trying to handle the case where there isn't a
> mux-control-names property at all

Yes, you indeed need to check for -EINVAL to catch that. No argument
about that.

>   but we're looking for something
> optional anyway. If there is a property, then we would see some other
> error if something went wrong and then pass the error up. There is a
> hole where there isn't a mux-control-names property and we're looking
> for something that's optional, but there is a mux-control property. Do
> we care though? The DT seems broken then.

I was thinking about the case where mux-control-names names *other* muxes
but not the one asked for in this call. That's not broken and should be
handled. The way I read it, you get -ENODATA in that case?

>> And if an optional mux is found here, but lookup
>> fails later in e.g. the of_parse_phandle_with_args call, then I think
>> an error should be returned. Because that seems like an indication that
>> DT specifies that there *should* be a mux, but then there isn't one.
> 
> of_parse_phandle_with_args() would return ENOENT when there isn't a
> mux-control property in DT. So I've trapped that case and returned an
> 

Re: [PATCH v2 1/3] mux: Add mux_control_get_optional() API

[Stephen Boyd]

Quoting Peter Rosin (2017-07-17 01:20:14)
> On 2017-07-14 23:40, Stephen Boyd wrote:
> > diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
> > index 90b8995f07cb..a0e5bf16f02f 100644
> > --- a/drivers/mux/mux-core.c
> > +++ b/drivers/mux/mux-core.c
> > @@ -289,6 +289,9 @@ EXPORT_SYMBOL_GPL(devm_mux_chip_register);
> >   */
> >  unsigned int mux_control_states(struct mux_control *mux)
> >  {
> > + if (!mux)
> > + return 0;
> > +
> 
> I don't think this is appropriate. For this function, it might be ok,
> but...
> 
> >   return mux->states;
> >  }
> >  EXPORT_SYMBOL_GPL(mux_control_states);
> > @@ -338,6 +341,9 @@ int mux_control_select(struct mux_control *mux, 
> > unsigned int state)
> >  {
> >   int ret;
> >  
> > + if (!mux)
> > + return 0;
> > +
> 
> ...here and for other cases below it's very odd to return "ok", when
> -EINVAL or something seems much more appropriate. And if -EINVAL is
> returned here, the benefit of returning fake values for anything
> pretty much falls apart.
> 
> I simply don't like it, and prefer if the consumer code is arranged
> to not call the mux functions when the optional get() does not find
> the mux.

Do you want the callers of the mux APIs to know that an optional mux
isn't there, and then have checks at all callsites on optional muxes to
make sure consumers don't call the mux functions? Won't that duplicate
lots of checks in drivers for something the core could treat as a don't
care case? Sorry, I don't understand why the consumer cares that it was
there or not when it is optional.

> 
> >   ret = down_killable(>lock);
> >   if (ret < 0)
> >   return ret;
> > @@ -370,6 +376,9 @@ int mux_control_try_select(struct mux_control *mux, 
> > unsigned int state)
> >  {
> >   int ret;
> >  
> > + if (!mux)
> > + return 0;
> > +
> >   if (down_trylock(>lock))
> >   return -EBUSY;
> >  
> > @@ -398,6 +407,9 @@ int mux_control_deselect(struct mux_control *mux)
> >  {
> >   int ret = 0;
> >  
> > + if (!mux)
> > + return 0;
> > +
> >   if (mux->idle_state != MUX_IDLE_AS_IS &&
> >   mux->idle_state != mux->cached_state)
> >   ret = mux_control_set(mux, mux->idle_state);
> > @@ -422,14 +434,8 @@ static struct mux_chip 
> > *of_find_mux_chip_by_node(struct device_node *np)
> >   return dev ? to_mux_chip(dev) : NULL;
> >  }
> >  
> > -/**
> > - * 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)
> > +struct mux_control *
> > +__mux_control_get(struct device *dev, const char *mux_name, bool optional)
> >  {
> >   struct device_node *np = dev->of_node;
> >   struct of_phandle_args args;
> > @@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device *dev, 
> > const char *mux_name)
> >   if (mux_name) {
> >   index = of_property_match_string(np, "mux-control-names",
> >mux_name);
> > + if (index == -EINVAL && optional)
> > + return NULL;
> 
> What about -ENODATA?

At this point in the code we're finding the index of a mux-control-names
property so I was trying to handle the case where there isn't a
mux-control-names property at all but we're looking for something
optional anyway. If there is a property, then we would see some other
error if something went wrong and then pass the error up. There is a
hole where there isn't a mux-control-names property and we're looking
for something that's optional, but there is a mux-control property. Do
we care though? The DT seems broken then.

> And if an optional mux is found here, but lookup
> fails later in e.g. the of_parse_phandle_with_args call, then I think
> an error should be returned. Because that seems like an indication that
> DT specifies that there *should* be a mux, but then there isn't one.

of_parse_phandle_with_args() would return ENOENT when there isn't a
mux-control property in DT. So I've trapped that case and returned an
"optional mux" pointer of NULL. I think we handle the case you mention,
where some index is found but it returns an error, because that would
return some error besides -ENOENT.

Sorry, I'm not really following what you're suggesting. Maybe it got
mixed up with the NULL vs. non-NULL return value from mux_control_get().
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] mux: Add mux_control_get_optional() API

[Peter Rosin]

Generally looks like I imagined, but there are a few nits and some
things that I'd like to do differently. Comments inline. Thanks!

On 2017-07-14 23:40, Stephen Boyd wrote:
> Sometimes drivers only use muxes under certain scenarios. For
> example, the chipidea usb controller may be connected to a usb
> switch on some platforms, and connected directly to a usb port on
> others. The driver won't know one way or the other though, so add
> a mux_control_get_optional() API that allows the driver to
> differentiate errors getting the mux from there not being a mux
> for the driver to use at all.
> 
> Cc: Jonathan Cameron 
> Cc: Philipp Zabel 
> Signed-off-by: Stephen Boyd 
> ---
>  Documentation/driver-model/devres.txt |  1 +
>  drivers/mux/mux-core.c| 98 
> ---
>  include/linux/mux/consumer.h  |  4 ++
>  3 files changed, 83 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/driver-model/devres.txt 
> b/Documentation/driver-model/devres.txt
> index 30e04f7a690d..4fdd3e63ff8b 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -342,6 +342,7 @@ MUX
>devm_mux_chip_alloc()
>devm_mux_chip_register()
>devm_mux_control_get()
> +  devm_mux_control_get_optional()
>  
>  PER-CPU MEM
>devm_alloc_percpu()
> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
> index 90b8995f07cb..a0e5bf16f02f 100644
> --- a/drivers/mux/mux-core.c
> +++ b/drivers/mux/mux-core.c
> @@ -289,6 +289,9 @@ EXPORT_SYMBOL_GPL(devm_mux_chip_register);
>   */
>  unsigned int mux_control_states(struct mux_control *mux)
>  {
> + if (!mux)
> + return 0;
> +

I don't think this is appropriate. For this function, it might be ok,
but...

>   return mux->states;
>  }
>  EXPORT_SYMBOL_GPL(mux_control_states);
> @@ -338,6 +341,9 @@ int mux_control_select(struct mux_control *mux, unsigned 
> int state)
>  {
>   int ret;
>  
> + if (!mux)
> + return 0;
> +

...here and for other cases below it's very odd to return "ok", when
-EINVAL or something seems much more appropriate. And if -EINVAL is
returned here, the benefit of returning fake values for anything
pretty much falls apart.

I simply don't like it, and prefer if the consumer code is arranged
to not call the mux functions when the optional get() does not find
the mux.

>   ret = down_killable(>lock);
>   if (ret < 0)
>   return ret;
> @@ -370,6 +376,9 @@ int mux_control_try_select(struct mux_control *mux, 
> unsigned int state)
>  {
>   int ret;
>  
> + if (!mux)
> + return 0;
> +
>   if (down_trylock(>lock))
>   return -EBUSY;
>  
> @@ -398,6 +407,9 @@ int mux_control_deselect(struct mux_control *mux)
>  {
>   int ret = 0;
>  
> + if (!mux)
> + return 0;
> +
>   if (mux->idle_state != MUX_IDLE_AS_IS &&
>   mux->idle_state != mux->cached_state)
>   ret = mux_control_set(mux, mux->idle_state);
> @@ -422,14 +434,8 @@ static struct mux_chip *of_find_mux_chip_by_node(struct 
> device_node *np)
>   return dev ? to_mux_chip(dev) : NULL;
>  }
>  
> -/**
> - * 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)
> +struct mux_control *
> +__mux_control_get(struct device *dev, const char *mux_name, bool optional)
>  {
>   struct device_node *np = dev->of_node;
>   struct of_phandle_args args;
> @@ -441,6 +447,8 @@ struct mux_control *mux_control_get(struct device *dev, 
> const char *mux_name)
>   if (mux_name) {
>   index = of_property_match_string(np, "mux-control-names",
>mux_name);
> + if (index == -EINVAL && optional)
> + return NULL;

What about -ENODATA? And if an optional mux is found here, but lookup
fails later in e.g. the of_parse_phandle_with_args call, then I think
an error should be returned. Because that seems like an indication that
DT specifies that there *should* be a mux, but then there isn't one.

>   if (index < 0) {
>   dev_err(dev, "mux controller '%s' not found\n",
>   mux_name);
> @@ -451,6 +459,8 @@ struct mux_control *mux_control_get(struct device *dev, 
> const char *mux_name)
>   ret = of_parse_phandle_with_args(np,
>"mux-controls", "#mux-control-cells",
>index, );
> + if (ret == -ENOENT && optional)
> + return NULL;
>   if (ret) {
>   dev_err(dev, "%s: failed to get mux-control