RE: [PATCH v7 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v7 4/5] counter: Add Renesas RZ/G2L MTU3a counter
> driver
> 
> Hi Biju,
> 
> On Thu, Nov 24, 2022 at 6:00 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> > Add RZ/G2L MTU3a counter driver. This IP supports the following phase
> > counting modes on MTU1 and MTU2 channels
> >
> > 1) 16-bit phase counting modes on MTU1 and MTU2 channels.
> > 2) 32-bit phase counting mode by cascading MTU1 and MTU2 channels.
> >
> > This patch adds 3 counter value channels.
> >         count0: 16-bit phase counter value channel on MTU1
> >         count1: 16-bit phase counter value channel on MTU2
> >         count2: 32-bit phase counter value channel by cascading
> >                 MTU1 and MTU2 channels.
> >
> > The external input phase clock pin for the counter value channels are
> > as follows:
> >         count0: "MTCLKA-MTCLKB"
> >         count1: "MTCLKA-MTCLKB" or "MTCLKC-MTCLKD"
> >         count2: "MTCLKA-MTCLKB" or "MTCLKC-MTCLKD"
> >
> > Use the sysfs variable "external_input_phase_clock_select" to select
> > the external input phase clock pin and "cascade_enable" to
> > enable/disable cascading of channels.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v6->v7:
> >  * Updated commit description
> >  * Added Register descriptions
> >  * Opimized size of cache variable by using union
> >  * Used test_bit() in rz_mtu3_is_counter_invalid()
> >  * Replaced val->timer_mode in rz_mtu3_count_function_{read,write}
> >  * Added TODO comment phase3 and phase5 modes.
> >  * replaced if-else with ternary expression in
> > rz_mtu3_count_direction_read()
> >  * Used switch statement in rz_mtu3_count_ceiling_read to consistent
> > with write
> >  * Provided default case for all switch statement.
> >  * Add mutex lock for avoiding races with other devices
> >  * Updated comments in rz_mtu3_action_read
> >  * Replaced COUNTER_COMP_DEVICE_BOOL->COUNTER_COMP_DEVICE_BOOL for
> >    cascade_enable
> >  * Replaced RZ_MTU3_GET_HW_CH->rz_mtu3_get_hw_ch
> >  * Added rz_mtu3_get_ch() to get channels
> >  * used rz_mtu3_shared_reg_update_bit for cascade_enable and
> >    selecting phase input clock.
> >  * Added rz_mtu3_is_counter_invalid() check in
> > rz_mtu3_count_ceiling_read()
> 
> Thanks for the update!
> 
> > --- /dev/null
> > +++ b/drivers/counter/rz-mtu3-cnt.c
> 
> > +static int rz_mtu3_initialize_counter(struct counter_device *counter,
> > +int id) {
> > +       struct rz_mtu3_channel *const ch = rz_mtu3_get_ch(counter, id);
> > +       struct rz_mtu3_channel *const ch1 = rz_mtu3_get_ch(counter, 0);
> > +       struct rz_mtu3_channel *const ch2 = rz_mtu3_get_ch(counter,
> > +1);
> > +
> > +       switch (id) {
> > +       case RZ_MTU3_16_BIT_MTU1_CH:
> > +       case RZ_MTU3_16_BIT_MTU2_CH:
> > +               mutex_lock(&ch->lock);
> > +               if (ch->function == RZ_MTU3_NORMAL)
> > +                       ch->function = RZ_MTU3_16BIT_PHASE_COUNTING;
> > +               mutex_unlock(&ch->lock);
> > +
> > +               if (ch->function != RZ_MTU3_16BIT_PHASE_COUNTING)
> > +                       return -EBUSY;
> 
> I think having a request_channel() API would make this more readable, and
> avoid duplication (here and in the PWM driver).

Agreed.

> 
> > +
> > +               rz_mtu3_16bit_cnt_setting(counter, id);
> > +
> > +               break;
> > +       case RZ_MTU3_32_BIT_CH:
> > +               /*
> > +                * 32-bit phase counting need MTU1 and MTU2 to create
> 32-bit
> > +                * cascade counter.
> > +                */
> > +               mutex_lock(&ch1->lock);
> > +               if (ch1->function == RZ_MTU3_NORMAL)
> > +                       ch1->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> > +               mutex_unlock(&ch1->lock);
> > +
> > +               mutex_lock(&ch2->lock);
> > +               if (ch2->function == RZ_MTU3_NORMAL)
> > +                       ch2->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> > +               mutex_unlock(&ch2->lock);
> > +
> > +               if (ch1->function != RZ_MTU3_32BIT_PHASE_COUNTING ||
> > +                   ch2->function != RZ_MTU3_32BIT_PHASE_COUNTING)
> > +                       return -EBUSY;
> 
> Surely you need to release the channel you did obtain (if any)?
Agreed.

> 
> > +
> > +               rz_mtu3_32bit_cnt_setting(counter, id);
> > +               break;
> > +       default:
> > +               /* should never reach this path */
> > +               return -EINVAL;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static void rz_mtu3_terminate_counter(struct counter_device *counter,
> > +int id) {
> > +       struct rz_mtu3_channel *const ch = rz_mtu3_get_ch(counter, id);
> > +       struct rz_mtu3_channel *const ch1 = rz_mtu3_get_ch(counter, 0);
> > +       struct rz_mtu3_channel *const ch2 = rz_mtu3_get_ch(counter,
> > +1);
> > +
> > +       if (id == RZ_MTU3_32_BIT_CH) {
> > +               mutex_lock(&ch1->lock);
> > +               ch1->function = RZ_MTU3_NORMAL;
> > +               mutex_unlock(&ch1->lock);
> 
> Locking around a simple integer assignment doesn't do much.
> You might as well just use WRITE_ONCE(), to avoid reordering by the
> compiler.

We don't need this anymore once we have request and release calls
as that function updates channel state by holding channel lock.

Cheers
Biju

> 
> > +
> > +               mutex_lock(&ch2->lock);
> > +               ch2->function = RZ_MTU3_NORMAL;
> > +               mutex_unlock(&ch2->lock);
> > +
> > +               rz_mtu3_disable(ch2);
> > +               rz_mtu3_disable(ch1);
> > +       } else {
> > +               mutex_lock(&ch->lock);
> > +               ch->function = RZ_MTU3_NORMAL;
> > +               mutex_unlock(&ch->lock);
> > +
> > +               rz_mtu3_disable(ch);
> > +       }
> > +}
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker.
> But when I'm talking to journalists I just say "programmer" or something
> like that.
>                                 -- Linus Torvalds




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux