linux-kernel - Re: [PATCHv3] mfd: cpcap: Add minimal support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20170106000823.GB2630@atomide.com>
Date:   Thu, 5 Jan 2017 16:08:23 -0800
From:   Tony Lindgren <tony@...mide.com>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     Samuel Ortiz <sameo@...ux.intel.com>, linux-kernel@...r.kernel.org,
        linux-omap@...r.kernel.org, devicetree@...r.kernel.org,
        Marcel Partap <mpartap@....net>,
        Mark Rutland <mark.rutland@....com>,
        Michael Scott <michael.scott@...aro.org>
Subject: Re: [PATCHv3] mfd: cpcap: Add minimal support

* Lee Jones <lee.jones@...aro.org> [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@...mide.com>.
> 
> Actually, it's more traditional to place the CR at the top.

Sure. Will repost v4 shortly.

Thanks,

Tony