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

[Lee Jones <lee.jones@linaro.org>]

On Thu, 16 Apr 2020, Adam Thomson wrote:

> On 16 April 2020 09:00, Lee Jones wrote:
> 
> > > +/*
> > > + * Raw I2C access required for just accessing chip and variant info before we
> > > + * know which device is present. The info read from the device using this
> > > + * approach is then used to select the correct regmap tables.
> > > + */
> > > +static int da9063_i2c_blockreg_read(struct i2c_client *client, u16 addr,
> > > +				    u8 *buf, int count)
> > > +{
> > > +	struct i2c_msg xfer[3];
> > > +	u8 page_num, paged_addr;
> > > +	u8 page_buf[2];
> > > +	int ret;
> > > +
> > > +	/* Determine page info based on register address */
> > > +	page_num = (addr / 0x100);
> > 
> > Please define magic numbers.
> > 
> > > +	if (page_num > 1)
> > 
> > Please define magic numbers.
> 
> I was going to but decided against it given the minimal use. Easy enough to
> change though.

It's purely for readability purposes.

> > > +		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.

> > > +	paged_addr = (addr % 0x100) & 0xFF;
> > > +	page_buf[0] = DA9063_REG_PAGE_CON;
> > > +	page_buf[1] = (page_num << DA9063_I2C_PAGE_SEL_SHIFT) &
> > > +		      DA9063_REG_PAGE_MASK;
> > > +
> > > +	/* Write reg address, page selection */
> > > +	xfer[0].addr = client->addr;
> > > +	xfer[0].flags = 0;
> > > +	xfer[0].len = 2;
> > > +	xfer[0].buf = page_buf;
> > > +
> > > +	/* Select register address */
> > > +	xfer[1].addr = client->addr;
> > > +	xfer[1].flags = 0;
> > > +	xfer[1].len = 1;
> > > +	xfer[1].buf = &paged_addr;
> > > +
> > > +	/* Read data */
> > > +	xfer[2].addr = client->addr;
> > > +	xfer[2].flags = I2C_M_RD;
> > > +	xfer[2].len = count;
> > > +	xfer[2].buf = buf;
> > > +
> > > +	ret = i2c_transfer(client->adapter, xfer, 3);
> > 
> > Why is this 3?  'count' and a NULL char?
> 
> Well there are 3 messages defined above so I want to process all of them. One to
> set the page register to the page we want to read from, one to select the
> register we want to read from in that page and then finally the read back of 
> the chip id and revision/variant info.

I see.  Thank you for the explanation.

> > > +	if (ret == 3)
> > > +		return 0;
> > > +	else if (ret < 0)
> > > +		return ret;
> > > +	else
> > > +		return -EIO;
> > 
> > I think the following makes it slightly clearer.
> > 
> > 	if (ret < 0)
> > 		return ret;
> > 
> > 	if (ret == 3)
> > 		return 0;
> > 	else
> > 		return -EIO;
> >
> 
> Ok. Don't think it makes much of a difference but don't mind really. I can add a
> #define for the number of messages to be sent which will clarify this slightly
> anyway.

Yes, I think that would be good.

> > > +}
> > > +
> > > +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?

[...]

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