Hi Andy,
Thanks for the reviews.
> Subject: Re: [PATCH v2 1/3] mfd: Add new mfd device TPS68470
>
> On Sun, Jun 11, 2017 at 9:09 AM, Rajmohan Mani
> <rajmohan.mani@xxxxxxxxx> wrote:
> > The TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for Flash and incorporates two LED drivers for
> > general purpose indicators.
> >
> > This patch adds support for TPS68470 mfd device.
>
> Thanks! This looks much better, though see my few comments below.
>
> > +/*
> > + * This lookup table for the TPS68470 GPIOs, lists
> > + * the 7 GPIOs (that can be configured as input or output
> > + * as appropriate) and 3 special purpose GPIOs that are
> > + * "output only". Exporting these GPIOs in a system mounted
> > + * with the TPS68470, in conjunction with the gpio-tps68470
> > + * driver, allows the platform firmware to configure these
> > + * GPIOs appropriately, through the ACPI operation region.
> > + * These 7 configurable GPIOs can be connected to power rails,
> > + * sensor control (e.g sensor reset), while the 3 GPIOs can
> > + * be used for sensor control.
> > + */
>
> > +struct gpiod_lookup_table gpios_table = {
> > + .dev_id = NULL,
>
> Why dev_id is NULL?
>
I have removed the GPIO lookup tables in the driver.
> > + .table = {
> > + GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 7, "s_enable",
> GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn",
> GPIO_ACTIVE_HIGH),
> > + {},
> > + },
> > +};
>
> I don't remember if I asked already why this table exists at all in the driver.
> Shouldn't it be provided by ACPI _DSD?
>
Ack. I have removed the GPIO lookup tables in the driver.
> > +static int tps68470_chip_init(struct device *dev, struct regmap
> > +*regmap) {
> > + unsigned int version;
> > + int ret;
> > +
> > + ret = regmap_read(regmap, TPS68470_REG_REVID, &version);
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to read revision register: %d\n", ret);
> > + return ret;
> > + }
> > +
>
> > + dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
>
> This will confuse user when probe fails. Should be printed only when we return
> 0 for sure.
>
Ack.
> > + ret = regmap_write(regmap, TPS68470_REG_RESET, 0xff);
> > + if (ret < 0)
> > + return ret;
> > +
>
> > + /* FIXME: configure these dynamically */
>
> Please, either fix or remove this comment.
>
Will keep this comment, until I see how this can be fixed.
> > + /* Enable daisy chain */
> > + ret = regmap_update_bits(regmap, TPS68470_REG_S_I2C_CTL, 1, 1);
> > + if (ret < 0)
> > + return ret;
> > +
>
> > + usleep_range(TPS68470_DAISY_CHAIN_DELAY_US,
> > + TPS68470_DAISY_CHAIN_DELAY_US + 10);
>
> This might require a comment, though I'm fine with it as long as it close to
> previous excerpt.
>
Ack. Added comment.
> > + return 0;
> > +}
>
> > +static int tps68470_probe(struct i2c_client *client) {
> > + struct device *dev = &client->dev;
> > + struct regmap *regmap;
> > + int ret;
> > +
> > + regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
> > + if (IS_ERR(regmap)) {
>
> > + dev_err(dev, "devm_regmap_init_i2c Error %ld\n",
> > + PTR_ERR(regmap));
>
> 1. Indentation.
Ack
> 2. Do we really need this message?
>
Since the driver probe fails, it would be useful to know more info on that.
> > + return PTR_ERR(regmap);
> > + }
> > +
> > + i2c_set_clientdata(client, regmap);
> > +
> > + gpiod_add_lookup_table(&gpios_table);
> > +
>
> > + ret = devm_mfd_add_devices(dev, -1, tps68470s,
> > + ARRAY_SIZE(tps68470s), NULL, 0, NULL);
>
> -1 has a definition for such case, use it instead.
>
Ack
> > + if (ret < 0) {
> > + dev_err(dev, "mfd_add_devices failed: %d\n", ret);
> > + return ret;
> > + }
>
> > +static const struct i2c_device_id tps68470_id_table[] = {
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, tps68470_id_table);
>
> Either choose ->probe() over ->probe_new() or remove above.
>
Ack. Chose the former.
> > +static struct i2c_driver tps68470_driver = {
> > + .driver = {
> > + .name = "tps68470",
>
> > + .acpi_match_table = ACPI_PTR(tps68470_acpi_ids),
>
> ACPI_PTR() is redundant.
>
Ack
> > +#include <linux/i2c.h>
>
> And this is for...?
>
use by tps68470.c which uses i2c_driver and other such i2c defines.
��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
