Re: [PATCHv5 05/10] mfd: rk808: split into core and i2c

[Lee Jones <lee@kernel.org>]

On Mon, 09 Jan 2023, Sebastian Reichel wrote:

> Split rk808 into a core and an i2c part in preperation for
> SPI support.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  drivers/clk/Kconfig                   |   2 +-
>  drivers/input/misc/Kconfig            |   2 +-
>  drivers/mfd/Kconfig                   |   7 +-
>  drivers/mfd/Makefile                  |   3 +-
>  drivers/mfd/{rk808.c => rk8xx-core.c} | 209 +++++---------------------
>  drivers/mfd/rk8xx-i2c.c               | 209 ++++++++++++++++++++++++++
>  drivers/pinctrl/Kconfig               |   2 +-
>  drivers/power/supply/Kconfig          |   2 +-
>  drivers/regulator/Kconfig             |   2 +-
>  drivers/rtc/Kconfig                   |   2 +-
>  include/linux/mfd/rk808.h             |   6 +
>  sound/soc/codecs/Kconfig              |   2 +-
>  12 files changed, 265 insertions(+), 183 deletions(-)
>  rename drivers/mfd/{rk808.c => rk8xx-core.c} (76%)
>  create mode 100644 drivers/mfd/rk8xx-i2c.c

[...]

> diff --git a/drivers/mfd/rk8xx-i2c.c b/drivers/mfd/rk8xx-i2c.c
> new file mode 100644
> index 000000000000..7babb0e1e64c
> --- /dev/null
> +++ b/drivers/mfd/rk8xx-i2c.c

[...]

> +static int rk8xx_i2c_get_variant(struct i2c_client *client)
> +{
> +	u8 pmic_id_msb, pmic_id_lsb;
> +	int msb, lsb;
> +
> +	if (of_device_is_compatible(client->dev.of_node, "rockchip,rk817") ||
> +	    of_device_is_compatible(client->dev.of_node, "rockchip,rk809")) {
> +		pmic_id_msb = RK817_ID_MSB;
> +		pmic_id_lsb = RK817_ID_LSB;
> +	} else {
> +		pmic_id_msb = RK808_ID_MSB;
> +		pmic_id_lsb = RK808_ID_LSB;
> +	}

Appreciate that this is probably old code, but it would be better do to
device matching with OF match.

> +	/* Read chip variant */
> +	msb = i2c_smbus_read_byte_data(client, pmic_id_msb);
> +	if (msb < 0)
> +		return dev_err_probe(&client->dev, msb, "failed to read the chip id MSB\n");
> +
> +	lsb = i2c_smbus_read_byte_data(client, pmic_id_lsb);
> +	if (lsb < 0)
> +		return dev_err_probe(&client->dev, lsb, "failed to read the chip id LSB\n");
> +
> +	return ((msb << 8) | lsb) & RK8XX_ID_MSK;

So this device provides the ability to dynamically read the chip IDs,
but in order to do so, you need to know what chip you're operating on?
Why wouldn't they always put the chip IDs in the same place!  I
understand this is just the variant, but still ...

> +static int rk8xx_i2c_probe(struct i2c_client *client)
> +{
> +	const struct regmap_config *regmap_cfg;
> +	struct regmap *regmap;
> +	int variant;
> +
> +	variant = rk8xx_i2c_get_variant(client);
> +	if (variant < 0)
> +		return variant;
> +
> +	switch (variant) {
> +	case RK805_ID:
> +		regmap_cfg = &rk805_regmap_config;
> +		break;
> +	case RK808_ID:
> +		regmap_cfg = &rk808_regmap_config;
> +		break;
> +	case RK818_ID:
> +		regmap_cfg = &rk818_regmap_config;
> +		break;
> +	case RK809_ID:
> +	case RK817_ID:
> +		regmap_cfg = &rk817_regmap_config;
> +		break;
> +	default:
> +		return dev_err_probe(&client->dev, -EINVAL, "Unsupported RK8XX ID %x\n", variant);
> +	}

All of this stuff could be passed through the OF match API.

> +
> +	regmap = devm_regmap_init_i2c(client, regmap_cfg);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(regmap),
> +				     "regmap initialization failed\n");
> +
> +	return rk8xx_probe(&client->dev, variant, client->irq, regmap);
> +}
> +
> +static void rk8xx_i2c_shutdown(struct i2c_client *client)
> +{
> +	rk8xx_shutdown(&client->dev);
> +}
> +
> +static int __maybe_unused rk8xx_i2c_suspend(struct device *dev)
> +{
> +	return rk8xx_suspend(dev);
> +}
> +
> +static int __maybe_unused rk8xx_i2c_resume(struct device *dev)
> +{
> +	return rk8xx_resume(dev);
> +}
> +static SIMPLE_DEV_PM_OPS(rk8xx_i2c_pm_ops, rk8xx_i2c_suspend, rk8xx_i2c_resume);

Why not place the exported functions straight into the MACRO?

> +static const struct of_device_id rk8xx_i2c_of_match[] = {
> +	{ .compatible = "rockchip,rk805" },
> +	{ .compatible = "rockchip,rk808" },
> +	{ .compatible = "rockchip,rk809" },
> +	{ .compatible = "rockchip,rk817" },
> +	{ .compatible = "rockchip,rk818" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, rk8xx_i2c_of_match);
> +
> +static struct i2c_driver rk8xx_i2c_driver = {
> +	.driver = {
> +		.name = "rk8xx-i2c",
> +		.of_match_table = rk8xx_i2c_of_match,
> +		.pm = &rk8xx_i2c_pm_ops,
> +	},
> +	.probe_new = rk8xx_i2c_probe,
> +	.shutdown  = rk8xx_i2c_shutdown,
> +};
> +module_i2c_driver(rk8xx_i2c_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Chris Zhong <zyw@rock-chips.com>");
> +MODULE_AUTHOR("Zhang Qing <zhangqing@rock-chips.com>");
> +MODULE_AUTHOR("Wadim Egorov <w.egorov@phytec.de>");
> +MODULE_DESCRIPTION("RK8xx I2C PMIC driver");

-- 
Lee Jones [李琼斯]