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

[Adam Thomson]

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.

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

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

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

> > +}
> > +
> > +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)'

> 
> > +		dev_err(da9063->dev, "Cannot read chip id info.\n");
> > +		return ret;
> > +	}
> > +
> > +	if (buf[DA9063_DEV_ID_REG] != PMIC_CHIP_ID_DA9063) {
> > +		dev_err(da9063->dev,
> > +			"Invalid chip device id: 0x%02x\n",
> 
> s/id/ID/

yep, will update.

> > @@ -199,18 +289,56 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
> >  	da9063->chip_irq = i2c->irq;
> >  	da9063->type = id->driver_data;
> >
> > -	if (da9063->variant_code == PMIC_DA9063_AD) {
> > -		da9063_regmap_config.rd_table = &da9063_ad_readable_table;
> > -		da9063_regmap_config.wr_table =
> &da9063_ad_writeable_table;
> > -		da9063_regmap_config.volatile_table =
> &da9063_ad_volatile_table;
> > -	} else if (da9063->type == PMIC_TYPE_DA9063L) {
> > -		da9063_regmap_config.rd_table = &da9063l_bb_readable_table;
> > -		da9063_regmap_config.wr_table =
> &da9063l_bb_writeable_table;
> > -		da9063_regmap_config.volatile_table =
> &da9063l_bb_volatile_table;
> > -	} else {
> > -		da9063_regmap_config.rd_table = &da9063_bb_readable_table;
> > -		da9063_regmap_config.wr_table =
> &da9063_bb_writeable_table;
> > -		da9063_regmap_config.volatile_table =
> &da9063_bb_volatile_table;
> > +	ret = da9063_get_device_type(i2c, da9063);
> > +	if (ret < 0)
> 
> Is a positive return value valid?
> 
> If not: if (ret)
> 

Yep, think this can be just 'if (ret)'