Re: [PATCH 1/6] soc: qcom: gsbi: Add support for ADM CRCI muxing

[Andy Gross <agross@codeaurora.org>]

On Wed, Jan 28, 2015 at 06:11:50PM -0800, Stephen Boyd wrote:

<snip>

> >  Required properties:
> > -- compatible: must contain "qcom,gsbi-v1.0.0" for APQ8064/IPQ8064
> > +- compatible:	Should contain:
> > +		"qcom,gsbi-ipq8064" for IPQ8064
> > +		"qcom,gsbi-apq8064" for APQ8064
> > +		"qcom,gsbi-msm8960" for MSM8960
> > +		"qcom,gsbi-msm8660" for MSM8660
> 
> Hopefully this is not necessary, but if it is we should leave the
> old compatible here and say it's deprecated or something.

Right.  I went back and forth with the tcsr vs gsbi.  If change the compats I'll
put in a deprecated.

> >  - reg: Address range for GSBI registers
> >  - clocks: required clock
> >  - clock-names: must contain "iface" entry
> >  - qcom,mode : indicates MUX value for configuration of the serial interface.
> >    Please reference dt-bindings/soc/qcom,gsbi.h for valid mux values.
> > +- qcom,gsbi-num: indicates GSBI instance number
> 
> Why not use DT aliases for this? Then other drivers or more
> generic code can search for a gsbiN alias for the particular gsbi
> node. No qcom specific property.

Yeah thats cleaner.  I'll do that.

> 
> > +- syscon-tcsr: indicates phandle of TCSR syscon node
> 
> Make this optional but required if any child nodes use dma?

To enforce that I'd have to determine that a child has a dmas.  I guess that
isn't so bad.

<snip>

> >  static int gsbi_probe(struct platform_device *pdev)
> >  {
> >  	struct device_node *node = pdev->dev.of_node;
> > +	const struct of_device_id *match;
> >  	struct resource *res;
> >  	void __iomem *base;
> >  	struct gsbi_info *gsbi;
> > +	u32 gsbi_num, i, val;
> 
> i should be int
> 
> > +	struct crci_config *config;
> 
> const?

will fix both.

> >  
> >  	gsbi = devm_kzalloc(&pdev->dev, sizeof(*gsbi), GFP_KERNEL);
> >  
> > @@ -45,6 +152,20 @@ static int gsbi_probe(struct platform_device *pdev)
> >  	if (IS_ERR(base))
> >  		return PTR_ERR(base);
> >  
> > +	gsbi->tcsr = syscon_regmap_lookup_by_phandle(node, "syscon-tcsr");
> > +	if (IS_ERR(gsbi->tcsr))
> > +		return -EINVAL;
> > +
> > +	if (of_property_read_u32(node, "qcom,gsbi-num", &gsbi_num)) {
> > +		dev_err(&pdev->dev, "missing gsbi instance number\n");
> > +		return -EINVAL;
> > +	}
> 
> As said before, aliases would do the job the same and not require
> some qcom specific property.

Yup. will fix.

> > +
> > +	if (!gsbi_num || gsbi_num > MAX_GSBI) {
> > +		dev_err(&pdev->dev, "invalid gsbi number\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	if (of_property_read_u32(node, "qcom,mode", &gsbi->mode)) {
> >  		dev_err(&pdev->dev, "missing mode configuration\n");
> >  		return -EINVAL;
> > @@ -64,6 +185,26 @@ static int gsbi_probe(struct platform_device *pdev)
> >  	writel_relaxed((gsbi->mode << GSBI_PROTOCOL_SHIFT) | gsbi->crci,
> >  				base + GSBI_CTRL_REG);
> >  
> > +	/*
> > +	 * modify tcsr to reflect mode and ADM CRCI mux
> > +	 * Each gsbi contains a pair of bits, one for RX and one for TX
> > +	 * SPI mode requires both bits cleared, otherwise they are set
> > +	 */
> > +	match = of_match_node(gsbi_dt_match, node);
> 
> Why not match the config to the TCSR compatible string? Wouldn't
> that more accurately reflect that we need to set different bits
> depending on which type of TCSR we're using? The version of GSBI
> hardware is not actually changing in every different SoC so I
> don't see why we want to change the compatible there just because
> the TCSR register layout changed.

That is true.  However, with the gsbi compat, I avoid doing a match multiple
times and get the table I need immediately.  The alternative is N checks or
pulling the compat strings and comparing them to get the right table.

> > +	config = (struct crci_config *)match->data;
> 
> Cast shouldn't be necessary if config is const?

will check if that works

> > +
> > +	if (config)
> > +		for (i = 0; i < config->num_rows; i++) {
> > +			if (gsbi->mode == GSBI_PROT_SPI)
> 
> Doesn't I2C need the same treatment (anything in QUP really)?
> Maybe the logic could be changed to check for gsbi->crci ==
> GSBI_CRCI_QUP?

Nope.  I2C doesn't support DMA when ADM is the controller.  It's only SPI or
UART.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project