Hi Lee Jones,
> Subject: Re: [PATCH v13 2/6] mfd: Add Renesas RZ/G2L MTU3a core driver
>
> On Wed, 08 Mar 2023, Biju Das wrote:
>
> > Hi Lee Jones,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v13 2/6] mfd: Add Renesas RZ/G2L MTU3a core
> > > driver
> > >
> > > On Mon, 06 Mar 2023, Biju Das wrote:
> > >
> > > > Hi Lee Jones,
> > > >
> > > > Thanks for the review.
> > > >
> > > > > Subject: Re: [PATCH v13 2/6] mfd: Add Renesas RZ/G2L MTU3a core
> > > > > driver
> > > > >
> > > > > On Thu, 16 Feb 2023, Biju Das wrote:
> > > > >
> > > > > > The RZ/G2L multi-function timer pulse unit 3 (MTU3a) is
> > > > > > embedded in the Renesas RZ/G2L family SoCs. It consists of
> > > > > > eight 16-bit timer channels and one 32-bit timer channel. It
> > > > > > supports the following functions
> > > > > > - Counter
> > > > > > - Timer
> > > > > > - PWM
> > > > > >
> > > > > > The 8/16/32 bit registers are mixed in each channel.
> > > > > >
> > > > > > Add MTU3a core driver for RZ/G2L SoC. The core driver shares
> > > > > > the clk and channel register access for the other child
> > > > > > devices like Counter, PWM and Clock event.
> > > > > >
> > > > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > > > > ---
> > > > > > Ref:
> > > > > >
> > > > > >
> > > > > > v12->v13:
> > > > > > * Moved RZ_MTU3_TMDR1_* macros from pwm driver to rz-mtu3.h.
> > > > > > v11->v2:
> > > > > > * Moved the core driver from timer to MFD.
> > > > > > * Moved header fine from
> > > > > > clocksource/rz-mtu3.h->linux/mfd/rz-mtu3.h
> > > > > > * Removed Select MFD_CORE option from config.
> > > > > > v10->v11:
> > > > > > * No change.
> > > > > > v9->v10:
> > > > > > * No change.
> > > > > > v8->v9:
> > > > > > * No change.
> > > > > > v7->v8:
> > > > > > * Add locking for RMW on rz_mtu3_shared_reg_update_bit()
> > > > > > * Replaced enum rz_mtu3_functions with channel busy flag
> > > > > > * Added API for request and release a channel.
> > > > > > v6->v7:
> > > > > > * Added channel specific mutex to avoid races between child
> devices
> > > > > > (for eg: pwm and counter)
> > > > > > * Added rz_mtu3_shared_reg_update_bit() to update bit.
> > > > > > v5->v6:
> > > > > > * Updated commit and KConfig description
> > > > > > * Selected MFD_CORE to avoid build error if CONFIG_MFD_CORE not
> set.
> > > > > > * Improved error handling in probe().
> > > > > > * Updated MODULE_DESCRIPTION and title.
> > > > > > v4->v5:
> > > > > > * Moved core driver from MFD to timer
> > > > > > * Child devices instatiated using mfd_add_devices()
> > > > > > v3->v4:
> > > > > > * A single driver that registers both the counter and the pwm
> > > > > functionalities
> > > > > > that binds against "renesas,rz-mtu3".
> > > > > > * Moved PM handling from child devices to here.
> > > > > > * replaced include/linux/mfd/rz-mtu3.h->drivers/mfd/rz-mtu3.h
> > > > > > * Removed "remove" callback
> > > > > > v2->v3:
> > > > > > * removed unwanted header files
> > > > > > * Added LUT for 32 bit registers as it needed for 32-bit
> > > > > > cascade
> > > > > counting.
> > > > > > * Exported 32 bit read/write functions.
> > > > > > v1->v2:
> > > > > > * Changed the compatible name
> > > > > > * Replaced
> > > > > > devm_reset_control_get->devm_reset_control_get_exclusive
> > > > > > * Renamed function names rzg2l_mtu3->rz_mtu3 as this is generic
> IP
> > > > > > in RZ family SoC's.
> > > > > > ---
> > > > > > drivers/mfd/Kconfig | 10 +
> > > > > > drivers/mfd/Makefile | 1 +
> > > > > > drivers/mfd/rz-mtu3.c | 458
> > > ++++++++++++++++++++++++++++++++++++
> > > > > > include/linux/mfd/rz-mtu3.h | 243 +++++++++++++++++++
> > > > > > 4 files changed, 712 insertions(+) create mode 100644
> > > > > > drivers/mfd/rz-mtu3.c create mode 100644
> > > > > > include/linux/mfd/rz-mtu3.h
> > >
> > > [...]
> > >
> > > > > > diff --git a/include/linux/mfd/rz-mtu3.h
> > > > > > b/include/linux/mfd/rz-mtu3.h new file mode 100644 index
> > > > > > 000000000000..42e561a9603c
> > > > > > --- /dev/null
> > > > > > +++ b/include/linux/mfd/rz-mtu3.h
> > > > > > @@ -0,0 +1,243 @@
> > > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > > +/*
> > > > > > + * Copyright (C) 2022 Renesas Electronics Corporation */
> > > > > > +#ifndef __LINUX_RZ_MTU3_H__ #define __LINUX_RZ_MTU3_H__
> > > > >
> > > > > __MFD_RZ_MTU3_H__
> > > >
> > > > OK.
> > > >
> > > > >
> > > > > > +#include <linux/clk.h>
> > > > >
> > > > > What about all the others?
> > > >
> > > > It is not required here. Will remove it.
> > >
> > > It is required. Please explicitly include all the headers you use here.
> >
> > OK will add others as well.
> >
> > >
> > > > > +#if IS_ENABLED(CONFIG_RZ_MTU3)
> > > > > > +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;
> > > > >
> > > > > Perhaps I'd reading this all wrong, but ...
> > > > >
> > > > > What are you trying to do here?
> > > >
> > > > It is to avoid race between counter and pwm to acquiring the same
> channel.
> > > > If a channel is free, then only they can access the channel.
> > > >
> > > > Please let me know if any clarifications needed, or correct me if
> > > > anything
> > > wrong.
> > >
> > > I mean the logic. Please explain it to me.
> >
> > lock()
> > get the idle state of a channel
> > if(idle state)
> > make the channel to busy
> > unlock
> >
> > return the idle state;
> >
> > >
> > > For instance, why not just do:
> > >
> > > bool success = false
> > >
> > > lock()
> > >
> > > if (!is_busy)
> > > is_busy = true
> > > success = true
> > >
> > > unlock()
> > >
> > > return success
> > >
> > > What do you think? Easier to brain parse?
> >
> > Yes, I am ok with your suggestion as well, eventhough there is
> > 3 assignment statements compared to 2 in previous logic, as it is
> > easier to read.
>
> Perhaps this ticks all the boxes:
>
> lock()
>
> if (is_busy)
> unlock()
> return false
>
> is_busy = true;
>
> unlock()
>
> return true
OK, Will do this change in next version.
Cheers,
Biju
