Re: [PATCH v2 1/3] mfd: rz-mtu3: Do not use struct rz_mtu3_channel::dev

[Lee Jones]

--- checkpatch.pl: clean (0 issues) ---

On Sun, 04 Oct 2026, Cosmin Tanislav wrote:

Please review, explain or fix the Sashiko review(s).

> Channels 1 and 2 are used by both the PWM and counter drivers. Both
> subdrivers assign struct rz_mtu3_channel::dev to their own device
> instance for those shared channels, so the value stored in it depends on
> which subdriver binds last.
>
> The MFD core helpers currently use ch->dev->parent to retrieve the
> parent driver data, which makes them depend on subdriver-populated
> state.
>
> This is fragile for shared channels and unnecessarily couples the core
> to child driver initialization.
>
> Add a priv pointer to struct rz_mtu3_channel, populated by the MFD
> driver at probe time, and use it directly in place of ch->dev->parent
> in every MFD core helper, so the core no longer depends on
> subdriver-assigned state. Remove struct rz_mtu3::priv_data, which
> becomes redundant now that every channel carries its own priv pointer.
>
> Keep struct rz_mtu3_channel::dev for now, as it is still used by the PWM
> and counter drivers.
>
> Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@xxxxxxxxxxx>
> ---
>
> V2:
>  * capitalize subject after prefix
>  * switch struct rz_mtu3_priv retrieval to a back-pointer stored in
>    struct rz_mtu3_channel::priv
>  * remove struct rz_mtu3::priv_data as it is unused now
>
>  drivers/mfd/rz-mtu3.c       | 40 +++++++++++++------------------------
>  include/linux/mfd/rz-mtu3.h |  7 ++++---
>  2 files changed, 18 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/mfd/rz-mtu3.c b/drivers/mfd/rz-mtu3.c
> index 0a254e61ec0a..0fca16aaa566 100644
> --- a/drivers/mfd/rz-mtu3.c
> +++ b/drivers/mfd/rz-mtu3.c
> @@ -62,8 +62,7 @@ static bool rz_mtu3_is_16bit_shared_reg(u16 offset)
>
>  u16 rz_mtu3_shared_reg_read(struct rz_mtu3_channel *ch, u16 offset)
>  {
> -	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> -	struct rz_mtu3_priv *priv = mtu->priv_data;
> +	struct rz_mtu3_priv *priv = ch->priv;
>
>  	if (rz_mtu3_is_16bit_shared_reg(offset))
>  		return readw(priv->mmio + offset);
> @@ -74,8 +73,7 @@ EXPORT_SYMBOL_GPL(rz_mtu3_shared_reg_read);
>
>  u8 rz_mtu3_8bit_ch_read(struct rz_mtu3_channel *ch, u16 offset)
>  {
> -	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> -	struct rz_mtu3_priv *priv = mtu->priv_data;
> +	struct rz_mtu3_priv *priv = ch->priv;
>  	u16 ch_offs;
>
>  	ch_offs = rz_mtu3_8bit_ch_reg_offs[ch->channel_number][offset];
> @@ -86,8 +84,7 @@ EXPORT_SYMBOL_GPL(rz_mtu3_8bit_ch_read);
>
>  u16 rz_mtu3_16bit_ch_read(struct rz_mtu3_channel *ch, u16 offset)
>  {
> -	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> -	struct rz_mtu3_priv *priv = mtu->priv_data;
> +	struct rz_mtu3_priv *priv = ch->priv;
>  	u16 ch_offs;
>
>  	/* MTU8 doesn't have 16-bit registers */
> @@ -102,8 +99,7 @@ EXPORT_SYMBOL_GPL(rz_mtu3_16bit_ch_read);
>
>  u32 rz_mtu3_32bit_ch_read(struct rz_mtu3_channel *ch, u16 offset)
>  {
> -	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> -	struct rz_mtu3_priv *priv = mtu->priv_data;
> +	struct rz_mtu3_priv *priv = ch->priv;
>  	u16 ch_offs;
>
>  	if (ch->channel_number != RZ_MTU3_CHAN_1 && ch->channel_number != RZ_MTU3_CHAN_8)
> @@ -117,8 +113,7 @@ EXPORT_SYMBOL_GPL(rz_mtu3_32bit_ch_read);
>
>  void rz_mtu3_8bit_ch_write(struct rz_mtu3_channel *ch, u16 offset, u8 val)
>  {
> -	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> -	struct rz_mtu3_priv *priv = mtu->priv_data;
> +	struct rz_mtu3_priv *priv = ch->priv;
>  	u16 ch_offs;
>
>  	ch_offs = rz_mtu3_8bit_ch_reg_offs[ch->channel_number][offset];
> @@ -128,8 +123,7 @@ EXPORT_SYMBOL_GPL(rz_mtu3_8bit_ch_write);
>
>  void rz_mtu3_16bit_ch_write(struct rz_mtu3_channel *ch, u16 offset, u16 val)
>  {
> -	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> -	struct rz_mtu3_priv *priv = mtu->priv_data;
> +	struct rz_mtu3_priv *priv = ch->priv;
>  	u16 ch_offs;
>
>  	/* MTU8 doesn't have 16-bit registers */
> @@ -143,8 +137,7 @@ EXPORT_SYMBOL_GPL(rz_mtu3_16bit_ch_write);
>
>  void rz_mtu3_32bit_ch_write(struct rz_mtu3_channel *ch, u16 offset, u32 val)
>  {
> -	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> -	struct rz_mtu3_priv *priv = mtu->priv_data;
> +	struct rz_mtu3_priv *priv = ch->priv;
>  	u16 ch_offs;
>
>  	if (ch->channel_number != RZ_MTU3_CHAN_1 && ch->channel_number != RZ_MTU3_CHAN_8)
> @@ -157,8 +150,7 @@ EXPORT_SYMBOL_GPL(rz_mtu3_32bit_ch_write);
>
>  void rz_mtu3_shared_reg_write(struct rz_mtu3_channel *ch, u16 offset, u16 value)
>  {
> -	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> -	struct rz_mtu3_priv *priv = mtu->priv_data;
> +	struct rz_mtu3_priv *priv = ch->priv;
>
>  	if (rz_mtu3_is_16bit_shared_reg(offset))
>  		writew(value, priv->mmio + offset);
> @@ -170,8 +162,7 @@ EXPORT_SYMBOL_GPL(rz_mtu3_shared_reg_write);
>  void rz_mtu3_shared_reg_update_bit(struct rz_mtu3_channel *ch, u16 offset,
>  				   u16 pos, u8 val)
>  {
> -	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> -	struct rz_mtu3_priv *priv = mtu->priv_data;
> +	struct rz_mtu3_priv *priv = ch->priv;
>  	unsigned long tmdr, flags;
>
>  	spin_lock_irqsave(&priv->lock, flags);
> @@ -244,8 +235,7 @@ static u8 rz_mtu3_get_tstr_bit_pos(struct rz_mtu3_channel *ch)
>
>  static void rz_mtu3_start_stop_ch(struct rz_mtu3_channel *ch, bool start)
>  {
> -	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> -	struct rz_mtu3_priv *priv = mtu->priv_data;
> +	struct rz_mtu3_priv *priv = ch->priv;
>  	unsigned long flags, tstr;
>  	u16 offset;
>  	u8 bitpos;
> @@ -265,8 +255,7 @@ static void rz_mtu3_start_stop_ch(struct rz_mtu3_channel *ch, bool start)
>
>  bool rz_mtu3_is_enabled(struct rz_mtu3_channel *ch)
>  {
> -	struct rz_mtu3 *mtu = dev_get_drvdata(ch->dev->parent);
> -	struct rz_mtu3_priv *priv = mtu->priv_data;
> +	struct rz_mtu3_priv *priv = ch->priv;
>  	unsigned long flags, tstr;
>  	u16 offset;
>  	u8 bitpos;
> @@ -320,12 +309,10 @@ static int rz_mtu3_probe(struct platform_device *pdev)
>  	if (!ddata)
>  		return -ENOMEM;
>
> -	ddata->priv_data = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> -	if (!ddata->priv_data)
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
>  		return -ENOMEM;
>
> -	priv = ddata->priv_data;
> -
>  	priv->mmio = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(priv->mmio))
>  		return PTR_ERR(priv->mmio);
> @@ -344,6 +331,7 @@ static int rz_mtu3_probe(struct platform_device *pdev)
>  	for (i = 0; i < RZ_MTU_NUM_CHANNELS; i++) {
>  		ddata->channels[i].channel_number = i;
>  		ddata->channels[i].is_busy = false;
> +		ddata->channels[i].priv = priv;
>  		mutex_init(&ddata->channels[i].lock);
>  	}
>
> diff --git a/include/linux/mfd/rz-mtu3.h b/include/linux/mfd/rz-mtu3.h
> index 8421d49500bf..3884ae5b008c 100644
> --- a/include/linux/mfd/rz-mtu3.h
> +++ b/include/linux/mfd/rz-mtu3.h
> @@ -122,6 +122,8 @@ enum rz_mtu3_channels {
>  	RZ_MTU_NUM_CHANNELS
>  };
>
> +struct rz_mtu3_priv;
> +
>  /**
>   * struct rz_mtu3_channel - MTU3 channel private data
>   *
> @@ -129,12 +131,14 @@ enum rz_mtu3_channels {
>   * @channel_number: channel number
>   * @lock: Lock to protect channel state
>   * @is_busy: channel state
> + * @priv: MFD private data
>   */
>  struct rz_mtu3_channel {
>  	struct device *dev;
>  	unsigned int channel_number;
>  	struct mutex lock;
>  	bool is_busy;
> +	struct rz_mtu3_priv *priv;
>  };
>
>  /**
> @@ -142,13 +146,10 @@ struct rz_mtu3_channel {
>   *
>   * @clk: MTU3 module clock
>   * @rz_mtu3_channel: HW channels
> - * @priv_data: MTU3 core driver private data
>   */
>  struct rz_mtu3 {
>  	struct clk *clk;
>  	struct rz_mtu3_channel channels[RZ_MTU_NUM_CHANNELS];
> -
> -	void *priv_data;
>  };
>
>  static inline bool rz_mtu3_request_channel(struct rz_mtu3_channel *ch)
> -- 
> 2.55.0

-- 
Lee Jones