linux-kernel - Re: [RESEND PATCH v2 1/2] mfd: da9063: Fix revision handling to correctly select reg tables

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20200420073155.GK3737@dell>
Date:   Mon, 20 Apr 2020 08:31:55 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Adam Thomson <Adam.Thomson.Opensource@...semi.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Support Opensource <Support.Opensource@...semi.com>
Subject: Re: [RESEND PATCH v2 1/2] mfd: da9063: Fix revision handling to
 correctly select reg tables

On Fri, 17 Apr 2020, Adam Thomson wrote:

> On 17 April 2020 10:24, Lee Jones wrote:
> 
> > > > > +		return -EINVAL;
> > > >
> > > > Do you want to fail silently here?
> > >
> > > Well an error message is printed in the calling code, so didn't feel like it
> > > was necessary to have additional debug here. Felt like bloat.
> > 
> > As a user, I would prefer a more specific reason.
> > 
> > Thus, I would provide an error message here and omit the generic one.
> 
> I can update although I'll of course then need to do similar messages for the
> other error legs of this function. FWIW, as this is only being called once in
> the same file this error leg of code currently can never occur.

As a tiny improvement, it's not a deal breaker.  If it's too much
work, you can either submit a subsequent patch or omit it completely.

> > > > > +}
> > > > > +
> > > > > +enum {
> > > > > +	DA9063_DEV_ID_REG = 0,
> > > > > +	DA9063_VAR_ID_REG,
> > > > > +	DA9063_CHIP_ID_REGS,
> > > > > +};
> > > > > +
> > > > > +static int da9063_get_device_type(struct i2c_client *i2c, struct da9063
> > > > *da9063)
> > > > > +{
> > > > > +	int ret;
> > > > > +	u8 buf[DA9063_CHIP_ID_REGS];
> > > >
> > > > Really small nit: Could you reverse these please.
> > >
> > > Yep, agreed.
> > >
> > > >
> > > > > +	ret = da9063_i2c_blockreg_read(i2c, DA9063_REG_DEVICE_ID, buf,
> > > > > +				       DA9063_CHIP_ID_REGS);
> > > > > +	if (ret < 0) {
> > > >
> > > > if (ret)
> > > >
> > > > Or better yet, as this is a read function, you could just return
> > > > i2c_transfer() and do the appropriate error checking here *instead*.
> > >
> > > I think given that the function handles all of the I2C specific stuff I'd prefer
> > > it be kept there. Logically that to me makes more sense. Can change this to
> > > 'if (ret)'
> > 
> > Yes, not that I understand the message length (3) has more do to with
> > the I2C interactions (rather than a derisive of 'count'), it makes
> > sense to handle that inside the function.
> > 
> > However, it does seem odd to handle the return value of a *_read()
> > function in this way.  They usually return the number of bytes read,
> > which in this case would be DA9063_CHIP_ID_REGS (count), right?
> 
> Well regmap_bulk_read and regmap_read return 0 for success and negative for
> failure so I'd disagree on this part.

Fair enough. :)

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog