Re: [PATCH v3 2/3] i3c: master: Add Qualcomm I3C controller driver

[Mukesh Kumar Savaliya]

Hi Krzysztof,

On 4/9/2025 12:11 AM, Krzysztof Kozlowski wrote:
> On 08/04/2025 15:23, Mukesh Kumar Savaliya wrote:
> > > > +
> > > > +static int i3c_geni_runtime_get_mutex_lock(struct geni_i3c_dev *gi3c)
> > > > +{
> > >
> > > You miss sparse/lockdep annotations.
> > This is called in pair only, but to avoid repeated code in caller
> > functions, we have designed this wrapper.
> > i3c_geni_runtime_get_mutex_lock()
> > i3c_geni_runtime_put_mutex_unlock().
> >
> > caller function maintains the parity. e.g. geni_i3c_master_priv_xfers().
> >
> > Does a comment help here ? Then i can write up to add.
>
> I do not see how this is relevant to my comment at all.
What i understood is you suspect about lock/unlock imbalance right ?
I know that Lockdep annotations will be used to check if locks are 
acquired and released in a proper order.

You want me to add below code in both the functions mentioned ?
    lockdep_assert_held(&gi3c->lock);

What exact sparse/attribute can be added ? I am not sure about that.
> > > > +	int ret;
> > > > +
> > > > +	mutex_lock(&gi3c->lock);
> > > > +	reinit_completion(&gi3c->done);
> > > > +	ret = pm_runtime_get_sync(gi3c->se.dev);
> > > > +	if (ret < 0) {
> > > > +		dev_err(gi3c->se.dev, "error turning on SE resources:%d\n", ret);
> > > > +		pm_runtime_put_noidle(gi3c->se.dev);
> > > > +		/* Set device in suspended since resume failed */
> > > > +		pm_runtime_set_suspended(gi3c->se.dev);
> > > > +		mutex_unlock(&gi3c->lock);
> > >
> > > Either you lock or don't lock, don't mix these up.
> > Caller is taking care of not calling i3c_geni_runtime_put_mutex_unlock()
> > if this failed.
>
>
> I do not see how this is relevant to my comment at all.
same as above
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void i3c_geni_runtime_put_mutex_unlock(struct geni_i3c_dev *gi3c)
> > > > +{
> > >
> > > Missing annotations.
> > Shall i add a comment here ?
>
> Do you understand what is sparse? And lockdep?
Little but not clear on exact sparse attribute to be added. please help 
me. if you can help with some clear comment and sample, will be easier 
if you can.

Lockdep : Helps with runtime check if lock, unlock are proper.
sparse: attributes /special comments to add which helps with static 
analysis which is not done by the compiler.
> > > > +	pm_runtime_mark_last_busy(gi3c->se.dev);
> > > > +	pm_runtime_put_autosuspend(gi3c->se.dev);
> > > > +	mutex_unlock(&gi3c->lock);
> > > > +}
> > > > +
> > > > +static void geni_i3c_abort_xfer(struct geni_i3c_dev *gi3c)
> > > > +{
> > > > +	unsigned long time_remaining;
> > > > +	unsigned long flags;
> > > > +
> > > > +	reinit_completion(&gi3c->done);
> > > > +	spin_lock_irqsave(&gi3c->irq_lock, flags);
> > > > +	geni_i3c_handle_err(gi3c, GENI_TIMEOUT);
> > > > +	geni_se_abort_m_cmd(&gi3c->se);
> > > > +	spin_unlock_irqrestore(&gi3c->irq_lock, flags);
> > > > +	time_remaining = wait_for_completion_timeout(&gi3c->done, XFER_TIMEOUT);
> > > > +	if (!time_remaining)
> > > > +		dev_err(gi3c->se.dev, "Timeout abort_m_cmd\n");
> > > > +}
> > >
> > > ...
> > >
> > > > +
> > > > +static int i3c_geni_resources_init(struct geni_i3c_dev *gi3c, struct platform_device *pdev)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	gi3c->se.base = devm_platform_ioremap_resource(pdev, 0);
> > > > +	if (IS_ERR(gi3c->se.base))
> > > > +		return PTR_ERR(gi3c->se.base);
> > > > +
> > > > +	gi3c->se.clk = devm_clk_get(&pdev->dev, "se");
> > > > +	if (IS_ERR(gi3c->se.clk))
> > > > +		return dev_err_probe(&pdev->dev, PTR_ERR(gi3c->se.clk),
> > > > +							"Unable to get serial engine core clock: %pe\n",
> > > > +							gi3c->se.clk);
> > >
> > > Totally messed indentation.
> > yes, corrected.
> > > > +	ret = geni_icc_get(&gi3c->se, NULL);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/* Set the bus quota to a reasonable value for register access */
> > > > +	gi3c->se.icc_paths[GENI_TO_CORE].avg_bw = GENI_DEFAULT_BW;
> > > > +	gi3c->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW;
> > > > +	ret = geni_icc_set_bw(&gi3c->se);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/* Default source clock (se-clock-frequency) freq is 100Mhz */
> > > > +	gi3c->clk_src_freq = KHZ(100000);
> > >
> > > And why can't you use clk_get_rate()?
> > During probe(), we need one time initialization of source clock
> > frequencey. HW has no clock set before this.
>
> How is it possible that there is no clock or clock was not configured
> but you need to know it? Anyway, it's tiring to keep discussing this.
>
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int geni_i3c_probe(struct platform_device *pdev)
> > > > +{
> > > > +	u32 proto, tx_depth, fifo_disable;
> > > > +	struct geni_i3c_dev *gi3c;
> > >
> > > Just store pdev->dev in local dev variable, to simplify everything here.
> > yes, thats right. But i see other drivers are using same pdev->dev. Is
> > it fine ? if really required, will change it.
>
> Are you going to discuss every little comment? And come with arguments
> like "I found poor code, so I am allowed to do the same"?
>
> Best regards,
> Krzysztof