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

[Lee Jones]

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