Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 18 Jun 2022 16:55:33 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     ChiaEn Wu <peterwu.pub@gmail.com>
Cc:     lars@metafoo.de, matthias.bgg@gmail.com, lee.jones@linaro.org,
        daniel.thompson@linaro.org, jingoohan1@gmail.com, pavel@ucw.cz,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org, linux-leds@vger.kernel.org,
        devicetree@vger.kernel.org, linux-pm@vger.kernel.org,
        linux-usb@vger.kernel.org, linux-fbdev@vger.kernel.org,
        szunichen@gmail.com, ChiYuan Huang <cy_huang@richtek.com>
Subject: Re: [PATCH v2 08/15] mfd: mt6370: Add Mediatek MT6370 support
Message-ID: <20220618165533.5669e288@jic23-huawei>
In-Reply-To: <20220613111146.25221-9-peterwu.pub@gmail.com>
References: <20220613111146.25221-1-peterwu.pub@gmail.com>
        <20220613111146.25221-9-peterwu.pub@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 13 Jun 2022 19:11:39 +0800
ChiaEn Wu <peterwu.pub@gmail.com> wrote:

> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Add Mediatek MT6370 MFD support.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
Hi.

A few trivial things that probably overlap with other reviewer
comments.


> +static int mt6370_check_vendor_info(struct mt6370_info *info)
> +{
> +	unsigned int devinfo;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, MT6370_REG_DEV_INFO, &devinfo);
> +	if (ret)
> +		return ret;
> +
> +	switch (FIELD_GET(MT6370_VENID_MASK, devinfo)) {
> +	case 0x8: /* RT5081 */
> +	case 0xA: /* RT5081A */
> +	case 0xE: /* MT6370 */
> +	case 0xF: /* MT6371 */
> +	case 0x9: /* MT6372P */
> +	case 0xB: /* MT6372CP */
> +		break;
> +	default:
> +		dev_err(info->dev, "Not invalid value 0x%02x\n", devinfo);

Double negative... You mean "Invalid value" 
though you probably want to say something more specific such as
		"Unknown Vendor ID 0x%02x\n")

> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mt6370_regmap_read(void *context, const void *reg_buf,
> +			      size_t reg_size, void *val_buf, size_t val_size)
> +{
> +	struct mt6370_info *info = context;
> +	u8 bank_idx = *(u8 *)reg_buf, bank_addr = *(u8 *)(reg_buf + 1);
> +	int ret;
> +
> +	ret = i2c_smbus_read_i2c_block_data(info->i2c[bank_idx], bank_addr,
> +					    val_size, val_buf);
> +	if (ret != val_size)
> +		return ret;
If it's positive , that is probably not what regmap expects.
See handling in here
https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap-i2c.c#L222

> +
> +	return 0;
> +}


> +
> +static int mt6370_probe(struct i2c_client *i2c)
> +{
> +	struct mt6370_info *info;
> +	struct i2c_client *usbc_i2c;
> +	int ret;
> +
> +	info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->dev = &i2c->dev;
> +
> +	usbc_i2c = devm_i2c_new_dummy_device(&i2c->dev, i2c->adapter,
> +					     MT6370_USBC_I2CADDR);
> +	if (IS_ERR(usbc_i2c)) {
> +		ret = PTR_ERR(usbc_i2c);
> +		dev_err(&i2c->dev, "Failed to register usbc i2c client %d\n", ret);
Might as well use
		return dev_err_probe(&i2c->dev, PTR_ERR(usb_i2c),
				     "Failed to register USBC I2C client\n");

Whilst the defer handling isn't relevant, it is safe to use throughout probe
and tidier in this case at least.

> +		return ret;
> +	}
> +
> +	/* Assign I2C client for PMU and TypeC */
> +	info->i2c[MT6370_PMU_I2C] = i2c;
> +	info->i2c[MT6370_USBC_I2C] = usbc_i2c;
> +
> +	info->regmap = devm_regmap_init(&i2c->dev, &mt6370_regmap_bus, info,
> +					&mt6370_regmap_config);
> +	if (IS_ERR(info->regmap)) {
> +		ret = PTR_ERR(info->regmap);
> +		dev_err(&i2c->dev, "Failed to register regmap (%d)\n", ret);

as above dev_err_probe() will tidy this and following cases up for you.

> +		return ret;
> +	}
> +
> +	ret = mt6370_check_vendor_info(info);
> +	if (ret) {
> +		dev_err(&i2c->dev, "Failed to check vendor info (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_regmap_add_irq_chip(&i2c->dev, info->regmap, i2c->irq,
> +				       IRQF_ONESHOT, -1, &mt6370_irq_chip,
> +				       &info->irq_data);
> +	if (ret) {
> +		dev_err(&i2c->dev, "Failed to add irq chip (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	return devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> +				    mt6370_devices, ARRAY_SIZE(mt6370_devices),
> +				    NULL, 0,
> +				    regmap_irq_get_domain(info->irq_data));
> +}