Re: [PATCHv3] mfd: cpcap: Add minimal support

[Tony Lindgren]

* Lee Jones <lee.jones@xxxxxxxxxx> [170103 08:32]:
> On Mon, 05 Dec 2016, Tony Lindgren wrote:
> > +static int cpcap_init_irq_chip(struct cpcap_ddata *cpcap, int irq_chip,
> > +			       int irq_start, int nr_irqs)
> > +{
> > +	struct regmap_irq_chip *chip = &cpcap_irq_chip[irq_chip];
> > +	int i, ret;
> > +
> > +	for (i = irq_start; i < irq_start + nr_irqs; i++) {
> > +		struct regmap_irq *cpcap_irq = &cpcap->irqs[i];
> > +
> > +		cpcap_irq->reg_offset =
> > +			((i - irq_start) / cpcap->regmap_conf->val_bits) *
> > +			cpcap->regmap_conf->reg_stride;
> > +		cpcap_irq->mask = BIT(i % cpcap->regmap_conf->val_bits);
> 
> If a code segment takes more than a few seconds to work out, it's
> either too cluttered (break it down), too complex (simplify it) or if
> neither of the first two approaches are possible, then a comment is
> required.

Hmm yeah I could not read that myself any longer :) Will split.

> > +static int cpcap_probe(struct spi_device *spi)
> > +{
> > +	const struct of_device_id *match;
> > +	int ret = -EINVAL;
> 
> What is the purpose of initialising ret?

Seems to be not needed.

> > +	struct cpcap_ddata *cpcap;
> > +
> > +	match = of_match_device(of_match_ptr(cpcap_of_match), &spi->dev);
> > +	if (!match)
> > +		return -ENODEV;
> > +
> > +	cpcap = devm_kzalloc(&spi->dev, sizeof(*cpcap), GFP_KERNEL);
> > +	if (!cpcap)
> > +		return -ENOMEM;
> > +
> > +	cpcap->spi = spi;
> > +	spi_set_drvdata(spi, cpcap);
> > +
> > +	spi->bits_per_word = 16;
> > +	spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
> > +
> > +	ret = spi_setup(spi);
> > +	if (ret < 0)
> > +		return ret;
> 
> Is it possible for spi_setup() to return >0?
> 
> If not, then "if (!ret)" will do.

You're right, the kerneldoc for spi_setup() says:

"Return: zero on success, else a negative error code."

Need to use "if (ret)" to return on error though, not "if (!ret)".

> > +static struct spi_driver cpcap_driver = {
> > +	.driver = {
> > +		.name = "cpcap-core",
> > +		.of_match_table = cpcap_of_match,
> 
> of_match_ptr() ?

Well there needs to be a platform specific init added at least in some
cases. I have not quite yet deciphered what all it does for droid 4
as it's already done for kexec based booting.

> > + * Copyright (C) 2016 Tony Lindgren <tony@xxxxxxxxxxx>.
> 
> Actually, it's more traditional to place the CR at the top.

Sure. Will repost v4 shortly.

Thanks,

Tony