Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6522 bytes --]

On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote:

> +	  Only select this regulator driver if the MFD part is selected
> +	  in the Kernel configuration, it is meant to be operable as a cell.

This i what Kconfig dependencies are for.

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * intel-cht-wc-regulator.c - CherryTrail regulator driver
> + *

Please use C++ style for the entire comment so things look more
consistent.

> +#define CHT_WC_VPROG1A_VRANGE	53
> +#define CHT_WC_VPROG1B_VRANGE	53
> +#define CHT_WC_VPROG1F_VRANGE	53
> +#define CHT_WC_V1P8SX_VRANGE	53
> +#define CHT_WC_V1P2SX_VRANGE	53
> +#define CHT_WC_V1P2A_VRANGE	53
> +#define CHT_WC_VSDIO_VRANGE	53
> +#define CHT_WC_V2P8SX_VRANGE	53
> +#define CHT_WC_V3P3SD_VRANGE	53
> +#define CHT_WC_VPROG2D_VRANGE	53
> +#define CHT_WC_VPROG3A_VRANGE	53
> +#define CHT_WC_VPROG3B_VRANGE	53
> +#define CHT_WC_VPROG4A_VRANGE	53
> +#define CHT_WC_VPROG4B_VRANGE	53
> +#define CHT_WC_VPROG4C_VRANGE	53
> +#define CHT_WC_VPROG4D_VRANGE	53
> +#define CHT_WC_VPROG5A_VRANGE	53
> +#define CHT_WC_VPROG5B_VRANGE	53
> +#define CHT_WC_VPROG6A_VRANGE	53
> +#define CHT_WC_VPROG6B_VRANGE	53

These appear to be identical - is this not just a bunch of
instantiations of the same IPs

> +/* voltage tables */
> +static unsigned int CHT_WC_V3P3A_VSEL_TABLE[CHT_WC_V3P3A_VRANGE],
> +		    CHT_WC_V1P8A_VSEL_TABLE[CHT_WC_V1P8A_VRANGE],
> +		    CHT_WC_V1P05A_VSEL_TABLE[CHT_WC_V1P05A_VRANGE],
> +		    CHT_WC_VDDQ_VSEL_TABLE[CHT_WC_VDDQ_VRANGE],
> +		    CHT_WC_V1P8SX_VSEL_TABLE[CHT_WC_V1P8SX_VRANGE],
> +		    CHT_WC_V1P2SX_VSEL_TABLE[CHT_WC_V1P2SX_VRANGE],
> +		    CHT_WC_V1P2A_VSEL_TABLE[CHT_WC_V1P2A_VRANGE],
> +		    CHT_WC_V2P8SX_VSEL_TABLE[CHT_WC_V2P8SX_VRANGE],
> +		    CHT_WC_V3P3SD_VSEL_TABLE[CHT_WC_V3P3SD_VRANGE],
> +		    CHT_WC_VPROG1A_VSEL_TABLE[CHT_WC_VPROG1A_VRANGE],
> +		    CHT_WC_VPROG1B_VSEL_TABLE[CHT_WC_VPROG1B_VRANGE],
> +		    CHT_WC_VPROG1F_VSEL_TABLE[CHT_WC_VPROG1F_VRANGE],
> +		    CHT_WC_VPROG2D_VSEL_TABLE[CHT_WC_VPROG2D_VRANGE],
> +		    CHT_WC_VPROG3A_VSEL_TABLE[CHT_WC_VPROG3A_VRANGE],
> +		    CHT_WC_VPROG3B_VSEL_TABLE[CHT_WC_VPROG3B_VRANGE],
> +		    CHT_WC_VPROG4A_VSEL_TABLE[CHT_WC_VPROG4A_VRANGE],
> +		    CHT_WC_VPROG4B_VSEL_TABLE[CHT_WC_VPROG4B_VRANGE],
> +		    CHT_WC_VPROG4C_VSEL_TABLE[CHT_WC_VPROG4C_VRANGE],
> +		    CHT_WC_VPROG4D_VSEL_TABLE[CHT_WC_VPROG4D_VRANGE],
> +		    CHT_WC_VPROG5A_VSEL_TABLE[CHT_WC_VPROG5A_VRANGE],
> +		    CHT_WC_VPROG5B_VSEL_TABLE[CHT_WC_VPROG5B_VRANGE],
> +		    CHT_WC_VPROG6A_VSEL_TABLE[CHT_WC_VPROG6A_VRANGE],
> +		    CHT_WC_VPROG6B_VSEL_TABLE[CHT_WC_VPROG6B_VRANGE];

Please write a series of individual variable declarations, the
above way of writing things is very unusual and hence confusing
to read.  Though like I say it looks like the tables are mostly
identical so you probably only need a much smaller number of
tables, one per IP.

> +/*
> + * The VSDIO regulator should only support 1.8V and 3.3V. All other
> + * voltages are invalid for sd card, so disable them here.
> + */
> +static unsigned int CHT_WC_VSDIO_VSEL_TABLE[CHT_WC_VSDIO_VRANGE] = {
> +	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +	0, 0, 0, 0, 0, 0, 0, 0, 1800000, 0, 0, 0,
> +	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +	0, 0, 3300000, 0, 0
> +};

Is this really a limitation of the *regulator* or is it a
limitation of the consumer?  The combination of the way this is
written and the register layout makes it look like it's a
consumer limitation in which case leave it up to the consumer to
figure out what constraints it has.

> +/* List the SD card interface as a consumer of vqmmc voltage source from VSDIO
> + * regulator. This is the only interface that requires this source on Cherry
> + * Trail to operate with UHS-I cards.
> + */
> +static struct regulator_consumer_supply cht_wc_vqmmc_supply_consumer[] = {
> +	REGULATOR_SUPPLY("vqmmc", "80860F14:02"),
> +};

This looks like board specific configuration so should be defined
in the board.

> +static struct regulator_init_data vqmmc_init_data = {
> +	.constraints = {
> +		.min_uV			= 1800000,
> +		.max_uV			= 3300000,
> +		.valid_ops_mask		= REGULATOR_CHANGE_VOLTAGE |
> +					REGULATOR_CHANGE_STATUS,
> +		.valid_modes_mask	= REGULATOR_MODE_NORMAL,
> +		.settling_time		= 20000,
> +	},
> +	.num_consumer_supplies	= ARRAY_SIZE(cht_wc_vqmmc_supply_consumer),
> +	.consumer_supplies	= cht_wc_vqmmc_supply_consumer,
> +};

This *definitely* appears to be board specific configuration and
should be defined for the board.

> +static int cht_wc_regulator_enable(struct regulator_dev *rdev)
> +{
> +	struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);

regulator_enable_regmap()

> +static int cht_wc_regulator_disable(struct regulator_dev *rdev)
> +{
> +	struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);

regulator_disable_regmap()

> +static int cht_wc_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev);
> +	int rval;

This looks like it's a get_status() operation (reading back the
actual staus of the regulator rather than if we asked for it to
be enabled or disabled).

> +static int cht_wc_regulator_read_voltage_sel(struct regulator_dev *rdev)
> +{

regulator_get_voltage_sel_regmap()

> +static int cht_wc_regulator_set_voltage_sel(struct regulator_dev *rdev,
> +		unsigned int selector)

regulator_set_voltage_sel_regmap()

> +static void initialize_vtable(struct ch_wc_regulator_info *reg_info)
> +{
> +	unsigned int i, volt;
> +
> +	if (reg_info->runtime_table == true) {
> +		for (i = 0; i < reg_info->nvolts; i++) {
> +			volt = reg_info->min_mV + (i * reg_info->scale);
> +			if (volt < reg_info->min_mV)
> +				volt = reg_info->min_mV;
> +			if (volt > reg_info->max_mV)
> +				volt = reg_info->max_mV;
> +			/* set value in uV */
> +			reg_info->vtable[i] = volt*1000;
> +		}
> +	}
> +	reg_info->desc.volt_table = reg_info->vtable;
> +	reg_info->desc.n_voltages = reg_info->nvolts;
> +}

This looks like you've got a linear range so you should be using
regulator_map_voltage_linear and regulator_list_voltage_linear.

> +	regulator->rdev = regulator_register(&reg_info->desc, &config);

devm_regulator_register()

> +static int __init cht_wc_regulator_init(void)
> +{
> +	return platform_driver_register(&cht_wc_regulator_driver);
> +}
> +subsys_initcall_sync(cht_wc_regulator_init);

Why subsys_initcall() and not just module_platform_driver?
Deferred probe should work fine for regulators.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]