Re: [PATCH v2 3/7] soc: qcom: Add GENI based QUP Wrapper driver

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Wed 31 Jan 10:58 PST 2018, Karthik Ramasubramanian wrote:
> On 1/16/2018 11:20 PM, Bjorn Andersson wrote:
> > On Fri 12 Jan 17:05 PST 2018, Karthikeyan Ramasubramanian wrote:
[..]
> > I'm not aware of any non-serial-engine "geni" at Qualcomm, so can we
> > drop the "se" throughout this driver?
> Currently GENI is used to program just the serial engines. But it is not
> restricted to that. It can be used to program other hardware cores. Hence
> keeping "se" will help to clarify that this driver is meant for GENI based
> serial engines only.

Okay, fair enough.

[..]
> > > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
[..]
> > > +	geni_cgc_ctrl = geni_read_reg(base, GENI_CGC_CTRL);
> > > +	dma_general_cfg = geni_read_reg(base, SE_DMA_GENERAL_CFG);
> > > +	geni_cgc_ctrl |= DEFAULT_CGC_EN;
> > > +	dma_general_cfg |= (AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CFG_ON |
> > > +			DMA_TX_CLK_CGC_ON | DMA_RX_CLK_CGC_ON);
> > 
> > Drop the parenthesis and there's no harm in making multiple assignments
> > in favour of splitting the line.
> Ok.
> > 
> > > +	io_op_ctrl = DEFAULT_IO_OUTPUT_CTRL_MSK;
> > > +	geni_write_reg(geni_cgc_ctrl, base, GENI_CGC_CTRL);
> > > +	geni_write_reg(dma_general_cfg, base, SE_DMA_GENERAL_CFG);
> > 
> > Is there a reason why this chunk of code is a mix of 3 independent
> > register updates?
> I am not sure I understand the context of your question. This is how the
> hardware programming manual is describing to program the registers as part
> of initializing a serial engine. Please let me know if this is not the
> information you are looking for.

Can you please double check with the hardware guys if it really is
required that you do:

a = read(A)
b = read(B)

modify a
modify b
assign c

write(a)
write(b)
write(c)

And if that is the case, then try to make things as easy to read as
possible - e.g. by inlining the value of "c" and adding an empty line
between reads, modifications and writes as I did here.

Regards,
Bjorn