lkml.org 
[lkml]   [2022]   [May]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 6/7] regulator: add mt6357 regulator
On Tue, May 31, 2022 at 02:49:58PM +0200, Fabien Parent wrote:

> @@ -0,0 +1,485 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 MediaTek Inc.
> + * Copyright (c) 2022 BayLibre, SAS.
> + * Author: Chen Zhong <chen.zhong@mediatek.com>
> + * Author: Fabien Parent <fparent@baylibre.com>
> + *
> + * Based on mt6397-regulator.c
> + */

Please make the entire comment a C++ one so things look more
intentional.

> +static int mt6357_set_voltage_sel(struct regulator_dev *rdev,
> + unsigned int selector)
> +{
> + int idx, ret;
> + const u32 *pvol;
> + struct mt6357_regulator_info *info = rdev_get_drvdata(rdev);
> +
> + pvol = info->index_table;
> +
> + idx = pvol[selector];
> + ret = regmap_update_bits(rdev->regmap, info->desc.vsel_reg,
> + info->desc.vsel_mask,
> + idx << 8);
> +
> + return ret;
> +}

I don't really understand what's going on with selectors here -
the general idea with selectors is that we just pass the value
that's written to the bitfield around and have mapping functions
which convert to/from voltages. Here we're mapping the selectors
using this index_table thing which seems like an extra layer of
mapping. What's the story there?

I don't see why this can't just use the functions in helpers.c.
It looks like you can use _list_voltage_table() -EINVALs in there
for invalid selectors and _map_voltage_iterate(). Note that
n_voltages is effectively a maximum selector.

> +static int mt6357_get_buck_voltage_sel(struct regulator_dev *rdev)
> +{
> + int ret, regval;
> + struct mt6357_regulator_info *info = rdev_get_drvdata(rdev);
> +
> + ret = regmap_read(rdev->regmap, info->da_vsel_reg, &regval);
> + if (ret != 0) {
> + dev_err(&rdev->dev,
> + "Failed to get mt6357 Buck %s vsel reg: %d\n",
> + info->desc.name, ret);
> + return ret;
> + }
> +
> + ret = (regval & info->da_vsel_mask) >> info->da_vsel_shift;
> +
> + return ret;
> +}

This looks like it should just be able to use
regulator_get_voltage_sel_regmap()?

Otherwise this looks good.
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2022-05-31 15:04    [W:0.027 / U:0.512 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site