| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20191025121737.GC4568@sirena.org.uk>
Date: Fri, 25 Oct 2019 13:17:37 +0100
From: Mark Brown <broonie@...nel.org>
To: Andrey Zhizhikin <andrey.z@...il.com>
Cc: lgirdwood@...il.com, andriy.shevchenko@...ux.intel.com,
lee.jones@...aro.org, linux-kernel@...r.kernel.org,
Andrey Zhizhikin <andrey.zhizhikin@...ca-geosystems.com>
Subject: Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove
regulator
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(®_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.
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists