| 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: <20220109161341.19484fc0@jic23-huawei>
Date: Sun, 9 Jan 2022 16:13:49 +0000
From: Jonathan Cameron <jic23@...23.retrosnub.co.uk>
To: Baolin Wang <baolin.wang7@...il.com>
Cc: Cixi Geng <gengcixi@...il.com>, Orson Zhai <orsonzhai@...il.com>,
Chunyan Zhang <zhang.lyra@...il.com>,
Lars-Peter Clausen <lars@...afoo.de>,
Rob Herring <robh+dt@...nel.org>, lgirdwood@...il.com,
Mark Brown <broonie@...nel.org>, yuming.zhu1@...soc.com,
linux-iio@...r.kernel.org,
Devicetree List <devicetree@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/7] iio: adc: sc27xx: add support for PMIC sc2720 and
sc2721
On Fri, 7 Jan 2022 15:16:15 +0800
Baolin Wang <baolin.wang7@...il.com> wrote:
> On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@...il.com> wrote:
> >
> > From: Cixi Geng <cixi.geng1@...soc.com>
> >
> > sc2720 and sc2721 is the product of sc27xx series.
> >
> > Signed-off-by: Yuming Zhu <yuming.zhu1@...soc.com>
> > Signed-off-by: Cixi Geng <cixi.geng1@...soc.com>
> > ---
> > drivers/iio/adc/sc27xx_adc.c | 198 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 198 insertions(+)
> >
> > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> > index d2712e54ee79..7b5c66660ac9 100644
> > --- a/drivers/iio/adc/sc27xx_adc.c
> > +++ b/drivers/iio/adc/sc27xx_adc.c
> > @@ -9,11 +9,13 @@
> > #include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > #include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > #include <linux/slab.h>
> >
> > /* PMIC global registers definition */
> > #define SC2731_MODULE_EN 0xc08
> > #define SC27XX_MODULE_ADC_EN BIT(5)
> > +#define SC2721_ARM_CLK_EN 0xc0c
> > #define SC2731_ARM_CLK_EN 0xc10
> > #define SC27XX_CLK_ADC_EN BIT(5)
> > #define SC27XX_CLK_ADC_CLK_EN BIT(6)
> > @@ -37,7 +39,9 @@
> > /* Bits and mask definition for SC27XX_ADC_CH_CFG register */
> > #define SC27XX_ADC_CHN_ID_MASK GENMASK(4, 0)
> > #define SC27XX_ADC_SCALE_MASK GENMASK(10, 9)
> > +#define SC2721_ADC_SCALE_MASK BIT(5)
> > #define SC27XX_ADC_SCALE_SHIFT 9
> > +#define SC2721_ADC_SCALE_SHIFT 5
> >
> > /* Bits definitions for SC27XX_ADC_INT_EN registers */
> > #define SC27XX_ADC_IRQ_EN BIT(0)
> > @@ -67,8 +71,21 @@
> > #define SC27XX_RATIO_NUMERATOR_OFFSET 16
> > #define SC27XX_RATIO_DENOMINATOR_MASK GENMASK(15, 0)
> >
> > +/* ADC specific channel reference voltage 3.5V */
> > +#define SC27XX_ADC_REFVOL_VDD35 3500000
> > +
> > +/* ADC default channel reference voltage is 2.8V */
> > +#define SC27XX_ADC_REFVOL_VDD28 2800000
> > +
> > +enum sc27xx_pmic_type {
> > + SC27XX_ADC,
> > + SC2721_ADC,
> > +};
> > +
> > struct sc27xx_adc_data {
> > + struct iio_dev *indio_dev;
>
> Why add an unused member?
It's very very rarely a good architecture structure to have
the data stored in iio_priv() have a pointer back to the indio_dev.
Normally it implies somewhere the wrong level of structure is being
passed to a function.
So I'm glad it's not used :)
>
> > struct device *dev;
> > + struct regulator *volref;
> > struct regmap *regmap;
> > /*
> > * One hardware spinlock to synchronize between the multiple
> > @@ -87,6 +104,7 @@ struct sc27xx_adc_data {
> > * in the device data structure.
> > */
...
>
> > +
> > static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
> > {
> > int i;
> > @@ -239,6 +373,24 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> > return ret;
> > }
> >
> > + /*
> > + * According to the sc2721 chip data sheet, the reference voltage of
> > + * specific channel 30 and channel 31 in ADC module needs to be set from
> > + * the default 2.8v to 3.5v.
That's horrible... :) Ah well...
> > + */
> > + if (data->var_data->pmic_type == SC2721_ADC) {
> > + if ((channel == 30) || (channel == 31)) {
>
> Combine the two branches please.
>
> > + ret = regulator_set_voltage(data->volref,
> > + SC27XX_ADC_REFVOL_VDD35,
> > + SC27XX_ADC_REFVOL_VDD35);
> > + if (ret) {
> > + dev_err(data->dev, "failed to set the volref 3.5V\n");
> > + hwspin_unlock_raw(data->hwlock);
> > + return ret;
> > + }
> > + }
> > + }
> > +
> > ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
> > SC27XX_ADC_EN, SC27XX_ADC_EN);
> > if (ret)
> > @@ -293,6 +445,16 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> > regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
> > SC27XX_ADC_EN, 0);
> > unlock_adc:
> > + if (data->var_data->pmic_type == SC2721_ADC) {
> > + if ((channel == 30) || (channel == 31)) {
> > + ret = regulator_set_voltage(data->volref,
> > + SC27XX_ADC_REFVOL_VDD28,
> > + SC27XX_ADC_REFVOL_VDD28);
> > + if (ret)
> > + dev_err(data->dev, "failed to set the volref 2.8V\n");
> > + }
> > + }
> > +
> > hwspin_unlock_raw(data->hwlock);
> >
> > if (!ret)
> > @@ -522,6 +684,7 @@ static void sc27xx_adc_disable(void *_data)
> > }
...
>
Powered by blists - more mailing lists