Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1628340 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux  controller Philipp Zabel <p.zabel@pengutronix.de> - 2017-04-21 16:30 +0200
    Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux  controller Philipp Zabel <p.zabel@pengutronix.de> - 2017-04-21 19:10 +0200
    Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux  controller Philipp Zabel <p.zabel@pengutronix.de> - 2017-04-21 19:10 +0200

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

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 = &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(&mux_chip->dev, "unable to set idle state\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = device_add(&mux_chip->dev);
> +	if (ret < 0)
> +		dev_err(&mux_chip->dev,
> +			"device_add failed in mux_chip_register: %d\n", ret);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_register);

regards
Philipp

[toc] | [next] | [standalone]

#1628372

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 = &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

[toc] | [prev] | [next] | [standalone]

#1628383

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 = &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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web