Re: [PATCH v13 2/6] mfd: Add Renesas RZ/G2L MTU3a core driver

[Lee Jones <lee@kernel.org>]

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@bp.renesas.com>
> > > ---
> > > 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.

[...]

 > > +#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.

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?

> > > +	mutex_unlock(&ch->lock);
> > > +
> > > +	return is_idle;
> > > +}

--
Lee Jones [李琼斯]