Re: [PATCH v9 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver

[William Breathitt Gray <william.gray@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 2061 bytes --]

On Fri, Dec 16, 2022 at 05:24:02PM +0000, Biju Das wrote:
> Hi William Breathitt Gray,
> 
> > Subject: Re: [PATCH v9 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver
> > 
> > On Fri, Dec 16, 2022 at 05:00:30PM +0000, Biju Das wrote:
> > > > Hello Biju,
> > > >
> > > > Do you need to take the ch->lock before checking ch->is_busy to
> > > > ensure it does not change?
> > >
> > > priv->count_is_enabled[count->id]-> true means channel is held by counter.
> > > So pwm won't be able to change the state ch->is_busy.
> > >
> > > priv->count_is_enabled[count->id]-> false and if there is contention
> > > priv->for ch->busy
> > > whoever is first calling rz_mtu3_request_channel() will get the channel.
> > > among pwm_request and counter_enable.
> > >
> > > So I think it is safe here. Please correct me if I am missing something.
> > >
> > > static inline bool rz_mtu3_request_channel(struct rz_mtu3_channel *ch)
> > > {
> > > 	bool is_idle;
> > >
> > > 	mutex_lock(&ch->lock);
> > > 	is_idle = !ch->is_busy;
> > > 	if (is_idle)
> > > 		ch->is_busy = true;
> > > 	mutex_unlock(&ch->lock);
> > >
> > > 	return is_idle;
> > > }
> > 
> > Okay seems safe then. If the respective count_is_enabled will only be true
> > when the respective channel is held by the counter, is there a need to check
> > ch->is_busy, or would checking count_is_enabled alone suffice?
> 
> We still can configure below properties before enabling the count.
> That can be done only when ch->is_busy = false or ch->is_busy= true and count_is_enabled.
> That is channel is used by the counter.
> 
> /sys/bus/counter/devices/counterX/external_input_phase_clock_select
> /sys/bus/counter/devices/counterX/long_word_access_ctrl_mode
> /sys/bus/counter/devices/counterX/count2/count
> /sys/bus/counter/devices/counterX/count2/ceiling
> 
> Cheers,
> Biju

I see what you mean now, we'll need to check ch->is_busy regardless and
not just the count_is_enable state. That should be fine then, thanks for
explaining.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]