Messages in this thread | | | Date | Fri, 17 Apr 2020 10:24:10 +0100 | From | Lee Jones <> | Subject | Re: [RESEND PATCH v2 1/2] mfd: da9063: Fix revision handling to correctly select reg tables |
| |
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
| |