Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-18 12:59, Peter Rosin wrote:
> On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
>> On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
>>> +/**
>>> + * devm_mux_chip_unregister() - Resource-managed version 
>>> mux_chip_unregister().
>>> + * @dev: The device that originally registered the mux-chip.
>>> + * @mux_chip: The mux-chip to unregister.
>>> + *
>>> + * See mux_chip_unregister() for more details.
>>> + *
>>> + * Note that you do not normally need to call this function.
>>
>> Odd, then why is it exported???
> 
> You normally don't call the devm_foo_{free,release,unregister,etc} functions.
> The intention is of course that the resourse cleans up automatically. But 
> there
> are no cases where the manual clean up is not available, at least not that I 
> can
> find?

I had a second look and there are of course plenty of examples of missing clean 
up
versions for devm function. I simply hadn't looked very hard at all.

So, for v15 I intend to remove all of devm_mux_chip_unregister, 
devm_mux_chip_free
and devm_mux_control_put. They are all just sitting there with no callers. And 
the
mux-mmio/video-mux drivers by Philipp Zabel that build on top of this series 
don't
need them either. Besides, easy to resurrect if needed...

I will do v15 with the above, the change from mutex to semaphore for locking the
mux controller state [1] and a few small documentation improvements. That will 
be
rebased onto v4.12-rc1 and sent in 10 days or so, or whenever v4.12-rc1 is out.

Meanwhile, what I currently intend for v15 but based on v4.11 is available from
https://gitlab.com/peda-linux/mux.git in the "mux" branch.

Cheers,
peda

[1] https://lkml.org/lkml/2017/4/25/411

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-18 12:59, Peter Rosin wrote:
> On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
>> On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
>>> +/**
>>> + * devm_mux_chip_unregister() - Resource-managed version 
>>> mux_chip_unregister().
>>> + * @dev: The device that originally registered the mux-chip.
>>> + * @mux_chip: The mux-chip to unregister.
>>> + *
>>> + * See mux_chip_unregister() for more details.
>>> + *
>>> + * Note that you do not normally need to call this function.
>>
>> Odd, then why is it exported???
> 
> You normally don't call the devm_foo_{free,release,unregister,etc} functions.
> The intention is of course that the resourse cleans up automatically. But 
> there
> are no cases where the manual clean up is not available, at least not that I 
> can
> find?

I had a second look and there are of course plenty of examples of missing clean 
up
versions for devm function. I simply hadn't looked very hard at all.

So, for v15 I intend to remove all of devm_mux_chip_unregister, 
devm_mux_chip_free
and devm_mux_control_put. They are all just sitting there with no callers. And 
the
mux-mmio/video-mux drivers by Philipp Zabel that build on top of this series 
don't
need them either. Besides, easy to resurrect if needed...

I will do v15 with the above, the change from mutex to semaphore for locking the
mux controller state [1] and a few small documentation improvements. That will 
be
rebased onto v4.12-rc1 and sent in 10 days or so, or whenever v4.12-rc1 is out.

Meanwhile, what I currently intend for v15 but based on v4.11 is available from
https://gitlab.com/peda-linux/mux.git in the "mux" branch.

Cheers,
peda

[1] https://lkml.org/lkml/2017/4/25/411

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-20 23:53, Peter Rosin wrote:
> On 2017-04-18 23:53, Peter Rosin wrote:
>> On 2017-04-18 13:44, Greg Kroah-Hartman wrote:
>>> On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
 On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
> On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
> 
> *snip*
> 
>> +if (mux->idle_state != MUX_IDLE_AS_IS &&
>> +mux->idle_state != mux->cached_state)
>> +ret = mux_control_set(mux, mux->idle_state);
>> +
>> +up_read(>lock);
>
> You require a lock to be held for a "global" function?  Without
> documentation?  Or even a sparse marking?  That's asking for trouble...

 Documentation I can handle, but where should I look to understand how I
 should add sparse markings?
>>>
>>> Run sparse on the code and see what it says :)
>>
>> Will do.
> 
> I just did, and even went through the trouble of getting the bleeding
> edge sparse from the git repo when sparse 0.5.0 came up empty, but it's
> all silence for me. So, how do I add sparse markings?

I looked some more into this, and the markings I find that seem related
are __acquire() and __release(). But neither mutex_lock() nor up_read()
has markings like that, so adding them when using those kinds of locks
in an imbalanced way seems like a sure way of *getting* sparse messages
about context imbalance...

So, either that, or you are talking about __must_check markings?

I feel like I'm missing something, please advise further.

Cheers,
peda

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-20 23:53, Peter Rosin wrote:
> On 2017-04-18 23:53, Peter Rosin wrote:
>> On 2017-04-18 13:44, Greg Kroah-Hartman wrote:
>>> On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
 On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
> On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
> 
> *snip*
> 
>> +if (mux->idle_state != MUX_IDLE_AS_IS &&
>> +mux->idle_state != mux->cached_state)
>> +ret = mux_control_set(mux, mux->idle_state);
>> +
>> +up_read(>lock);
>
> You require a lock to be held for a "global" function?  Without
> documentation?  Or even a sparse marking?  That's asking for trouble...

 Documentation I can handle, but where should I look to understand how I
 should add sparse markings?
>>>
>>> Run sparse on the code and see what it says :)
>>
>> Will do.
> 
> I just did, and even went through the trouble of getting the bleeding
> edge sparse from the git repo when sparse 0.5.0 came up empty, but it's
> all silence for me. So, how do I add sparse markings?

I looked some more into this, and the markings I find that seem related
are __acquire() and __release(). But neither mutex_lock() nor up_read()
has markings like that, so adding them when using those kinds of locks
in an imbalanced way seems like a sure way of *getting* sparse messages
about context imbalance...

So, either that, or you are talking about __must_check markings?

I feel like I'm missing something, please advise further.

Cheers,
peda

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-21 16:41, Philipp Zabel wrote:
> On Fri, 2017-04-21 at 16:32 +0200, Peter Rosin wrote:
>> On 2017-04-21 16:23, Philipp Zabel wrote:
>>> On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
>>> [...]
 +int mux_chip_register(struct mux_chip *mux_chip)
 +{
 +  int i;
 +  int ret;
 +
 +  for (i = 0; i < mux_chip->controllers; ++i) {
 +  struct mux_control *mux = _chip->mux[i];
 +
 +  if (mux->idle_state == mux->cached_state)
 +  continue;
>>>
>>> I think this should be changed to
>>>  
>>> -   if (mux->idle_state == mux->cached_state)
>>> +   if (mux->idle_state == mux->cached_state ||
>>> +   mux->idle_state == MUX_IDLE_AS_IS)
>>> continue;
>>>
>>> or the following mux_control_set will be called with state ==
>>> MUX_IDLE_AS_IS. Alternatively, mux_control_set should return when passed
>>> this value.
>>
>> That cannot happen because ->cached_state is initialized to -1
>> in mux_chip_alloc, so should always be == MUX_IDLE_AS_IS when
>> registering. And drivers are not supposed to touch ->cached_state.
>> I.e., ->cached_state is "owned" by the core.
> 
> So this was caused by me filling cached_state from register reads in the
> mmio driver. Makes me wonder why I am not allowed to do this, though, if
> I am able to read back the initial state?

You gain fairly little by reading back the original state. If the mux
should idle-as-is, you can avoid a maximum of one mux update if the first
consumer happens to starts by requesting the previously active state.
Similarly, if the mux should idle in a specific state, you can avoid a
maximum of one mux update.

In both cases it costs one unconditional read of the mux state.

Sure, in some cases reads are cheaper than writes, but I didn't think
support for seeding the cache was worth it. Is it worth it?

Cheers,
peda

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-21 16:41, Philipp Zabel wrote:
> On Fri, 2017-04-21 at 16:32 +0200, Peter Rosin wrote:
>> On 2017-04-21 16:23, Philipp Zabel wrote:
>>> On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
>>> [...]
 +int mux_chip_register(struct mux_chip *mux_chip)
 +{
 +  int i;
 +  int ret;
 +
 +  for (i = 0; i < mux_chip->controllers; ++i) {
 +  struct mux_control *mux = _chip->mux[i];
 +
 +  if (mux->idle_state == mux->cached_state)
 +  continue;
>>>
>>> I think this should be changed to
>>>  
>>> -   if (mux->idle_state == mux->cached_state)
>>> +   if (mux->idle_state == mux->cached_state ||
>>> +   mux->idle_state == MUX_IDLE_AS_IS)
>>> continue;
>>>
>>> or the following mux_control_set will be called with state ==
>>> MUX_IDLE_AS_IS. Alternatively, mux_control_set should return when passed
>>> this value.
>>
>> That cannot happen because ->cached_state is initialized to -1
>> in mux_chip_alloc, so should always be == MUX_IDLE_AS_IS when
>> registering. And drivers are not supposed to touch ->cached_state.
>> I.e., ->cached_state is "owned" by the core.
> 
> So this was caused by me filling cached_state from register reads in the
> mmio driver. Makes me wonder why I am not allowed to do this, though, if
> I am able to read back the initial state?

You gain fairly little by reading back the original state. If the mux
should idle-as-is, you can avoid a maximum of one mux update if the first
consumer happens to starts by requesting the previously active state.
Similarly, if the mux should idle in a specific state, you can avoid a
maximum of one mux update.

In both cases it costs one unconditional read of the mux state.

Sure, in some cases reads are cheaper than writes, but I didn't think
support for seeding the cache was worth it. Is it worth it?

Cheers,
peda

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-21 16:18, Philipp Zabel wrote:
> Hi Peter,
> 
> On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> [...]
>> +int mux_control_select(struct mux_control *mux, int state)
> 
> state could be unsigned int for the consumer facing API.
> 
>> +{
>> +int ret;
> 
> And mux_control_select should check that (0 <= state < mux->states).

Yes, that makes sense. I worried that we might end up with
signed/unsigned comparisons since the internal state still needs
to be signed, but that didn't happen when I tried...

I'll include this change in v14.

Cheers,
peda

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-21 16:18, Philipp Zabel wrote:
> Hi Peter,
> 
> On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> [...]
>> +int mux_control_select(struct mux_control *mux, int state)
> 
> state could be unsigned int for the consumer facing API.
> 
>> +{
>> +int ret;
> 
> And mux_control_select should check that (0 <= state < mux->states).

Yes, that makes sense. I worried that we might end up with
signed/unsigned comparisons since the internal state still needs
to be signed, but that didn't happen when I tried...

I'll include this change in v14.

Cheers,
peda

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Philipp Zabel]

On Fri, 2017-04-21 at 16:55 +0200, Peter Rosin wrote:
> On 2017-04-21 16:41, Philipp Zabel wrote:
> > On Fri, 2017-04-21 at 16:32 +0200, Peter Rosin wrote:
> >> On 2017-04-21 16:23, Philipp Zabel wrote:
> >>> On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> >>> [...]
>  +int mux_chip_register(struct mux_chip *mux_chip)
>  +{
>  +int i;
>  +int ret;
>  +
>  +for (i = 0; i < mux_chip->controllers; ++i) {
>  +struct mux_control *mux = _chip->mux[i];
>  +
>  +if (mux->idle_state == mux->cached_state)
>  +continue;
> >>>
> >>> I think this should be changed to
> >>>  
> >>> -   if (mux->idle_state == mux->cached_state)
> >>> +   if (mux->idle_state == mux->cached_state ||
> >>> +   mux->idle_state == MUX_IDLE_AS_IS)
> >>> continue;
> >>>
> >>> or the following mux_control_set will be called with state ==
> >>> MUX_IDLE_AS_IS. Alternatively, mux_control_set should return when passed
> >>> this value.
> >>
> >> That cannot happen because ->cached_state is initialized to -1
> >> in mux_chip_alloc, so should always be == MUX_IDLE_AS_IS when
> >> registering. And drivers are not supposed to touch ->cached_state.
> >> I.e., ->cached_state is "owned" by the core.
> > 
> > So this was caused by me filling cached_state from register reads in the
> > mmio driver. Makes me wonder why I am not allowed to do this, though, if
> > I am able to read back the initial state?
> 
> You gain fairly little by reading back the original state. If the mux
> should idle-as-is, you can avoid a maximum of one mux update if the first
> consumer happens to starts by requesting the previously active state.
> Similarly, if the mux should idle in a specific state, you can avoid a
> maximum of one mux update.
> 
> In both cases it costs one unconditional read of the mux state.
> 
> Sure, in some cases reads are cheaper than writes, but I didn't think
> support for seeding the cache was worth it. Is it worth it?

Probably not, I'll just drop the cached_state initialization. It should
be documented in the mux.h that this field is framework internal and not
to be touched by the drivers. At least I was surprised.

regards
Philipp

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Philipp Zabel]

On Fri, 2017-04-21 at 16:55 +0200, Peter Rosin wrote:
> On 2017-04-21 16:41, Philipp Zabel wrote:
> > On Fri, 2017-04-21 at 16:32 +0200, Peter Rosin wrote:
> >> On 2017-04-21 16:23, Philipp Zabel wrote:
> >>> On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> >>> [...]
>  +int mux_chip_register(struct mux_chip *mux_chip)
>  +{
>  +int i;
>  +int ret;
>  +
>  +for (i = 0; i < mux_chip->controllers; ++i) {
>  +struct mux_control *mux = _chip->mux[i];
>  +
>  +if (mux->idle_state == mux->cached_state)
>  +continue;
> >>>
> >>> I think this should be changed to
> >>>  
> >>> -   if (mux->idle_state == mux->cached_state)
> >>> +   if (mux->idle_state == mux->cached_state ||
> >>> +   mux->idle_state == MUX_IDLE_AS_IS)
> >>> continue;
> >>>
> >>> or the following mux_control_set will be called with state ==
> >>> MUX_IDLE_AS_IS. Alternatively, mux_control_set should return when passed
> >>> this value.
> >>
> >> That cannot happen because ->cached_state is initialized to -1
> >> in mux_chip_alloc, so should always be == MUX_IDLE_AS_IS when
> >> registering. And drivers are not supposed to touch ->cached_state.
> >> I.e., ->cached_state is "owned" by the core.
> > 
> > So this was caused by me filling cached_state from register reads in the
> > mmio driver. Makes me wonder why I am not allowed to do this, though, if
> > I am able to read back the initial state?
> 
> You gain fairly little by reading back the original state. If the mux
> should idle-as-is, you can avoid a maximum of one mux update if the first
> consumer happens to starts by requesting the previously active state.
> Similarly, if the mux should idle in a specific state, you can avoid a
> maximum of one mux update.
> 
> In both cases it costs one unconditional read of the mux state.
> 
> Sure, in some cases reads are cheaper than writes, but I didn't think
> support for seeding the cache was worth it. Is it worth it?

Probably not, I'll just drop the cached_state initialization. It should
be documented in the mux.h that this field is framework internal and not
to be touched by the drivers. At least I was surprised.

regards
Philipp

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Philipp Zabel]

On Fri, 2017-04-21 at 16:32 +0200, Peter Rosin wrote:
> On 2017-04-21 16:23, Philipp Zabel wrote:
> > On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> > [...]
> >> +int mux_chip_register(struct mux_chip *mux_chip)
> >> +{
> >> +  int i;
> >> +  int ret;
> >> +
> >> +  for (i = 0; i < mux_chip->controllers; ++i) {
> >> +  struct mux_control *mux = _chip->mux[i];
> >> +
> >> +  if (mux->idle_state == mux->cached_state)
> >> +  continue;
> > 
> > I think this should be changed to
> >  
> > -   if (mux->idle_state == mux->cached_state)
> > +   if (mux->idle_state == mux->cached_state ||
> > +   mux->idle_state == MUX_IDLE_AS_IS)
> > continue;
> > 
> > or the following mux_control_set will be called with state ==
> > MUX_IDLE_AS_IS. Alternatively, mux_control_set should return when passed
> > this value.
> 
> That cannot happen because ->cached_state is initialized to -1
> in mux_chip_alloc, so should always be == MUX_IDLE_AS_IS when
> registering. And drivers are not supposed to touch ->cached_state.
> I.e., ->cached_state is "owned" by the core.

So this was caused by me filling cached_state from register reads in the
mmio driver. Makes me wonder why I am not allowed to do this, though, if
I am able to read back the initial state?

regards
Philipp

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Philipp Zabel]

On Fri, 2017-04-21 at 16:32 +0200, Peter Rosin wrote:
> On 2017-04-21 16:23, Philipp Zabel wrote:
> > On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> > [...]
> >> +int mux_chip_register(struct mux_chip *mux_chip)
> >> +{
> >> +  int i;
> >> +  int ret;
> >> +
> >> +  for (i = 0; i < mux_chip->controllers; ++i) {
> >> +  struct mux_control *mux = _chip->mux[i];
> >> +
> >> +  if (mux->idle_state == mux->cached_state)
> >> +  continue;
> > 
> > I think this should be changed to
> >  
> > -   if (mux->idle_state == mux->cached_state)
> > +   if (mux->idle_state == mux->cached_state ||
> > +   mux->idle_state == MUX_IDLE_AS_IS)
> > continue;
> > 
> > or the following mux_control_set will be called with state ==
> > MUX_IDLE_AS_IS. Alternatively, mux_control_set should return when passed
> > this value.
> 
> That cannot happen because ->cached_state is initialized to -1
> in mux_chip_alloc, so should always be == MUX_IDLE_AS_IS when
> registering. And drivers are not supposed to touch ->cached_state.
> I.e., ->cached_state is "owned" by the core.

So this was caused by me filling cached_state from register reads in the
mmio driver. Makes me wonder why I am not allowed to do this, though, if
I am able to read back the initial state?

regards
Philipp

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-21 16:23, Philipp Zabel wrote:
> On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> [...]
>> +int mux_chip_register(struct mux_chip *mux_chip)
>> +{
>> +int i;
>> +int ret;
>> +
>> +for (i = 0; i < mux_chip->controllers; ++i) {
>> +struct mux_control *mux = _chip->mux[i];
>> +
>> +if (mux->idle_state == mux->cached_state)
>> +continue;
> 
> I think this should be changed to
>  
> -   if (mux->idle_state == mux->cached_state)
> +   if (mux->idle_state == mux->cached_state ||
> +   mux->idle_state == MUX_IDLE_AS_IS)
> continue;
> 
> or the following mux_control_set will be called with state ==
> MUX_IDLE_AS_IS. Alternatively, mux_control_set should return when passed
> this value.

That cannot happen because ->cached_state is initialized to -1
in mux_chip_alloc, so should always be == MUX_IDLE_AS_IS when
registering. And drivers are not supposed to touch ->cached_state.
I.e., ->cached_state is "owned" by the core.

Cheers,
peda

>> +ret = mux_control_set(mux, mux->idle_state);
>> +if (ret < 0) {
>> +dev_err(_chip->dev, "unable to set idle state\n");
>> +return ret;
>> +}
>> +}
>> +
>> +ret = device_add(_chip->dev);
>> +if (ret < 0)
>> +dev_err(_chip->dev,
>> +"device_add failed in mux_chip_register: %d\n", ret);
>> +return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_chip_register);
> 
> regards
> Philipp
> 

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-21 16:23, Philipp Zabel wrote:
> On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> [...]
>> +int mux_chip_register(struct mux_chip *mux_chip)
>> +{
>> +int i;
>> +int ret;
>> +
>> +for (i = 0; i < mux_chip->controllers; ++i) {
>> +struct mux_control *mux = _chip->mux[i];
>> +
>> +if (mux->idle_state == mux->cached_state)
>> +continue;
> 
> I think this should be changed to
>  
> -   if (mux->idle_state == mux->cached_state)
> +   if (mux->idle_state == mux->cached_state ||
> +   mux->idle_state == MUX_IDLE_AS_IS)
> continue;
> 
> or the following mux_control_set will be called with state ==
> MUX_IDLE_AS_IS. Alternatively, mux_control_set should return when passed
> this value.

That cannot happen because ->cached_state is initialized to -1
in mux_chip_alloc, so should always be == MUX_IDLE_AS_IS when
registering. And drivers are not supposed to touch ->cached_state.
I.e., ->cached_state is "owned" by the core.

Cheers,
peda

>> +ret = mux_control_set(mux, mux->idle_state);
>> +if (ret < 0) {
>> +dev_err(_chip->dev, "unable to set idle state\n");
>> +return ret;
>> +}
>> +}
>> +
>> +ret = device_add(_chip->dev);
>> +if (ret < 0)
>> +dev_err(_chip->dev,
>> +"device_add failed in mux_chip_register: %d\n", ret);
>> +return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_chip_register);
> 
> regards
> Philipp
> 

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Philipp Zabel]

On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
[...]
> +int mux_chip_register(struct mux_chip *mux_chip)
> +{
> + int i;
> + int ret;
> +
> + for (i = 0; i < mux_chip->controllers; ++i) {
> + struct mux_control *mux = _chip->mux[i];
> +
> + if (mux->idle_state == mux->cached_state)
> + continue;

I think this should be changed to
 
-   if (mux->idle_state == mux->cached_state)
+   if (mux->idle_state == mux->cached_state ||
+   mux->idle_state == MUX_IDLE_AS_IS)
continue;

or the following mux_control_set will be called with state ==
MUX_IDLE_AS_IS. Alternatively, mux_control_set should return when passed
this value.

> + ret = mux_control_set(mux, mux->idle_state);
> + if (ret < 0) {
> + dev_err(_chip->dev, "unable to set idle state\n");
> + return ret;
> + }
> + }
> +
> + ret = device_add(_chip->dev);
> + if (ret < 0)
> + dev_err(_chip->dev,
> + "device_add failed in mux_chip_register: %d\n", ret);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_register);

regards
Philipp

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Philipp Zabel]

On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
[...]
> +int mux_chip_register(struct mux_chip *mux_chip)
> +{
> + int i;
> + int ret;
> +
> + for (i = 0; i < mux_chip->controllers; ++i) {
> + struct mux_control *mux = _chip->mux[i];
> +
> + if (mux->idle_state == mux->cached_state)
> + continue;

I think this should be changed to
 
-   if (mux->idle_state == mux->cached_state)
+   if (mux->idle_state == mux->cached_state ||
+   mux->idle_state == MUX_IDLE_AS_IS)
continue;

or the following mux_control_set will be called with state ==
MUX_IDLE_AS_IS. Alternatively, mux_control_set should return when passed
this value.

> + ret = mux_control_set(mux, mux->idle_state);
> + if (ret < 0) {
> + dev_err(_chip->dev, "unable to set idle state\n");
> + return ret;
> + }
> + }
> +
> + ret = device_add(_chip->dev);
> + if (ret < 0)
> + dev_err(_chip->dev,
> + "device_add failed in mux_chip_register: %d\n", ret);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_register);

regards
Philipp

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Philipp Zabel]

Hi Peter,

On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
[...]
> +int mux_control_select(struct mux_control *mux, int state)

state could be unsigned int for the consumer facing API.

> +{
> + int ret;

And mux_control_select should check that (0 <= state < mux->states).

regards
Philipp

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Philipp Zabel]

Hi Peter,

On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
[...]
> +int mux_control_select(struct mux_control *mux, int state)

state could be unsigned int for the consumer facing API.

> +{
> + int ret;

And mux_control_select should check that (0 <= state < mux->states).

regards
Philipp

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-18 23:53, Peter Rosin wrote:
> On 2017-04-18 13:44, Greg Kroah-Hartman wrote:
>> On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
>>> On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
 On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:

*snip*

> + if (mux->idle_state != MUX_IDLE_AS_IS &&
> + mux->idle_state != mux->cached_state)
> + ret = mux_control_set(mux, mux->idle_state);
> +
> + up_read(>lock);

 You require a lock to be held for a "global" function?  Without
 documentation?  Or even a sparse marking?  That's asking for trouble...
>>>
>>> Documentation I can handle, but where should I look to understand how I
>>> should add sparse markings?
>>
>> Run sparse on the code and see what it says :)
> 
> Will do.

I just did, and even went through the trouble of getting the bleeding
edge sparse from the git repo when sparse 0.5.0 came up empty, but it's
all silence for me. So, how do I add sparse markings?

Cheers,
peda

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-18 23:53, Peter Rosin wrote:
> On 2017-04-18 13:44, Greg Kroah-Hartman wrote:
>> On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
>>> On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
 On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:

*snip*

> + if (mux->idle_state != MUX_IDLE_AS_IS &&
> + mux->idle_state != mux->cached_state)
> + ret = mux_control_set(mux, mux->idle_state);
> +
> + up_read(>lock);

 You require a lock to be held for a "global" function?  Without
 documentation?  Or even a sparse marking?  That's asking for trouble...
>>>
>>> Documentation I can handle, but where should I look to understand how I
>>> should add sparse markings?
>>
>> Run sparse on the code and see what it says :)
> 
> Will do.

I just did, and even went through the trouble of getting the bleeding
edge sparse from the git repo when sparse 0.5.0 came up empty, but it's
all silence for me. So, how do I add sparse markings?

Cheers,
peda

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-19 15:49, Philipp Zabel wrote:
> On Wed, 2017-04-19 at 14:00 +0200, Peter Rosin wrote:
> [...]
 +int mux_control_select(struct mux_control *mux, int state)
>>>
>>> If we let two of these race, ...
>>
>> The window for this "race" is positively huge. If there are several
>> mux consumers of a single mux controller, it is self-evident that
>> if one of them grabs the mux for a long time, the others will suffer.
>>
>> The design is that the rwsem is reader-locked for the full duration
>> of a select/deselect operation by the mux consumer.
> 
> I was not clear. I meant: I think this can also happen if we let them
> race with the same state target.

Right, but there is another glaring problem with the v13 implementation
of select/deselect. If there are three consumers and the first one
holds the mux while the other two tries to select it to some other
position, then even if the two "new" consumers agree on the mux state,
then both of them will end up in the "it's just contended" case and
then be serialized. So, yes, the select/deselect implementation is not
perfect. To quote the cover letter:

I'm using an rwsem to lock a mux, but that isn't really a
perfect fit. Is there a better locking primitive that I don't
know about that fits better? I had a mutex at one point, but
that didn't allow any concurrent accesses at all. At least
the rwsem allows concurrent access as long as all users
agree on the mux state, but I suspect that the rwsem will
degrade to the mutex situation pretty quickly if there is
any contention.

But with your discovery that there's a race when two consumers go
for the same state in a free mux, in addition to the above contention
problem, I'm about ready to go with Greg's suggestion and just use a
mutex. I.e. ignore the desire to allow concurrent use. Because it's
not like the sketchy thing I threw out in the other part of the thread
solves any of these problems. I can live without the concurrency, and
I guess I can also live with passing the buck to the poor sod that
eventually needs it.

 +{
 +  int ret;
 +
 +  if (down_read_trylock(>lock)) {
 +  if (mux->cached_state == state)
 +  return 0;
> 
> This check makes it clear that a second select call is not intended to
> block if the intended state is already selected. But if the instance we
> will lose the race against has not yet updated cached_state, ...
> 
 +  /* Sigh, the mux needs updating... */
 +  up_read(>lock);
 +  }
 +
 +  /* ...or it's just contended. */
 +  down_write(>lock);
> 
> ... we are blocking here until the other instance calls up_read. Even
> though in this case (same state target) we would only have to block
> until the other instance calls downgrade_write after the mux control is
> set to the correct state.
> 
> Basically there is a small window before down_write with no lock at all,
> where multiple instances can already have decided they must change the
> mux (to the same state). If this happens, they go on to block each other
> unnecessarily.
> 
>>> ... then the last to get to down_write will just wait here forever (or
>>> until the first consumer calls mux_control_deselect, which may never
>>> happen)?
>>
>> It is vital that the mux consumer call _deselect when it is done with
>> the mux. Not doing so will surely starve out any other mux consumers.
>> The whole thing is designed around the fact that mux consumers should
>> deselect the mux as soon as it's no longer needed.
> 
> I'd like to use this for video bus multiplexers. Those would not be
> selected for the duration of an i2c transfer, but for the duration of a
> running video capture stream, or for the duration of an enabled display
> path. While I currently have no use case for multiple consumers
> controlling the same mux, this scenario is what shapes my perspective.
> For such long running selections the consumer should have the option to
> return -EBUSY instead of blocking when the lock can't be taken.

I'll see if I can implement a try variant for the mutex based select I
will probably end up with in v14...

Cheers,
peda

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-19 15:49, Philipp Zabel wrote:
> On Wed, 2017-04-19 at 14:00 +0200, Peter Rosin wrote:
> [...]
 +int mux_control_select(struct mux_control *mux, int state)
>>>
>>> If we let two of these race, ...
>>
>> The window for this "race" is positively huge. If there are several
>> mux consumers of a single mux controller, it is self-evident that
>> if one of them grabs the mux for a long time, the others will suffer.
>>
>> The design is that the rwsem is reader-locked for the full duration
>> of a select/deselect operation by the mux consumer.
> 
> I was not clear. I meant: I think this can also happen if we let them
> race with the same state target.

Right, but there is another glaring problem with the v13 implementation
of select/deselect. If there are three consumers and the first one
holds the mux while the other two tries to select it to some other
position, then even if the two "new" consumers agree on the mux state,
then both of them will end up in the "it's just contended" case and
then be serialized. So, yes, the select/deselect implementation is not
perfect. To quote the cover letter:

I'm using an rwsem to lock a mux, but that isn't really a
perfect fit. Is there a better locking primitive that I don't
know about that fits better? I had a mutex at one point, but
that didn't allow any concurrent accesses at all. At least
the rwsem allows concurrent access as long as all users
agree on the mux state, but I suspect that the rwsem will
degrade to the mutex situation pretty quickly if there is
any contention.

But with your discovery that there's a race when two consumers go
for the same state in a free mux, in addition to the above contention
problem, I'm about ready to go with Greg's suggestion and just use a
mutex. I.e. ignore the desire to allow concurrent use. Because it's
not like the sketchy thing I threw out in the other part of the thread
solves any of these problems. I can live without the concurrency, and
I guess I can also live with passing the buck to the poor sod that
eventually needs it.

 +{
 +  int ret;
 +
 +  if (down_read_trylock(>lock)) {
 +  if (mux->cached_state == state)
 +  return 0;
> 
> This check makes it clear that a second select call is not intended to
> block if the intended state is already selected. But if the instance we
> will lose the race against has not yet updated cached_state, ...
> 
 +  /* Sigh, the mux needs updating... */
 +  up_read(>lock);
 +  }
 +
 +  /* ...or it's just contended. */
 +  down_write(>lock);
> 
> ... we are blocking here until the other instance calls up_read. Even
> though in this case (same state target) we would only have to block
> until the other instance calls downgrade_write after the mux control is
> set to the correct state.
> 
> Basically there is a small window before down_write with no lock at all,
> where multiple instances can already have decided they must change the
> mux (to the same state). If this happens, they go on to block each other
> unnecessarily.
> 
>>> ... then the last to get to down_write will just wait here forever (or
>>> until the first consumer calls mux_control_deselect, which may never
>>> happen)?
>>
>> It is vital that the mux consumer call _deselect when it is done with
>> the mux. Not doing so will surely starve out any other mux consumers.
>> The whole thing is designed around the fact that mux consumers should
>> deselect the mux as soon as it's no longer needed.
> 
> I'd like to use this for video bus multiplexers. Those would not be
> selected for the duration of an i2c transfer, but for the duration of a
> running video capture stream, or for the duration of an enabled display
> path. While I currently have no use case for multiple consumers
> controlling the same mux, this scenario is what shapes my perspective.
> For such long running selections the consumer should have the option to
> return -EBUSY instead of blocking when the lock can't be taken.

I'll see if I can implement a try variant for the mutex based select I
will probably end up with in v14...

Cheers,
peda

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Philipp Zabel]

On Wed, 2017-04-19 at 14:00 +0200, Peter Rosin wrote:
[...]
> >> +int mux_control_select(struct mux_control *mux, int state)
> > 
> > If we let two of these race, ...
>
> The window for this "race" is positively huge. If there are several
> mux consumers of a single mux controller, it is self-evident that
> if one of them grabs the mux for a long time, the others will suffer.
> 
> The design is that the rwsem is reader-locked for the full duration
> of a select/deselect operation by the mux consumer.

I was not clear. I meant: I think this can also happen if we let them
race with the same state target.

> >> +{
> >> +  int ret;
> >> +
> >> +  if (down_read_trylock(>lock)) {
> >> +  if (mux->cached_state == state)
> >> +  return 0;

This check makes it clear that a second select call is not intended to
block if the intended state is already selected. But if the instance we
will lose the race against has not yet updated cached_state, ...

> >> +  /* Sigh, the mux needs updating... */
> >> +  up_read(>lock);
> >> +  }
> >> +
> >> +  /* ...or it's just contended. */
> >> +  down_write(>lock);

... we are blocking here until the other instance calls up_read. Even
though in this case (same state target) we would only have to block
until the other instance calls downgrade_write after the mux control is
set to the correct state.

Basically there is a small window before down_write with no lock at all,
where multiple instances can already have decided they must change the
mux (to the same state). If this happens, they go on to block each other
unnecessarily.

> > ... then the last to get to down_write will just wait here forever (or
> > until the first consumer calls mux_control_deselect, which may never
> > happen)?
> 
> It is vital that the mux consumer call _deselect when it is done with
> the mux. Not doing so will surely starve out any other mux consumers.
> The whole thing is designed around the fact that mux consumers should
> deselect the mux as soon as it's no longer needed.

I'd like to use this for video bus multiplexers. Those would not be
selected for the duration of an i2c transfer, but for the duration of a
running video capture stream, or for the duration of an enabled display
path. While I currently have no use case for multiple consumers
controlling the same mux, this scenario is what shapes my perspective.
For such long running selections the consumer should have the option to
return -EBUSY instead of blocking when the lock can't be taken.

regards
Philipp

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Philipp Zabel]

On Wed, 2017-04-19 at 14:00 +0200, Peter Rosin wrote:
[...]
> >> +int mux_control_select(struct mux_control *mux, int state)
> > 
> > If we let two of these race, ...
>
> The window for this "race" is positively huge. If there are several
> mux consumers of a single mux controller, it is self-evident that
> if one of them grabs the mux for a long time, the others will suffer.
> 
> The design is that the rwsem is reader-locked for the full duration
> of a select/deselect operation by the mux consumer.

I was not clear. I meant: I think this can also happen if we let them
race with the same state target.

> >> +{
> >> +  int ret;
> >> +
> >> +  if (down_read_trylock(>lock)) {
> >> +  if (mux->cached_state == state)
> >> +  return 0;

This check makes it clear that a second select call is not intended to
block if the intended state is already selected. But if the instance we
will lose the race against has not yet updated cached_state, ...

> >> +  /* Sigh, the mux needs updating... */
> >> +  up_read(>lock);
> >> +  }
> >> +
> >> +  /* ...or it's just contended. */
> >> +  down_write(>lock);

... we are blocking here until the other instance calls up_read. Even
though in this case (same state target) we would only have to block
until the other instance calls downgrade_write after the mux control is
set to the correct state.

Basically there is a small window before down_write with no lock at all,
where multiple instances can already have decided they must change the
mux (to the same state). If this happens, they go on to block each other
unnecessarily.

> > ... then the last to get to down_write will just wait here forever (or
> > until the first consumer calls mux_control_deselect, which may never
> > happen)?
> 
> It is vital that the mux consumer call _deselect when it is done with
> the mux. Not doing so will surely starve out any other mux consumers.
> The whole thing is designed around the fact that mux consumers should
> deselect the mux as soon as it's no longer needed.

I'd like to use this for video bus multiplexers. Those would not be
selected for the duration of an i2c transfer, but for the duration of a
running video capture stream, or for the duration of an enabled display
path. While I currently have no use case for multiple consumers
controlling the same mux, this scenario is what shapes my perspective.
For such long running selections the consumer should have the option to
return -EBUSY instead of blocking when the lock can't be taken.

regards
Philipp

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-19 11:06, Philipp Zabel wrote:
> On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
>> Add a new minimalistic subsystem that handles multiplexer controllers.
>> When multiplexers are used in various places in the kernel, and the
>> same multiplexer controller can be used for several independent things,
>> there should be one place to implement support for said multiplexer
>> controller.
>>
>> A single multiplexer controller can also be used to control several
>> parallel multiplexers, that are in turn used by different subsystems
>> in the kernel, leading to a need to coordinate multiplexer accesses.
>> The multiplexer subsystem handles this coordination.
>>
>> This new mux controller subsystem initially comes with a single backend
>> driver that controls gpio based multiplexers. Even though not needed by
>> this initial driver, the mux controller subsystem is prepared to handle
>> chips with multiple (independent) mux controllers.
>>
>> Reviewed-by: Jonathan Cameron 
>> Signed-off-by: Peter Rosin 
>> ---

*snip*

>> +int mux_control_select(struct mux_control *mux, int state)
> 
> If we let two of these race, ...

The window for this "race" is positively huge. If there are several
mux consumers of a single mux controller, it is self-evident that
if one of them grabs the mux for a long time, the others will suffer.

The design is that the rwsem is reader-locked for the full duration
of a select/deselect operation by the mux consumer.

>> +{
>> +int ret;
>> +
>> +if (down_read_trylock(>lock)) {
>> +if (mux->cached_state == state)
>> +return 0;
>> +/* Sigh, the mux needs updating... */
>> +up_read(>lock);
> 
> ... and both decide the mux needs updating ...
> 
>> +}
>> +
>> +/* ...or it's just contended. */
>> +down_write(>lock);
> 
> ... then the last to get to down_write will just wait here forever (or
> until the first consumer calls mux_control_deselect, which may never
> happen)?

It is vital that the mux consumer call _deselect when it is done with
the mux. Not doing so will surely starve out any other mux consumers.
The whole thing is designed around the fact that mux consumers should
deselect the mux as soon as it's no longer needed.

It's simply not possible to share something as fundamental as a mux
without some cooperation. It's not like suffering mux consumers can
go off and use some other mux, and it's also not possible for a
"competing" mux consumer to just clobber the mux state.

>> +
>> +if (mux->cached_state == state) {
>> +/*
>> + * Hmmm, someone else changed the mux to my liking.
>> + * That makes me wonder how long I waited for nothing?
>> + */
>> +downgrade_write(>lock);
>> +return 0;
>> +}
>> +
>> +ret = mux_control_set(mux, state);
>> +if (ret < 0) {
>> +if (mux->idle_state != MUX_IDLE_AS_IS)
>> +mux_control_set(mux, mux->idle_state);
>> +
>> +up_write(>lock);
>> +return ret;
>> +}
>> +
>> +downgrade_write(>lock);
>> +
>> +return 1;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_control_select);
> 
> I wonder if these should be called mux_control_lock/unlock instead,
> which would allow for try_lock and lock_timeout variants.

Maybe, I'm not totally against it. Do others care to opine?

But mux_control_try_select and mux_control_select_timeout does not
look all that bad either. But maybe foo_lock is making it clearer
that a foo_unlock is needed, if you compared it to foo_select and
foo_unselect? I'm probably not the best person to make the call,
as I know all to well what to expect from the functions...

Cheers,
peda

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-19 11:06, Philipp Zabel wrote:
> On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
>> Add a new minimalistic subsystem that handles multiplexer controllers.
>> When multiplexers are used in various places in the kernel, and the
>> same multiplexer controller can be used for several independent things,
>> there should be one place to implement support for said multiplexer
>> controller.
>>
>> A single multiplexer controller can also be used to control several
>> parallel multiplexers, that are in turn used by different subsystems
>> in the kernel, leading to a need to coordinate multiplexer accesses.
>> The multiplexer subsystem handles this coordination.
>>
>> This new mux controller subsystem initially comes with a single backend
>> driver that controls gpio based multiplexers. Even though not needed by
>> this initial driver, the mux controller subsystem is prepared to handle
>> chips with multiple (independent) mux controllers.
>>
>> Reviewed-by: Jonathan Cameron 
>> Signed-off-by: Peter Rosin 
>> ---

*snip*

>> +int mux_control_select(struct mux_control *mux, int state)
> 
> If we let two of these race, ...

The window for this "race" is positively huge. If there are several
mux consumers of a single mux controller, it is self-evident that
if one of them grabs the mux for a long time, the others will suffer.

The design is that the rwsem is reader-locked for the full duration
of a select/deselect operation by the mux consumer.

>> +{
>> +int ret;
>> +
>> +if (down_read_trylock(>lock)) {
>> +if (mux->cached_state == state)
>> +return 0;
>> +/* Sigh, the mux needs updating... */
>> +up_read(>lock);
> 
> ... and both decide the mux needs updating ...
> 
>> +}
>> +
>> +/* ...or it's just contended. */
>> +down_write(>lock);
> 
> ... then the last to get to down_write will just wait here forever (or
> until the first consumer calls mux_control_deselect, which may never
> happen)?

It is vital that the mux consumer call _deselect when it is done with
the mux. Not doing so will surely starve out any other mux consumers.
The whole thing is designed around the fact that mux consumers should
deselect the mux as soon as it's no longer needed.

It's simply not possible to share something as fundamental as a mux
without some cooperation. It's not like suffering mux consumers can
go off and use some other mux, and it's also not possible for a
"competing" mux consumer to just clobber the mux state.

>> +
>> +if (mux->cached_state == state) {
>> +/*
>> + * Hmmm, someone else changed the mux to my liking.
>> + * That makes me wonder how long I waited for nothing?
>> + */
>> +downgrade_write(>lock);
>> +return 0;
>> +}
>> +
>> +ret = mux_control_set(mux, state);
>> +if (ret < 0) {
>> +if (mux->idle_state != MUX_IDLE_AS_IS)
>> +mux_control_set(mux, mux->idle_state);
>> +
>> +up_write(>lock);
>> +return ret;
>> +}
>> +
>> +downgrade_write(>lock);
>> +
>> +return 1;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_control_select);
> 
> I wonder if these should be called mux_control_lock/unlock instead,
> which would allow for try_lock and lock_timeout variants.

Maybe, I'm not totally against it. Do others care to opine?

But mux_control_try_select and mux_control_select_timeout does not
look all that bad either. But maybe foo_lock is making it clearer
that a foo_unlock is needed, if you compared it to foo_select and
foo_unselect? I'm probably not the best person to make the call,
as I know all to well what to expect from the functions...

Cheers,
peda

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Philipp Zabel]

On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> Add a new minimalistic subsystem that handles multiplexer controllers.
> When multiplexers are used in various places in the kernel, and the
> same multiplexer controller can be used for several independent things,
> there should be one place to implement support for said multiplexer
> controller.
> 
> A single multiplexer controller can also be used to control several
> parallel multiplexers, that are in turn used by different subsystems
> in the kernel, leading to a need to coordinate multiplexer accesses.
> The multiplexer subsystem handles this coordination.
> 
> This new mux controller subsystem initially comes with a single backend
> driver that controls gpio based multiplexers. Even though not needed by
> this initial driver, the mux controller subsystem is prepared to handle
> chips with multiple (independent) mux controllers.
> 
> Reviewed-by: Jonathan Cameron 
> Signed-off-by: Peter Rosin 
> ---
>  Documentation/driver-model/devres.txt |   8 +
>  MAINTAINERS   |   2 +
>  drivers/Kconfig   |   2 +
>  drivers/Makefile  |   1 +
>  drivers/mux/Kconfig   |  34 +++
>  drivers/mux/Makefile  |   6 +
>  drivers/mux/mux-core.c| 422 
> ++
>  drivers/mux/mux-gpio.c| 114 +
>  include/linux/mux.h   | 252 
>  9 files changed, 841 insertions(+)
>  create mode 100644 drivers/mux/Kconfig
>  create mode 100644 drivers/mux/Makefile
>  create mode 100644 drivers/mux/mux-core.c
>  create mode 100644 drivers/mux/mux-gpio.c
>  create mode 100644 include/linux/mux.h
> 
> diff --git a/Documentation/driver-model/devres.txt 
> b/Documentation/driver-model/devres.txt
> index efb8200819d6..e2343d9cbec7 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -337,6 +337,14 @@ MEM
>  MFD
>devm_mfd_add_devices()
>  
> +MUX
> +  devm_mux_chip_alloc()
> +  devm_mux_chip_free()
> +  devm_mux_chip_register()
> +  devm_mux_chip_unregister()
> +  devm_mux_control_get()
> +  devm_mux_control_put()
> +
>  PER-CPU MEM
>devm_alloc_percpu()
>devm_free_percpu()
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7fc06739c8ad..591eba737678 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8563,6 +8563,8 @@ M:  Peter Rosin 
>  S:   Maintained
>  F:   Documentation/devicetree/bindings/mux/
>  F:   include/linux/dt-bindings/mux/
> +F:   include/linux/mux.h
> +F:   drivers/mux/
>  
>  MULTISOUND SOUND DRIVER
>  M:   Andrew Veliath 
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 117ca14ccf85..a7ea13e1b869 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -204,4 +204,6 @@ source "drivers/fpga/Kconfig"
>  
>  source "drivers/fsi/Kconfig"
>  
> +source "drivers/mux/Kconfig"
> +
>  endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 2eced9afba53..c0436f6dd5a9 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -177,3 +177,4 @@ obj-$(CONFIG_ANDROID) += android/
>  obj-$(CONFIG_NVMEM)  += nvmem/
>  obj-$(CONFIG_FPGA)   += fpga/
>  obj-$(CONFIG_FSI)+= fsi/
> +obj-$(CONFIG_MULTIPLEXER)+= mux/
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> new file mode 100644
> index ..41dfe08ead84
> --- /dev/null
> +++ b/drivers/mux/Kconfig
> @@ -0,0 +1,34 @@
> +#
> +# Multiplexer devices
> +#
> +
> +menuconfig MULTIPLEXER
> + tristate "Multiplexer subsystem"
> + help
> +   Multiplexer controller subsystem. Multiplexers are used in a
> +   variety of settings, and this subsystem abstracts their use
> +   so that the rest of the kernel sees a common interface. When
> +   multiple parallel multiplexers are controlled by one single
> +   multiplexer controller, this subsystem also coordinates the
> +   multiplexer accesses.
> +
> +   To compile the subsystem as a module, choose M here: the module will
> +   be called mux-core.
> +
> +if MULTIPLEXER
> +
> +config MUX_GPIO
> + tristate "GPIO-controlled Multiplexer"
> + depends on OF && GPIOLIB
> + help
> +   GPIO-controlled Multiplexer controller.
> +
> +   The driver builds a single multiplexer controller using a number
> +   of gpio pins. For N pins, there will be 2^N possible multiplexer
> +   states. The GPIO pins can be connected (by the hardware) to several
> +   multiplexers, which in that case will be operated in parallel.
> +
> +   To compile the driver as a module, choose M here: the module will
> +   be called mux-gpio.
> +
> +endif
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> new file mode 100644
> index ..bb16953f6290
> --- /dev/null
> +++ b/drivers/mux/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for 

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Philipp Zabel]

On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
> Add a new minimalistic subsystem that handles multiplexer controllers.
> When multiplexers are used in various places in the kernel, and the
> same multiplexer controller can be used for several independent things,
> there should be one place to implement support for said multiplexer
> controller.
> 
> A single multiplexer controller can also be used to control several
> parallel multiplexers, that are in turn used by different subsystems
> in the kernel, leading to a need to coordinate multiplexer accesses.
> The multiplexer subsystem handles this coordination.
> 
> This new mux controller subsystem initially comes with a single backend
> driver that controls gpio based multiplexers. Even though not needed by
> this initial driver, the mux controller subsystem is prepared to handle
> chips with multiple (independent) mux controllers.
> 
> Reviewed-by: Jonathan Cameron 
> Signed-off-by: Peter Rosin 
> ---
>  Documentation/driver-model/devres.txt |   8 +
>  MAINTAINERS   |   2 +
>  drivers/Kconfig   |   2 +
>  drivers/Makefile  |   1 +
>  drivers/mux/Kconfig   |  34 +++
>  drivers/mux/Makefile  |   6 +
>  drivers/mux/mux-core.c| 422 
> ++
>  drivers/mux/mux-gpio.c| 114 +
>  include/linux/mux.h   | 252 
>  9 files changed, 841 insertions(+)
>  create mode 100644 drivers/mux/Kconfig
>  create mode 100644 drivers/mux/Makefile
>  create mode 100644 drivers/mux/mux-core.c
>  create mode 100644 drivers/mux/mux-gpio.c
>  create mode 100644 include/linux/mux.h
> 
> diff --git a/Documentation/driver-model/devres.txt 
> b/Documentation/driver-model/devres.txt
> index efb8200819d6..e2343d9cbec7 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -337,6 +337,14 @@ MEM
>  MFD
>devm_mfd_add_devices()
>  
> +MUX
> +  devm_mux_chip_alloc()
> +  devm_mux_chip_free()
> +  devm_mux_chip_register()
> +  devm_mux_chip_unregister()
> +  devm_mux_control_get()
> +  devm_mux_control_put()
> +
>  PER-CPU MEM
>devm_alloc_percpu()
>devm_free_percpu()
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7fc06739c8ad..591eba737678 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8563,6 +8563,8 @@ M:  Peter Rosin 
>  S:   Maintained
>  F:   Documentation/devicetree/bindings/mux/
>  F:   include/linux/dt-bindings/mux/
> +F:   include/linux/mux.h
> +F:   drivers/mux/
>  
>  MULTISOUND SOUND DRIVER
>  M:   Andrew Veliath 
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 117ca14ccf85..a7ea13e1b869 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -204,4 +204,6 @@ source "drivers/fpga/Kconfig"
>  
>  source "drivers/fsi/Kconfig"
>  
> +source "drivers/mux/Kconfig"
> +
>  endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 2eced9afba53..c0436f6dd5a9 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -177,3 +177,4 @@ obj-$(CONFIG_ANDROID) += android/
>  obj-$(CONFIG_NVMEM)  += nvmem/
>  obj-$(CONFIG_FPGA)   += fpga/
>  obj-$(CONFIG_FSI)+= fsi/
> +obj-$(CONFIG_MULTIPLEXER)+= mux/
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> new file mode 100644
> index ..41dfe08ead84
> --- /dev/null
> +++ b/drivers/mux/Kconfig
> @@ -0,0 +1,34 @@
> +#
> +# Multiplexer devices
> +#
> +
> +menuconfig MULTIPLEXER
> + tristate "Multiplexer subsystem"
> + help
> +   Multiplexer controller subsystem. Multiplexers are used in a
> +   variety of settings, and this subsystem abstracts their use
> +   so that the rest of the kernel sees a common interface. When
> +   multiple parallel multiplexers are controlled by one single
> +   multiplexer controller, this subsystem also coordinates the
> +   multiplexer accesses.
> +
> +   To compile the subsystem as a module, choose M here: the module will
> +   be called mux-core.
> +
> +if MULTIPLEXER
> +
> +config MUX_GPIO
> + tristate "GPIO-controlled Multiplexer"
> + depends on OF && GPIOLIB
> + help
> +   GPIO-controlled Multiplexer controller.
> +
> +   The driver builds a single multiplexer controller using a number
> +   of gpio pins. For N pins, there will be 2^N possible multiplexer
> +   states. The GPIO pins can be connected (by the hardware) to several
> +   multiplexers, which in that case will be operated in parallel.
> +
> +   To compile the driver as a module, choose M here: the module will
> +   be called mux-gpio.
> +
> +endif
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> new file mode 100644
> index ..bb16953f6290
> --- /dev/null
> +++ b/drivers/mux/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for multiplexer devices.
> +#
> +
> +obj-$(CONFIG_MULTIPLEXER)+= mux-core.o

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Joe Perches]

On Tue, 2017-04-18 at 23:53 +0200, Peter Rosin wrote:
> On 2017-04-18 13:44, Greg Kroah-Hartman wrote:
> > On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
[]
> > > > > + ret = device_add(_chip->dev);
> > > > > + if (ret < 0)
> > > > > + dev_err(_chip->dev,
> > > > > + "device_add failed in mux_chip_register: %d\n", 
> > > > > ret);
> > > > 
> > > > Did you run checkpatch.pl in strict mode on this new file?  Please do 
> > > > so :)
> > > 
> > > I did, and did it again just to be sure, and I do not get any complaints.
> > > So, what's wrong?
> > 
> > You list the function name in the printk string, it should complain
> > that __func__ should be used.  Oh well, it's just a perl script, it
> > doesn't always catch everything.
> > isn't always correct :)
> 
> Ah, ok.

Also, please use the checkpatch in -next as it has a
slightly better mechanism to identify functions and
uses in strings.

$ ./scripts/checkpatch.pl ~/1.patch
WARNING: Prefer using '"%s...", __func__' to using 'mux_chip_register', this 
function's name, in a string
#302: FILE: drivers/mux/mux-core.c:134:
+   "device_add failed in mux_chip_register: %d\n", ret);

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Joe Perches]

On Tue, 2017-04-18 at 23:53 +0200, Peter Rosin wrote:
> On 2017-04-18 13:44, Greg Kroah-Hartman wrote:
> > On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
[]
> > > > > + ret = device_add(_chip->dev);
> > > > > + if (ret < 0)
> > > > > + dev_err(_chip->dev,
> > > > > + "device_add failed in mux_chip_register: %d\n", 
> > > > > ret);
> > > > 
> > > > Did you run checkpatch.pl in strict mode on this new file?  Please do 
> > > > so :)
> > > 
> > > I did, and did it again just to be sure, and I do not get any complaints.
> > > So, what's wrong?
> > 
> > You list the function name in the printk string, it should complain
> > that __func__ should be used.  Oh well, it's just a perl script, it
> > doesn't always catch everything.
> > isn't always correct :)
> 
> Ah, ok.

Also, please use the checkpatch in -next as it has a
slightly better mechanism to identify functions and
uses in strings.

$ ./scripts/checkpatch.pl ~/1.patch
WARNING: Prefer using '"%s...", __func__' to using 'mux_chip_register', this 
function's name, in a string
#302: FILE: drivers/mux/mux-core.c:134:
+   "device_add failed in mux_chip_register: %d\n", ret);

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-18 13:44, Greg Kroah-Hartman wrote:
> On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
>> On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
>>> On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
 +config MUX_GPIO
 +  tristate "GPIO-controlled Multiplexer"
 +  depends on OF && GPIOLIB
>>>
>>> Why have the gpio and mux core in the same patch?
>>
>> One is not usable w/o the other. I can split them if you want to?
> 
> Then why are they two different config options?  Add the core code in
> one patch, and then add the gpio controled mutiplexer in a separate
> patch.

Ah, I meant when there are not yet any other mux drivers. I'll just
split the patch.

 +static struct class mux_class = {
 +  .name = "mux",
 +  .owner = THIS_MODULE,
 +};
>>>
>>> No Documentation/ABI/ update for your sysfs files?  Please do so.
>>
>> Ok I'll look into it. Wasn't even aware that I added any. But there's the
>> new class of course...
> 
> Hint, you have files, the devices that belong to the class :)

Right.

 +static int __init mux_init(void)
 +{
 +  return class_register(_class);
 +}
 +
 +static DEFINE_IDA(mux_ida);
>>>
>>> When your module is unloaded, you forgot to clean this structure up with
>>> what was done with it.
>>
>> I was under the impression that not providing an exit function for modules
>> made the module infrastructure prevent unloading (by bumping some reference
>> counter). Maybe that is a misconception?
> 
> Ah, messy, don't do that.  Make it so you can unload your module please,
> why wouldn't you want that to happen?

What made me do it was the worry that mux drivers might be left dangling
w/o the core. But that can probably only happen if someone is very
deliberately trying to break stuff by forcing unloads??

 +  mux_chip = kzalloc(sizeof(*mux_chip) +
 + controllers * sizeof(*mux_chip->mux) +
 + sizeof_priv, GFP_KERNEL);
 +  if (!mux_chip)
 +  return NULL;
>>>
>>> You don't return PTR_ERR(-ENOMEM)?  Ok, why not?  (I'm not arguing for
>>> it, just curious...)
>>
>> There's no particular reason. Do you think I should change it?
> 
> What does the caller do with an error?  Pass it up to where?  Who gets
> it?  Don't you want the caller to know you are out of memory?

The current callers return -ENOMEM when NULL is returned here. Looks
like I'm going to be doing some fairly major changes anyway so I'll
just change this too while at it...

 +
 +  device_initialize(_chip->dev);
>>>
>>> Why are you not registering the device here as well?  Why have this be a
>>> two step process?
>>
>> Because of idle state handling. The drivers are expected to fill in
>> the desired idle state(s) after allocating the mux controller(s).
>> Then, when registering, the desired idle state is activated (if the
>> idle state is not idle-as-is, of course) and as a last step the mux
>> is "advertised".
> 
> Ok, is that documented in the functions somewhere?

I'll make sure to add it if it's missing.

 +  ret = device_add(_chip->dev);
 +  if (ret < 0)
 +  dev_err(_chip->dev,
 +  "device_add failed in mux_chip_register: %d\n", ret);
>>>
>>> Did you run checkpatch.pl in strict mode on this new file?  Please do so :)
>>
>> I did, and did it again just to be sure, and I do not get any complaints.
>> So, what's wrong?
> 
> You list the function name in the printk string, it should complain
> that __func__ should be used.  Oh well, it's just a perl script, it
> doesn't always catch everything.
> isn't always correct :)

Ah, ok.

 +EXPORT_SYMBOL_GPL(devm_mux_chip_alloc);
>>>
>>>
>>> Having devm functions that create/destroy other struct devices worries
>>> me, do we have other examples of this happening today?  Are you sure you
>>> got the reference counting all correct?
>>
>> drivers/iio/industrialio_core.c:devm_iio_device_alloc
>>
>> Or is the iio case different in some subtle way that I'm missing?
> 
> I don't know, hopefully you got it all correct, I haven't audited that
> code path in a long time :)

Looks as ok to me as it did before. Moving on... :-)

 +
 +static int devm_mux_chip_match(struct device *dev, void *res, void *data)
 +{
 +  struct mux_chip **r = res;
 +
 +  if (WARN_ON(!r || !*r))
>>>
>>> How can this happen?
>>
>> It shouldn't. I copied the pattern from the iio subsystem.
> 
> Then it should be removed there too...

Ok, I'll see if I can find time to suggest some patch(es) to Jonathan.

 +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip)
 +{
 +  WARN_ON(devres_release(dev, devm_mux_chip_release,
 + devm_mux_chip_match, mux_chip));
>>>
>>> What can someone do with these WARN_ON() splats in the kernel log?
>>
>> Don't know. Again, I copied the pattern from the iio subsystem.
> 
> If you don't know what it should be used for, don't copy it!
> 
> 

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-18 13:44, Greg Kroah-Hartman wrote:
> On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
>> On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
>>> On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
 +config MUX_GPIO
 +  tristate "GPIO-controlled Multiplexer"
 +  depends on OF && GPIOLIB
>>>
>>> Why have the gpio and mux core in the same patch?
>>
>> One is not usable w/o the other. I can split them if you want to?
> 
> Then why are they two different config options?  Add the core code in
> one patch, and then add the gpio controled mutiplexer in a separate
> patch.

Ah, I meant when there are not yet any other mux drivers. I'll just
split the patch.

 +static struct class mux_class = {
 +  .name = "mux",
 +  .owner = THIS_MODULE,
 +};
>>>
>>> No Documentation/ABI/ update for your sysfs files?  Please do so.
>>
>> Ok I'll look into it. Wasn't even aware that I added any. But there's the
>> new class of course...
> 
> Hint, you have files, the devices that belong to the class :)

Right.

 +static int __init mux_init(void)
 +{
 +  return class_register(_class);
 +}
 +
 +static DEFINE_IDA(mux_ida);
>>>
>>> When your module is unloaded, you forgot to clean this structure up with
>>> what was done with it.
>>
>> I was under the impression that not providing an exit function for modules
>> made the module infrastructure prevent unloading (by bumping some reference
>> counter). Maybe that is a misconception?
> 
> Ah, messy, don't do that.  Make it so you can unload your module please,
> why wouldn't you want that to happen?

What made me do it was the worry that mux drivers might be left dangling
w/o the core. But that can probably only happen if someone is very
deliberately trying to break stuff by forcing unloads??

 +  mux_chip = kzalloc(sizeof(*mux_chip) +
 + controllers * sizeof(*mux_chip->mux) +
 + sizeof_priv, GFP_KERNEL);
 +  if (!mux_chip)
 +  return NULL;
>>>
>>> You don't return PTR_ERR(-ENOMEM)?  Ok, why not?  (I'm not arguing for
>>> it, just curious...)
>>
>> There's no particular reason. Do you think I should change it?
> 
> What does the caller do with an error?  Pass it up to where?  Who gets
> it?  Don't you want the caller to know you are out of memory?

The current callers return -ENOMEM when NULL is returned here. Looks
like I'm going to be doing some fairly major changes anyway so I'll
just change this too while at it...

 +
 +  device_initialize(_chip->dev);
>>>
>>> Why are you not registering the device here as well?  Why have this be a
>>> two step process?
>>
>> Because of idle state handling. The drivers are expected to fill in
>> the desired idle state(s) after allocating the mux controller(s).
>> Then, when registering, the desired idle state is activated (if the
>> idle state is not idle-as-is, of course) and as a last step the mux
>> is "advertised".
> 
> Ok, is that documented in the functions somewhere?

I'll make sure to add it if it's missing.

 +  ret = device_add(_chip->dev);
 +  if (ret < 0)
 +  dev_err(_chip->dev,
 +  "device_add failed in mux_chip_register: %d\n", ret);
>>>
>>> Did you run checkpatch.pl in strict mode on this new file?  Please do so :)
>>
>> I did, and did it again just to be sure, and I do not get any complaints.
>> So, what's wrong?
> 
> You list the function name in the printk string, it should complain
> that __func__ should be used.  Oh well, it's just a perl script, it
> doesn't always catch everything.
> isn't always correct :)

Ah, ok.

 +EXPORT_SYMBOL_GPL(devm_mux_chip_alloc);
>>>
>>>
>>> Having devm functions that create/destroy other struct devices worries
>>> me, do we have other examples of this happening today?  Are you sure you
>>> got the reference counting all correct?
>>
>> drivers/iio/industrialio_core.c:devm_iio_device_alloc
>>
>> Or is the iio case different in some subtle way that I'm missing?
> 
> I don't know, hopefully you got it all correct, I haven't audited that
> code path in a long time :)

Looks as ok to me as it did before. Moving on... :-)

 +
 +static int devm_mux_chip_match(struct device *dev, void *res, void *data)
 +{
 +  struct mux_chip **r = res;
 +
 +  if (WARN_ON(!r || !*r))
>>>
>>> How can this happen?
>>
>> It shouldn't. I copied the pattern from the iio subsystem.
> 
> Then it should be removed there too...

Ok, I'll see if I can find time to suggest some patch(es) to Jonathan.

 +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip)
 +{
 +  WARN_ON(devres_release(dev, devm_mux_chip_release,
 + devm_mux_chip_match, mux_chip));
>>>
>>> What can someone do with these WARN_ON() splats in the kernel log?
>>
>> Don't know. Again, I copied the pattern from the iio subsystem.
> 
> If you don't know what it should be used for, don't copy it!
> 
> 

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Greg Kroah-Hartman]

On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
> On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
> > On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
> >> +config MUX_GPIO
> >> +  tristate "GPIO-controlled Multiplexer"
> >> +  depends on OF && GPIOLIB
> > 
> > Why have the gpio and mux core in the same patch?
> 
> One is not usable w/o the other. I can split them if you want to?

Then why are they two different config options?  Add the core code in
one patch, and then add the gpio controled mutiplexer in a separate
patch.

> >> +static struct class mux_class = {
> >> +  .name = "mux",
> >> +  .owner = THIS_MODULE,
> >> +};
> > 
> > No Documentation/ABI/ update for your sysfs files?  Please do so.
> 
> Ok I'll look into it. Wasn't even aware that I added any. But there's the
> new class of course...

Hint, you have files, the devices that belong to the class :)

> >> +static int __init mux_init(void)
> >> +{
> >> +  return class_register(_class);
> >> +}
> >> +
> >> +static DEFINE_IDA(mux_ida);
> > 
> > When your module is unloaded, you forgot to clean this structure up with
> > what was done with it.
> 
> I was under the impression that not providing an exit function for modules
> made the module infrastructure prevent unloading (by bumping some reference
> counter). Maybe that is a misconception?

Ah, messy, don't do that.  Make it so you can unload your module please,
why wouldn't you want that to happen?

> >> +  mux_chip = kzalloc(sizeof(*mux_chip) +
> >> + controllers * sizeof(*mux_chip->mux) +
> >> + sizeof_priv, GFP_KERNEL);
> >> +  if (!mux_chip)
> >> +  return NULL;
> > 
> > You don't return PTR_ERR(-ENOMEM)?  Ok, why not?  (I'm not arguing for
> > it, just curious...)
> 
> There's no particular reason. Do you think I should change it?

What does the caller do with an error?  Pass it up to where?  Who gets
it?  Don't you want the caller to know you are out of memory?

> >> +
> >> +  device_initialize(_chip->dev);
> > 
> > Why are you not registering the device here as well?  Why have this be a
> > two step process?
> 
> Because of idle state handling. The drivers are expected to fill in
> the desired idle state(s) after allocating the mux controller(s).
> Then, when registering, the desired idle state is activated (if the
> idle state is not idle-as-is, of course) and as a last step the mux
> is "advertised".

Ok, is that documented in the functions somewhere?

> >> +  ret = device_add(_chip->dev);
> >> +  if (ret < 0)
> >> +  dev_err(_chip->dev,
> >> +  "device_add failed in mux_chip_register: %d\n", ret);
> > 
> > Did you run checkpatch.pl in strict mode on this new file?  Please do so :)
> 
> I did, and did it again just to be sure, and I do not get any complaints.
> So, what's wrong?

You list the function name in the printk string, it should complain
that __func__ should be used.  Oh well, it's just a perl script, it
doesn't always catch everything.
isn't always correct :)

> >> +EXPORT_SYMBOL_GPL(devm_mux_chip_alloc);
> > 
> > 
> > Having devm functions that create/destroy other struct devices worries
> > me, do we have other examples of this happening today?  Are you sure you
> > got the reference counting all correct?
> 
> drivers/iio/industrialio_core.c:devm_iio_device_alloc
> 
> Or is the iio case different in some subtle way that I'm missing?

I don't know, hopefully you got it all correct, I haven't audited that
code path in a long time :)

> >> +
> >> +static int devm_mux_chip_match(struct device *dev, void *res, void *data)
> >> +{
> >> +  struct mux_chip **r = res;
> >> +
> >> +  if (WARN_ON(!r || !*r))
> > 
> > How can this happen?
> 
> It shouldn't. I copied the pattern from the iio subsystem.

Then it should be removed there too...

> >> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip)
> >> +{
> >> +  WARN_ON(devres_release(dev, devm_mux_chip_release,
> >> + devm_mux_chip_match, mux_chip));
> > 
> > What can someone do with these WARN_ON() splats in the kernel log?
> 
> Don't know. Again, I copied the pattern from the iio subsystem.

If you don't know what it should be used for, don't copy it!

Cargo-cult coding is horrible, please no.

> >> +  /* ...or it's just contended. */
> >> +  down_write(>lock);
> > 
> > Why use a read/write lock at all?  Have you tested this to verify it
> > really is faster and needed?
> 
> For one of the HW configuration that drove the development, the same mux
> controller is used to mux both an I2C channel and a couple of ADC lines.
> 
> If there is no kind of reader/writer locking going on, there is no way to
> do ADC readings concurrently with an I2C transfer even when the consumers
> want the mux in the same position. With an ordinary mutex controlling the
> mux position, the consumers will unconditionally get serialized, which
> seems like a waste to me. Or maybe I'm missing something?

Why is 

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Greg Kroah-Hartman]

On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
> On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
> > On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
> >> +config MUX_GPIO
> >> +  tristate "GPIO-controlled Multiplexer"
> >> +  depends on OF && GPIOLIB
> > 
> > Why have the gpio and mux core in the same patch?
> 
> One is not usable w/o the other. I can split them if you want to?

Then why are they two different config options?  Add the core code in
one patch, and then add the gpio controled mutiplexer in a separate
patch.

> >> +static struct class mux_class = {
> >> +  .name = "mux",
> >> +  .owner = THIS_MODULE,
> >> +};
> > 
> > No Documentation/ABI/ update for your sysfs files?  Please do so.
> 
> Ok I'll look into it. Wasn't even aware that I added any. But there's the
> new class of course...

Hint, you have files, the devices that belong to the class :)

> >> +static int __init mux_init(void)
> >> +{
> >> +  return class_register(_class);
> >> +}
> >> +
> >> +static DEFINE_IDA(mux_ida);
> > 
> > When your module is unloaded, you forgot to clean this structure up with
> > what was done with it.
> 
> I was under the impression that not providing an exit function for modules
> made the module infrastructure prevent unloading (by bumping some reference
> counter). Maybe that is a misconception?

Ah, messy, don't do that.  Make it so you can unload your module please,
why wouldn't you want that to happen?

> >> +  mux_chip = kzalloc(sizeof(*mux_chip) +
> >> + controllers * sizeof(*mux_chip->mux) +
> >> + sizeof_priv, GFP_KERNEL);
> >> +  if (!mux_chip)
> >> +  return NULL;
> > 
> > You don't return PTR_ERR(-ENOMEM)?  Ok, why not?  (I'm not arguing for
> > it, just curious...)
> 
> There's no particular reason. Do you think I should change it?

What does the caller do with an error?  Pass it up to where?  Who gets
it?  Don't you want the caller to know you are out of memory?

> >> +
> >> +  device_initialize(_chip->dev);
> > 
> > Why are you not registering the device here as well?  Why have this be a
> > two step process?
> 
> Because of idle state handling. The drivers are expected to fill in
> the desired idle state(s) after allocating the mux controller(s).
> Then, when registering, the desired idle state is activated (if the
> idle state is not idle-as-is, of course) and as a last step the mux
> is "advertised".

Ok, is that documented in the functions somewhere?

> >> +  ret = device_add(_chip->dev);
> >> +  if (ret < 0)
> >> +  dev_err(_chip->dev,
> >> +  "device_add failed in mux_chip_register: %d\n", ret);
> > 
> > Did you run checkpatch.pl in strict mode on this new file?  Please do so :)
> 
> I did, and did it again just to be sure, and I do not get any complaints.
> So, what's wrong?

You list the function name in the printk string, it should complain
that __func__ should be used.  Oh well, it's just a perl script, it
doesn't always catch everything.
isn't always correct :)

> >> +EXPORT_SYMBOL_GPL(devm_mux_chip_alloc);
> > 
> > 
> > Having devm functions that create/destroy other struct devices worries
> > me, do we have other examples of this happening today?  Are you sure you
> > got the reference counting all correct?
> 
> drivers/iio/industrialio_core.c:devm_iio_device_alloc
> 
> Or is the iio case different in some subtle way that I'm missing?

I don't know, hopefully you got it all correct, I haven't audited that
code path in a long time :)

> >> +
> >> +static int devm_mux_chip_match(struct device *dev, void *res, void *data)
> >> +{
> >> +  struct mux_chip **r = res;
> >> +
> >> +  if (WARN_ON(!r || !*r))
> > 
> > How can this happen?
> 
> It shouldn't. I copied the pattern from the iio subsystem.

Then it should be removed there too...

> >> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip)
> >> +{
> >> +  WARN_ON(devres_release(dev, devm_mux_chip_release,
> >> + devm_mux_chip_match, mux_chip));
> > 
> > What can someone do with these WARN_ON() splats in the kernel log?
> 
> Don't know. Again, I copied the pattern from the iio subsystem.

If you don't know what it should be used for, don't copy it!

Cargo-cult coding is horrible, please no.

> >> +  /* ...or it's just contended. */
> >> +  down_write(>lock);
> > 
> > Why use a read/write lock at all?  Have you tested this to verify it
> > really is faster and needed?
> 
> For one of the HW configuration that drove the development, the same mux
> controller is used to mux both an I2C channel and a couple of ADC lines.
> 
> If there is no kind of reader/writer locking going on, there is no way to
> do ADC readings concurrently with an I2C transfer even when the consumers
> want the mux in the same position. With an ordinary mutex controlling the
> mux position, the consumers will unconditionally get serialized, which
> seems like a waste to me. Or maybe I'm missing something?

Why is 

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
> On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
>> +config MUX_GPIO
>> +tristate "GPIO-controlled Multiplexer"
>> +depends on OF && GPIOLIB
> 
> Why have the gpio and mux core in the same patch?

One is not usable w/o the other. I can split them if you want to?

> And why does this depend on OF?

That's historical, I was originally using of_property_read_u32.
I'll remove the dep...

>> +help
>> +  GPIO-controlled Multiplexer controller.
>> +
>> +  The driver builds a single multiplexer controller using a number
>> +  of gpio pins. For N pins, there will be 2^N possible multiplexer
>> +  states. The GPIO pins can be connected (by the hardware) to several
>> +  multiplexers, which in that case will be operated in parallel.
>> +
>> +  To compile the driver as a module, choose M here: the module will
>> +  be called mux-gpio.
>> +
>> +endif
>> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
>> new file mode 100644
>> index ..bb16953f6290
>> --- /dev/null
>> +++ b/drivers/mux/Makefile
>> @@ -0,0 +1,6 @@
>> +#
>> +# Makefile for multiplexer devices.
>> +#
>> +
>> +obj-$(CONFIG_MULTIPLEXER)   += mux-core.o
>> +obj-$(CONFIG_MUX_GPIO)  += mux-gpio.o
>> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
>> new file mode 100644
>> index ..66a8bccfc3d7
>> --- /dev/null
>> +++ b/drivers/mux/mux-core.c
>> @@ -0,0 +1,422 @@
>> +/*
>> + * Multiplexer subsystem
>> + *
>> + * Copyright (C) 2017 Axentia Technologies AB
>> + *
>> + * Author: Peter Rosin 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#define pr_fmt(fmt) "mux-core: " fmt
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +/*
>> + * The idle-as-is "state" is not an actual state that may be selected, it
>> + * only implies that the state should not be changed. So, use that state
>> + * as indication that the cached state of the multiplexer is unknown.
>> + */
>> +#define MUX_CACHE_UNKNOWN MUX_IDLE_AS_IS
>> +
>> +static struct class mux_class = {
>> +.name = "mux",
>> +.owner = THIS_MODULE,
>> +};
> 
> No Documentation/ABI/ update for your sysfs files?  Please do so.

Ok I'll look into it. Wasn't even aware that I added any. But there's the
new class of course...

>> +
>> +static int __init mux_init(void)
>> +{
>> +return class_register(_class);
>> +}
>> +
>> +static DEFINE_IDA(mux_ida);
> 
> When your module is unloaded, you forgot to clean this structure up with
> what was done with it.

I was under the impression that not providing an exit function for modules
made the module infrastructure prevent unloading (by bumping some reference
counter). Maybe that is a misconception?

>> +
>> +static void mux_chip_release(struct device *dev)
>> +{
>> +struct mux_chip *mux_chip = to_mux_chip(dev);
>> +
>> +ida_simple_remove(_ida, mux_chip->id);
>> +kfree(mux_chip);
>> +}
>> +
>> +static struct device_type mux_type = {
>> +.name = "mux-chip",
>> +.release = mux_chip_release,
>> +};
>> +
>> +struct mux_chip *mux_chip_alloc(struct device *dev,
>> +unsigned int controllers, size_t sizeof_priv)
>> +{
>> +struct mux_chip *mux_chip;
>> +int i;
>> +
>> +if (WARN_ON(!dev || !controllers))
>> +return NULL;
>> +
>> +mux_chip = kzalloc(sizeof(*mux_chip) +
>> +   controllers * sizeof(*mux_chip->mux) +
>> +   sizeof_priv, GFP_KERNEL);
>> +if (!mux_chip)
>> +return NULL;
> 
> You don't return PTR_ERR(-ENOMEM)?  Ok, why not?  (I'm not arguing for
> it, just curious...)

There's no particular reason. Do you think I should change it?

>> +
>> +mux_chip->mux = (struct mux_control *)(mux_chip + 1);
>> +mux_chip->dev.class = _class;
>> +mux_chip->dev.type = _type;
>> +mux_chip->dev.parent = dev;
>> +mux_chip->dev.of_node = dev->of_node;
>> +dev_set_drvdata(_chip->dev, mux_chip);
>> +
>> +mux_chip->id = ida_simple_get(_ida, 0, 0, GFP_KERNEL);
>> +if (mux_chip->id < 0) {
>> +pr_err("muxchipX failed to get a device id\n");
>> +kfree(mux_chip);
>> +return NULL;
>> +}
>> +dev_set_name(_chip->dev, "muxchip%d", mux_chip->id);
>> +
>> +mux_chip->controllers = controllers;
>> +for (i = 0; i < controllers; ++i) {
>> +struct mux_control *mux = _chip->mux[i];
>> +
>> +mux->chip = mux_chip;
>> +init_rwsem(>lock);
>> +mux->cached_state = MUX_CACHE_UNKNOWN;
>> +mux->idle_state = MUX_IDLE_AS_IS;
>> +}
>> +
>> +device_initialize(_chip->dev);
> 
> Why are you not 

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
> On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
>> +config MUX_GPIO
>> +tristate "GPIO-controlled Multiplexer"
>> +depends on OF && GPIOLIB
> 
> Why have the gpio and mux core in the same patch?

One is not usable w/o the other. I can split them if you want to?

> And why does this depend on OF?

That's historical, I was originally using of_property_read_u32.
I'll remove the dep...

>> +help
>> +  GPIO-controlled Multiplexer controller.
>> +
>> +  The driver builds a single multiplexer controller using a number
>> +  of gpio pins. For N pins, there will be 2^N possible multiplexer
>> +  states. The GPIO pins can be connected (by the hardware) to several
>> +  multiplexers, which in that case will be operated in parallel.
>> +
>> +  To compile the driver as a module, choose M here: the module will
>> +  be called mux-gpio.
>> +
>> +endif
>> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
>> new file mode 100644
>> index ..bb16953f6290
>> --- /dev/null
>> +++ b/drivers/mux/Makefile
>> @@ -0,0 +1,6 @@
>> +#
>> +# Makefile for multiplexer devices.
>> +#
>> +
>> +obj-$(CONFIG_MULTIPLEXER)   += mux-core.o
>> +obj-$(CONFIG_MUX_GPIO)  += mux-gpio.o
>> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
>> new file mode 100644
>> index ..66a8bccfc3d7
>> --- /dev/null
>> +++ b/drivers/mux/mux-core.c
>> @@ -0,0 +1,422 @@
>> +/*
>> + * Multiplexer subsystem
>> + *
>> + * Copyright (C) 2017 Axentia Technologies AB
>> + *
>> + * Author: Peter Rosin 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#define pr_fmt(fmt) "mux-core: " fmt
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +/*
>> + * The idle-as-is "state" is not an actual state that may be selected, it
>> + * only implies that the state should not be changed. So, use that state
>> + * as indication that the cached state of the multiplexer is unknown.
>> + */
>> +#define MUX_CACHE_UNKNOWN MUX_IDLE_AS_IS
>> +
>> +static struct class mux_class = {
>> +.name = "mux",
>> +.owner = THIS_MODULE,
>> +};
> 
> No Documentation/ABI/ update for your sysfs files?  Please do so.

Ok I'll look into it. Wasn't even aware that I added any. But there's the
new class of course...

>> +
>> +static int __init mux_init(void)
>> +{
>> +return class_register(_class);
>> +}
>> +
>> +static DEFINE_IDA(mux_ida);
> 
> When your module is unloaded, you forgot to clean this structure up with
> what was done with it.

I was under the impression that not providing an exit function for modules
made the module infrastructure prevent unloading (by bumping some reference
counter). Maybe that is a misconception?

>> +
>> +static void mux_chip_release(struct device *dev)
>> +{
>> +struct mux_chip *mux_chip = to_mux_chip(dev);
>> +
>> +ida_simple_remove(_ida, mux_chip->id);
>> +kfree(mux_chip);
>> +}
>> +
>> +static struct device_type mux_type = {
>> +.name = "mux-chip",
>> +.release = mux_chip_release,
>> +};
>> +
>> +struct mux_chip *mux_chip_alloc(struct device *dev,
>> +unsigned int controllers, size_t sizeof_priv)
>> +{
>> +struct mux_chip *mux_chip;
>> +int i;
>> +
>> +if (WARN_ON(!dev || !controllers))
>> +return NULL;
>> +
>> +mux_chip = kzalloc(sizeof(*mux_chip) +
>> +   controllers * sizeof(*mux_chip->mux) +
>> +   sizeof_priv, GFP_KERNEL);
>> +if (!mux_chip)
>> +return NULL;
> 
> You don't return PTR_ERR(-ENOMEM)?  Ok, why not?  (I'm not arguing for
> it, just curious...)

There's no particular reason. Do you think I should change it?

>> +
>> +mux_chip->mux = (struct mux_control *)(mux_chip + 1);
>> +mux_chip->dev.class = _class;
>> +mux_chip->dev.type = _type;
>> +mux_chip->dev.parent = dev;
>> +mux_chip->dev.of_node = dev->of_node;
>> +dev_set_drvdata(_chip->dev, mux_chip);
>> +
>> +mux_chip->id = ida_simple_get(_ida, 0, 0, GFP_KERNEL);
>> +if (mux_chip->id < 0) {
>> +pr_err("muxchipX failed to get a device id\n");
>> +kfree(mux_chip);
>> +return NULL;
>> +}
>> +dev_set_name(_chip->dev, "muxchip%d", mux_chip->id);
>> +
>> +mux_chip->controllers = controllers;
>> +for (i = 0; i < controllers; ++i) {
>> +struct mux_control *mux = _chip->mux[i];
>> +
>> +mux->chip = mux_chip;
>> +init_rwsem(>lock);
>> +mux->cached_state = MUX_CACHE_UNKNOWN;
>> +mux->idle_state = MUX_IDLE_AS_IS;
>> +}
>> +
>> +device_initialize(_chip->dev);
> 
> Why are you not registering the device here 

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Greg Kroah-Hartman]

On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
> +config MUX_GPIO
> + tristate "GPIO-controlled Multiplexer"
> + depends on OF && GPIOLIB

Why have the gpio and mux core in the same patch?

And why does this depend on OF?


> + help
> +   GPIO-controlled Multiplexer controller.
> +
> +   The driver builds a single multiplexer controller using a number
> +   of gpio pins. For N pins, there will be 2^N possible multiplexer
> +   states. The GPIO pins can be connected (by the hardware) to several
> +   multiplexers, which in that case will be operated in parallel.
> +
> +   To compile the driver as a module, choose M here: the module will
> +   be called mux-gpio.
> +
> +endif
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> new file mode 100644
> index ..bb16953f6290
> --- /dev/null
> +++ b/drivers/mux/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for multiplexer devices.
> +#
> +
> +obj-$(CONFIG_MULTIPLEXER)+= mux-core.o
> +obj-$(CONFIG_MUX_GPIO)   += mux-gpio.o
> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
> new file mode 100644
> index ..66a8bccfc3d7
> --- /dev/null
> +++ b/drivers/mux/mux-core.c
> @@ -0,0 +1,422 @@
> +/*
> + * Multiplexer subsystem
> + *
> + * Copyright (C) 2017 Axentia Technologies AB
> + *
> + * Author: Peter Rosin 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#define pr_fmt(fmt) "mux-core: " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * The idle-as-is "state" is not an actual state that may be selected, it
> + * only implies that the state should not be changed. So, use that state
> + * as indication that the cached state of the multiplexer is unknown.
> + */
> +#define MUX_CACHE_UNKNOWN MUX_IDLE_AS_IS
> +
> +static struct class mux_class = {
> + .name = "mux",
> + .owner = THIS_MODULE,
> +};

No Documentation/ABI/ update for your sysfs files?  Please do so.

> +
> +static int __init mux_init(void)
> +{
> + return class_register(_class);
> +}
> +
> +static DEFINE_IDA(mux_ida);

When your module is unloaded, you forgot to clean this structure up with
what was done with it.

> +
> +static void mux_chip_release(struct device *dev)
> +{
> + struct mux_chip *mux_chip = to_mux_chip(dev);
> +
> + ida_simple_remove(_ida, mux_chip->id);
> + kfree(mux_chip);
> +}
> +
> +static struct device_type mux_type = {
> + .name = "mux-chip",
> + .release = mux_chip_release,
> +};
> +
> +struct mux_chip *mux_chip_alloc(struct device *dev,
> + unsigned int controllers, size_t sizeof_priv)
> +{
> + struct mux_chip *mux_chip;
> + int i;
> +
> + if (WARN_ON(!dev || !controllers))
> + return NULL;
> +
> + mux_chip = kzalloc(sizeof(*mux_chip) +
> +controllers * sizeof(*mux_chip->mux) +
> +sizeof_priv, GFP_KERNEL);
> + if (!mux_chip)
> + return NULL;

You don't return PTR_ERR(-ENOMEM)?  Ok, why not?  (I'm not arguing for
it, just curious...)

> +
> + mux_chip->mux = (struct mux_control *)(mux_chip + 1);
> + mux_chip->dev.class = _class;
> + mux_chip->dev.type = _type;
> + mux_chip->dev.parent = dev;
> + mux_chip->dev.of_node = dev->of_node;
> + dev_set_drvdata(_chip->dev, mux_chip);
> +
> + mux_chip->id = ida_simple_get(_ida, 0, 0, GFP_KERNEL);
> + if (mux_chip->id < 0) {
> + pr_err("muxchipX failed to get a device id\n");
> + kfree(mux_chip);
> + return NULL;
> + }
> + dev_set_name(_chip->dev, "muxchip%d", mux_chip->id);
> +
> + mux_chip->controllers = controllers;
> + for (i = 0; i < controllers; ++i) {
> + struct mux_control *mux = _chip->mux[i];
> +
> + mux->chip = mux_chip;
> + init_rwsem(>lock);
> + mux->cached_state = MUX_CACHE_UNKNOWN;
> + mux->idle_state = MUX_IDLE_AS_IS;
> + }
> +
> + device_initialize(_chip->dev);

Why are you not registering the device here as well?  Why have this be a
two step process?

> +
> + return mux_chip;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_alloc);
> +
> +static int mux_control_set(struct mux_control *mux, int state)
> +{
> + int ret = mux->chip->ops->set(mux, state);
> +
> + mux->cached_state = ret < 0 ? MUX_CACHE_UNKNOWN : state;
> +
> + return ret;
> +}
> +
> +int mux_chip_register(struct mux_chip *mux_chip)
> +{
> + int i;
> + int ret;
> +
> + for (i = 0; i < mux_chip->controllers; ++i) {
> + struct mux_control *mux = _chip->mux[i];
> +
> + if (mux->idle_state == mux->cached_state)
> + 

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Greg Kroah-Hartman]

On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
> +config MUX_GPIO
> + tristate "GPIO-controlled Multiplexer"
> + depends on OF && GPIOLIB

Why have the gpio and mux core in the same patch?

And why does this depend on OF?


> + help
> +   GPIO-controlled Multiplexer controller.
> +
> +   The driver builds a single multiplexer controller using a number
> +   of gpio pins. For N pins, there will be 2^N possible multiplexer
> +   states. The GPIO pins can be connected (by the hardware) to several
> +   multiplexers, which in that case will be operated in parallel.
> +
> +   To compile the driver as a module, choose M here: the module will
> +   be called mux-gpio.
> +
> +endif
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> new file mode 100644
> index ..bb16953f6290
> --- /dev/null
> +++ b/drivers/mux/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for multiplexer devices.
> +#
> +
> +obj-$(CONFIG_MULTIPLEXER)+= mux-core.o
> +obj-$(CONFIG_MUX_GPIO)   += mux-gpio.o
> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
> new file mode 100644
> index ..66a8bccfc3d7
> --- /dev/null
> +++ b/drivers/mux/mux-core.c
> @@ -0,0 +1,422 @@
> +/*
> + * Multiplexer subsystem
> + *
> + * Copyright (C) 2017 Axentia Technologies AB
> + *
> + * Author: Peter Rosin 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#define pr_fmt(fmt) "mux-core: " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * The idle-as-is "state" is not an actual state that may be selected, it
> + * only implies that the state should not be changed. So, use that state
> + * as indication that the cached state of the multiplexer is unknown.
> + */
> +#define MUX_CACHE_UNKNOWN MUX_IDLE_AS_IS
> +
> +static struct class mux_class = {
> + .name = "mux",
> + .owner = THIS_MODULE,
> +};

No Documentation/ABI/ update for your sysfs files?  Please do so.

> +
> +static int __init mux_init(void)
> +{
> + return class_register(_class);
> +}
> +
> +static DEFINE_IDA(mux_ida);

When your module is unloaded, you forgot to clean this structure up with
what was done with it.

> +
> +static void mux_chip_release(struct device *dev)
> +{
> + struct mux_chip *mux_chip = to_mux_chip(dev);
> +
> + ida_simple_remove(_ida, mux_chip->id);
> + kfree(mux_chip);
> +}
> +
> +static struct device_type mux_type = {
> + .name = "mux-chip",
> + .release = mux_chip_release,
> +};
> +
> +struct mux_chip *mux_chip_alloc(struct device *dev,
> + unsigned int controllers, size_t sizeof_priv)
> +{
> + struct mux_chip *mux_chip;
> + int i;
> +
> + if (WARN_ON(!dev || !controllers))
> + return NULL;
> +
> + mux_chip = kzalloc(sizeof(*mux_chip) +
> +controllers * sizeof(*mux_chip->mux) +
> +sizeof_priv, GFP_KERNEL);
> + if (!mux_chip)
> + return NULL;

You don't return PTR_ERR(-ENOMEM)?  Ok, why not?  (I'm not arguing for
it, just curious...)

> +
> + mux_chip->mux = (struct mux_control *)(mux_chip + 1);
> + mux_chip->dev.class = _class;
> + mux_chip->dev.type = _type;
> + mux_chip->dev.parent = dev;
> + mux_chip->dev.of_node = dev->of_node;
> + dev_set_drvdata(_chip->dev, mux_chip);
> +
> + mux_chip->id = ida_simple_get(_ida, 0, 0, GFP_KERNEL);
> + if (mux_chip->id < 0) {
> + pr_err("muxchipX failed to get a device id\n");
> + kfree(mux_chip);
> + return NULL;
> + }
> + dev_set_name(_chip->dev, "muxchip%d", mux_chip->id);
> +
> + mux_chip->controllers = controllers;
> + for (i = 0; i < controllers; ++i) {
> + struct mux_control *mux = _chip->mux[i];
> +
> + mux->chip = mux_chip;
> + init_rwsem(>lock);
> + mux->cached_state = MUX_CACHE_UNKNOWN;
> + mux->idle_state = MUX_IDLE_AS_IS;
> + }
> +
> + device_initialize(_chip->dev);

Why are you not registering the device here as well?  Why have this be a
two step process?

> +
> + return mux_chip;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_alloc);
> +
> +static int mux_control_set(struct mux_control *mux, int state)
> +{
> + int ret = mux->chip->ops->set(mux, state);
> +
> + mux->cached_state = ret < 0 ? MUX_CACHE_UNKNOWN : state;
> +
> + return ret;
> +}
> +
> +int mux_chip_register(struct mux_chip *mux_chip)
> +{
> + int i;
> + int ret;
> +
> + for (i = 0; i < mux_chip->controllers; ++i) {
> + struct mux_control *mux = _chip->mux[i];
> +
> + if (mux->idle_state == mux->cached_state)
> + continue;
> +
> +

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Philipp Zabel]

Hi Peter,

On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
[...]
> diff --git a/include/linux/mux.h b/include/linux/mux.h
> new file mode 100644
> index ..febdde4246df
> --- /dev/null
> +++ b/include/linux/mux.h
[...]

Consider separating mux.h into a consumer header and a driver header.
Right now there is no separation between the consumer API and the
framework internals.

regards
Philipp

Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Philipp Zabel]

Hi Peter,

On Thu, 2017-04-13 at 18:43 +0200, Peter Rosin wrote:
[...]
> diff --git a/include/linux/mux.h b/include/linux/mux.h
> new file mode 100644
> index ..febdde4246df
> --- /dev/null
> +++ b/include/linux/mux.h
[...]

Consider separating mux.h into a consumer header and a driver header.
Right now there is no separation between the consumer API and the
framework internals.

regards
Philipp

[PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

Add a new minimalistic subsystem that handles multiplexer controllers.
When multiplexers are used in various places in the kernel, and the
same multiplexer controller can be used for several independent things,
there should be one place to implement support for said multiplexer
controller.

A single multiplexer controller can also be used to control several
parallel multiplexers, that are in turn used by different subsystems
in the kernel, leading to a need to coordinate multiplexer accesses.
The multiplexer subsystem handles this coordination.

This new mux controller subsystem initially comes with a single backend
driver that controls gpio based multiplexers. Even though not needed by
this initial driver, the mux controller subsystem is prepared to handle
chips with multiple (independent) mux controllers.

Reviewed-by: Jonathan Cameron 
Signed-off-by: Peter Rosin 
---
 Documentation/driver-model/devres.txt |   8 +
 MAINTAINERS   |   2 +
 drivers/Kconfig   |   2 +
 drivers/Makefile  |   1 +
 drivers/mux/Kconfig   |  34 +++
 drivers/mux/Makefile  |   6 +
 drivers/mux/mux-core.c| 422 ++
 drivers/mux/mux-gpio.c| 114 +
 include/linux/mux.h   | 252 
 9 files changed, 841 insertions(+)
 create mode 100644 drivers/mux/Kconfig
 create mode 100644 drivers/mux/Makefile
 create mode 100644 drivers/mux/mux-core.c
 create mode 100644 drivers/mux/mux-gpio.c
 create mode 100644 include/linux/mux.h

diff --git a/Documentation/driver-model/devres.txt 
b/Documentation/driver-model/devres.txt
index efb8200819d6..e2343d9cbec7 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -337,6 +337,14 @@ MEM
 MFD
   devm_mfd_add_devices()
 
+MUX
+  devm_mux_chip_alloc()
+  devm_mux_chip_free()
+  devm_mux_chip_register()
+  devm_mux_chip_unregister()
+  devm_mux_control_get()
+  devm_mux_control_put()
+
 PER-CPU MEM
   devm_alloc_percpu()
   devm_free_percpu()
diff --git a/MAINTAINERS b/MAINTAINERS
index 7fc06739c8ad..591eba737678 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8563,6 +8563,8 @@ M:Peter Rosin 
 S: Maintained
 F: Documentation/devicetree/bindings/mux/
 F: include/linux/dt-bindings/mux/
+F: include/linux/mux.h
+F: drivers/mux/
 
 MULTISOUND SOUND DRIVER
 M: Andrew Veliath 
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 117ca14ccf85..a7ea13e1b869 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -204,4 +204,6 @@ source "drivers/fpga/Kconfig"
 
 source "drivers/fsi/Kconfig"
 
+source "drivers/mux/Kconfig"
+
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 2eced9afba53..c0436f6dd5a9 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -177,3 +177,4 @@ obj-$(CONFIG_ANDROID)   += android/
 obj-$(CONFIG_NVMEM)+= nvmem/
 obj-$(CONFIG_FPGA) += fpga/
 obj-$(CONFIG_FSI)  += fsi/
+obj-$(CONFIG_MULTIPLEXER)  += mux/
diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
new file mode 100644
index ..41dfe08ead84
--- /dev/null
+++ b/drivers/mux/Kconfig
@@ -0,0 +1,34 @@
+#
+# Multiplexer devices
+#
+
+menuconfig MULTIPLEXER
+   tristate "Multiplexer subsystem"
+   help
+ Multiplexer controller subsystem. Multiplexers are used in a
+ variety of settings, and this subsystem abstracts their use
+ so that the rest of the kernel sees a common interface. When
+ multiple parallel multiplexers are controlled by one single
+ multiplexer controller, this subsystem also coordinates the
+ multiplexer accesses.
+
+ To compile the subsystem as a module, choose M here: the module will
+ be called mux-core.
+
+if MULTIPLEXER
+
+config MUX_GPIO
+   tristate "GPIO-controlled Multiplexer"
+   depends on OF && GPIOLIB
+   help
+ GPIO-controlled Multiplexer controller.
+
+ The driver builds a single multiplexer controller using a number
+ of gpio pins. For N pins, there will be 2^N possible multiplexer
+ states. The GPIO pins can be connected (by the hardware) to several
+ multiplexers, which in that case will be operated in parallel.
+
+ To compile the driver as a module, choose M here: the module will
+ be called mux-gpio.
+
+endif
diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
new file mode 100644
index ..bb16953f6290
--- /dev/null
+++ b/drivers/mux/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for multiplexer devices.
+#
+
+obj-$(CONFIG_MULTIPLEXER)  += mux-core.o
+obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
new file mode 100644
index ..66a8bccfc3d7
--- /dev/null
+++ 

[PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller

[Peter Rosin]

Add a new minimalistic subsystem that handles multiplexer controllers.
When multiplexers are used in various places in the kernel, and the
same multiplexer controller can be used for several independent things,
there should be one place to implement support for said multiplexer
controller.

A single multiplexer controller can also be used to control several
parallel multiplexers, that are in turn used by different subsystems
in the kernel, leading to a need to coordinate multiplexer accesses.
The multiplexer subsystem handles this coordination.

This new mux controller subsystem initially comes with a single backend
driver that controls gpio based multiplexers. Even though not needed by
this initial driver, the mux controller subsystem is prepared to handle
chips with multiple (independent) mux controllers.

Reviewed-by: Jonathan Cameron 
Signed-off-by: Peter Rosin 
---
 Documentation/driver-model/devres.txt |   8 +
 MAINTAINERS   |   2 +
 drivers/Kconfig   |   2 +
 drivers/Makefile  |   1 +
 drivers/mux/Kconfig   |  34 +++
 drivers/mux/Makefile  |   6 +
 drivers/mux/mux-core.c| 422 ++
 drivers/mux/mux-gpio.c| 114 +
 include/linux/mux.h   | 252 
 9 files changed, 841 insertions(+)
 create mode 100644 drivers/mux/Kconfig
 create mode 100644 drivers/mux/Makefile
 create mode 100644 drivers/mux/mux-core.c
 create mode 100644 drivers/mux/mux-gpio.c
 create mode 100644 include/linux/mux.h

diff --git a/Documentation/driver-model/devres.txt 
b/Documentation/driver-model/devres.txt
index efb8200819d6..e2343d9cbec7 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -337,6 +337,14 @@ MEM
 MFD
   devm_mfd_add_devices()
 
+MUX
+  devm_mux_chip_alloc()
+  devm_mux_chip_free()
+  devm_mux_chip_register()
+  devm_mux_chip_unregister()
+  devm_mux_control_get()
+  devm_mux_control_put()
+
 PER-CPU MEM
   devm_alloc_percpu()
   devm_free_percpu()
diff --git a/MAINTAINERS b/MAINTAINERS
index 7fc06739c8ad..591eba737678 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8563,6 +8563,8 @@ M:Peter Rosin 
 S: Maintained
 F: Documentation/devicetree/bindings/mux/
 F: include/linux/dt-bindings/mux/
+F: include/linux/mux.h
+F: drivers/mux/
 
 MULTISOUND SOUND DRIVER
 M: Andrew Veliath 
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 117ca14ccf85..a7ea13e1b869 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -204,4 +204,6 @@ source "drivers/fpga/Kconfig"
 
 source "drivers/fsi/Kconfig"
 
+source "drivers/mux/Kconfig"
+
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 2eced9afba53..c0436f6dd5a9 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -177,3 +177,4 @@ obj-$(CONFIG_ANDROID)   += android/
 obj-$(CONFIG_NVMEM)+= nvmem/
 obj-$(CONFIG_FPGA) += fpga/
 obj-$(CONFIG_FSI)  += fsi/
+obj-$(CONFIG_MULTIPLEXER)  += mux/
diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
new file mode 100644
index ..41dfe08ead84
--- /dev/null
+++ b/drivers/mux/Kconfig
@@ -0,0 +1,34 @@
+#
+# Multiplexer devices
+#
+
+menuconfig MULTIPLEXER
+   tristate "Multiplexer subsystem"
+   help
+ Multiplexer controller subsystem. Multiplexers are used in a
+ variety of settings, and this subsystem abstracts their use
+ so that the rest of the kernel sees a common interface. When
+ multiple parallel multiplexers are controlled by one single
+ multiplexer controller, this subsystem also coordinates the
+ multiplexer accesses.
+
+ To compile the subsystem as a module, choose M here: the module will
+ be called mux-core.
+
+if MULTIPLEXER
+
+config MUX_GPIO
+   tristate "GPIO-controlled Multiplexer"
+   depends on OF && GPIOLIB
+   help
+ GPIO-controlled Multiplexer controller.
+
+ The driver builds a single multiplexer controller using a number
+ of gpio pins. For N pins, there will be 2^N possible multiplexer
+ states. The GPIO pins can be connected (by the hardware) to several
+ multiplexers, which in that case will be operated in parallel.
+
+ To compile the driver as a module, choose M here: the module will
+ be called mux-gpio.
+
+endif
diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
new file mode 100644
index ..bb16953f6290
--- /dev/null
+++ b/drivers/mux/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for multiplexer devices.
+#
+
+obj-$(CONFIG_MULTIPLEXER)  += mux-core.o
+obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
new file mode 100644
index ..66a8bccfc3d7
--- /dev/null
+++ b/drivers/mux/mux-core.c
@@ -0,0 +1,422 @@
+/*
+ * Multiplexer subsystem
+ *
+ *