| 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: <CA+V-a8vQm+JDL19TwjcNBD-xGzFApZywJCqVisBVRBNC2xwLHA@mail.gmail.com>
Date: Sun, 8 Dec 2024 21:06:45 +0000
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Claudiu <claudiu.beznea@...on.dev>
Cc: prabhakar.mahadev-lad.rj@...renesas.com, jic23@...nel.org, lars@...afoo.de,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
geert+renesas@...der.be, magnus.damm@...il.com, mturquette@...libre.com,
sboyd@...nel.org, p.zabel@...gutronix.de, linux-iio@...r.kernel.org,
linux-renesas-soc@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH v2 09/15] iio: adc: rzg2l_adc: Prepare for the addition of
RZ/G3S support
On Fri, Dec 6, 2024 at 11:16 AM Claudiu <claudiu.beznea@...on.dev> wrote:
>
> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>
> The ADC IP available on the RZ/G3S differs slightly from the one found on
> the RZ/G2L. The identified differences are as follows:
> - different number of channels (one being used for temperature conversion);
> consequently, various registers differ
> - different default sampling periods
> - the RZ/G3S variant lacks the ADVIC register.
>
> To accommodate these differences, the rzg2l_adc driver has been updated by
> introducing the struct rzg2l_adc_hw_params, which encapsulates the
> hardware-specific differences between the IP variants. A pointer to an
> object of type struct rzg2l_adc_hw_params is embedded in
> struct rzg2l_adc_data.
>
> Additionally, the completion member of struct rzg2l_adc_data was relocated
> to avoid potential padding, if any.
>
> The code has been adjusted to utilize hardware-specific parameters stored
> in the new structure instead of relying on plain macros.
>
> The check of chan->channel in rzg2l_adc_read_raw() function, against the
> driver specific mask was removed as the subsystem should have already
> been done this before reaching the rzg2l_adc_read_raw() function. Along
> with it the local variable ch was dropped as chan->channel could be used
> instead.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> ---
>
> Changes in v2:
> - kept the RZG2L_ADC_MAX_CHANNELS as suggested in the review process;
> along with it, last_val[] is now again statically alocated; code
> from v1 around last_val has been adjusted to align with the new
> approach
> - dropped ch variable from rzg2l_adc_read_raw() and adjusted the
> patch description to reflect it.
>
> drivers/iio/adc/rzg2l_adc.c | 87 +++++++++++++++++++++++++------------
> 1 file changed, 59 insertions(+), 28 deletions(-)
>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Cheers,
Prabhakar
> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> index c3f9f95cdbba..6740912f83c5 100644
> --- a/drivers/iio/adc/rzg2l_adc.c
> +++ b/drivers/iio/adc/rzg2l_adc.c
> @@ -33,20 +33,15 @@
> #define RZG2L_ADM1_MS BIT(2)
> #define RZG2L_ADM1_BS BIT(4)
> #define RZG2L_ADM1_EGA_MASK GENMASK(13, 12)
> -#define RZG2L_ADM2_CHSEL_MASK GENMASK(7, 0)
> #define RZG2L_ADM3_ADIL_MASK GENMASK(31, 24)
> #define RZG2L_ADM3_ADCMP_MASK GENMASK(23, 16)
> -#define RZG2L_ADM3_ADCMP_E FIELD_PREP(RZG2L_ADM3_ADCMP_MASK, 0xe)
> -#define RZG2L_ADM3_ADSMP_MASK GENMASK(15, 0)
>
> #define RZG2L_ADINT 0x20
> -#define RZG2L_ADINT_INTEN_MASK GENMASK(7, 0)
> #define RZG2L_ADINT_CSEEN BIT(16)
> #define RZG2L_ADINT_INTS BIT(31)
>
> #define RZG2L_ADSTS 0x24
> #define RZG2L_ADSTS_CSEST BIT(16)
> -#define RZG2L_ADSTS_INTST_MASK GENMASK(7, 0)
>
> #define RZG2L_ADIVC 0x28
> #define RZG2L_ADIVC_DIVADC_MASK GENMASK(8, 0)
> @@ -57,12 +52,27 @@
> #define RZG2L_ADCR(n) (0x30 + ((n) * 0x4))
> #define RZG2L_ADCR_AD_MASK GENMASK(11, 0)
>
> -#define RZG2L_ADSMP_DEFAULT_SAMPLING 0x578
> -
> #define RZG2L_ADC_MAX_CHANNELS 8
> -#define RZG2L_ADC_CHN_MASK 0x7
> #define RZG2L_ADC_TIMEOUT usecs_to_jiffies(1 * 4)
>
> +/**
> + * struct rzg2l_adc_hw_params - ADC hardware specific parameters
> + * @default_adsmp: default ADC sampling period (see ADM3 register)
> + * @adsmp_mask: ADC sampling period mask (see ADM3 register)
> + * @adint_inten_mask: conversion end interrupt mask (see ADINT register)
> + * @default_adcmp: default ADC cmp (see ADM3 register)
> + * @num_channels: number of supported channels
> + * @adivc: specifies if ADVIC register is available
> + */
> +struct rzg2l_adc_hw_params {
> + u16 default_adsmp;
> + u16 adsmp_mask;
> + u16 adint_inten_mask;
> + u8 default_adcmp;
> + u8 num_channels;
> + bool adivc;
> +};
> +
> struct rzg2l_adc_data {
> const struct iio_chan_spec *channels;
> u8 num_channels;
> @@ -72,8 +82,9 @@ struct rzg2l_adc {
> void __iomem *base;
> struct reset_control *presetn;
> struct reset_control *adrstn;
> - struct completion completion;
> const struct rzg2l_adc_data *data;
> + const struct rzg2l_adc_hw_params *hw_params;
> + struct completion completion;
> struct mutex lock;
> u16 last_val[RZG2L_ADC_MAX_CHANNELS];
> };
> @@ -154,6 +165,7 @@ static void rzg2l_set_trigger(struct rzg2l_adc *adc)
>
> static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
> {
> + const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
> u32 reg;
>
> if (rzg2l_adc_readl(adc, RZG2L_ADM(0)) & RZG2L_ADM0_ADBSY)
> @@ -163,7 +175,7 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
>
> /* Select analog input channel subjected to conversion. */
> reg = rzg2l_adc_readl(adc, RZG2L_ADM(2));
> - reg &= ~RZG2L_ADM2_CHSEL_MASK;
> + reg &= ~GENMASK(hw_params->num_channels - 1, 0);
> reg |= BIT(ch);
> rzg2l_adc_writel(adc, RZG2L_ADM(2), reg);
>
> @@ -175,7 +187,7 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
> */
> reg = rzg2l_adc_readl(adc, RZG2L_ADINT);
> reg &= ~RZG2L_ADINT_INTS;
> - reg &= ~RZG2L_ADINT_INTEN_MASK;
> + reg &= ~hw_params->adint_inten_mask;
> reg |= (RZG2L_ADINT_CSEEN | BIT(ch));
> rzg2l_adc_writel(adc, RZG2L_ADINT, reg);
>
> @@ -184,6 +196,7 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
>
> static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc, u8 ch)
> {
> + const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
> struct device *dev = indio_dev->dev.parent;
> int ret;
>
> @@ -201,7 +214,7 @@ static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc
>
> if (!wait_for_completion_timeout(&adc->completion, RZG2L_ADC_TIMEOUT)) {
> rzg2l_adc_writel(adc, RZG2L_ADINT,
> - rzg2l_adc_readl(adc, RZG2L_ADINT) & ~RZG2L_ADINT_INTEN_MASK);
> + rzg2l_adc_readl(adc, RZG2L_ADINT) & ~hw_params->adint_inten_mask);
> ret = -ETIMEDOUT;
> }
>
> @@ -219,7 +232,6 @@ static int rzg2l_adc_read_raw(struct iio_dev *indio_dev,
> {
> struct rzg2l_adc *adc = iio_priv(indio_dev);
> int ret;
> - u8 ch;
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW: {
> @@ -228,12 +240,11 @@ static int rzg2l_adc_read_raw(struct iio_dev *indio_dev,
>
> guard(mutex)(&adc->lock);
>
> - ch = chan->channel & RZG2L_ADC_CHN_MASK;
> - ret = rzg2l_adc_conversion(indio_dev, adc, ch);
> + ret = rzg2l_adc_conversion(indio_dev, adc, chan->channel);
> if (ret)
> return ret;
>
> - *val = adc->last_val[ch];
> + *val = adc->last_val[chan->channel];
>
> return IIO_VAL_INT;
> }
> @@ -258,6 +269,7 @@ static const struct iio_info rzg2l_adc_iio_info = {
> static irqreturn_t rzg2l_adc_isr(int irq, void *dev_id)
> {
> struct rzg2l_adc *adc = dev_id;
> + const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
> unsigned long intst;
> u32 reg;
> int ch;
> @@ -270,11 +282,11 @@ static irqreturn_t rzg2l_adc_isr(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> - intst = reg & RZG2L_ADSTS_INTST_MASK;
> + intst = reg & GENMASK(hw_params->num_channels - 1, 0);
> if (!intst)
> return IRQ_NONE;
>
> - for_each_set_bit(ch, &intst, RZG2L_ADC_MAX_CHANNELS)
> + for_each_set_bit(ch, &intst, hw_params->num_channels)
> adc->last_val[ch] = rzg2l_adc_readl(adc, RZG2L_ADCR(ch)) & RZG2L_ADCR_AD_MASK;
>
> /* clear the channel interrupt */
> @@ -287,6 +299,7 @@ static irqreturn_t rzg2l_adc_isr(int irq, void *dev_id)
>
> static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l_adc *adc)
> {
> + const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
> struct iio_chan_spec *chan_array;
> struct rzg2l_adc_data *data;
> unsigned int channel;
> @@ -302,7 +315,7 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
> if (!num_channels)
> return dev_err_probe(&pdev->dev, -ENODEV, "no channel children\n");
>
> - if (num_channels > RZG2L_ADC_MAX_CHANNELS)
> + if (num_channels > hw_params->num_channels)
> return dev_err_probe(&pdev->dev, -EINVAL, "num of channel children out of range\n");
>
> chan_array = devm_kcalloc(&pdev->dev, num_channels, sizeof(*chan_array),
> @@ -316,7 +329,7 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
> if (ret)
> return ret;
>
> - if (channel >= RZG2L_ADC_MAX_CHANNELS)
> + if (channel >= hw_params->num_channels)
> return -EINVAL;
>
> chan_array[i].type = IIO_VOLTAGE;
> @@ -336,6 +349,7 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
>
> static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
> {
> + const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
> u32 reg;
> int ret;
>
> @@ -353,11 +367,13 @@ static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
> if (ret)
> goto exit_hw_init;
>
> - /* Only division by 4 can be set */
> - reg = rzg2l_adc_readl(adc, RZG2L_ADIVC);
> - reg &= ~RZG2L_ADIVC_DIVADC_MASK;
> - reg |= RZG2L_ADIVC_DIVADC_4;
> - rzg2l_adc_writel(adc, RZG2L_ADIVC, reg);
> + if (hw_params->adivc) {
> + /* Only division by 4 can be set */
> + reg = rzg2l_adc_readl(adc, RZG2L_ADIVC);
> + reg &= ~RZG2L_ADIVC_DIVADC_MASK;
> + reg |= RZG2L_ADIVC_DIVADC_4;
> + rzg2l_adc_writel(adc, RZG2L_ADIVC, reg);
> + }
>
> /*
> * Setup AMD3
> @@ -368,8 +384,10 @@ static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
> reg = rzg2l_adc_readl(adc, RZG2L_ADM(3));
> reg &= ~RZG2L_ADM3_ADIL_MASK;
> reg &= ~RZG2L_ADM3_ADCMP_MASK;
> - reg &= ~RZG2L_ADM3_ADSMP_MASK;
> - reg |= (RZG2L_ADM3_ADCMP_E | RZG2L_ADSMP_DEFAULT_SAMPLING);
> + reg &= ~hw_params->adsmp_mask;
> + reg |= FIELD_PREP(RZG2L_ADM3_ADCMP_MASK, hw_params->default_adcmp) |
> + hw_params->default_adsmp;
> +
> rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
>
> exit_hw_init:
> @@ -392,6 +410,10 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>
> adc = iio_priv(indio_dev);
>
> + adc->hw_params = device_get_match_data(dev);
> + if (!adc->hw_params || adc->hw_params->num_channels > RZG2L_ADC_MAX_CHANNELS)
> + return -EINVAL;
> +
> ret = rzg2l_adc_parse_properties(pdev, adc);
> if (ret)
> return ret;
> @@ -444,8 +466,17 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
> return devm_iio_device_register(dev, indio_dev);
> }
>
> +static const struct rzg2l_adc_hw_params rzg2l_hw_params = {
> + .num_channels = 8,
> + .default_adcmp = 0xe,
> + .default_adsmp = 0x578,
> + .adsmp_mask = GENMASK(15, 0),
> + .adint_inten_mask = GENMASK(7, 0),
> + .adivc = true
> +};
> +
> static const struct of_device_id rzg2l_adc_match[] = {
> - { .compatible = "renesas,rzg2l-adc",},
> + { .compatible = "renesas,rzg2l-adc", .data = &rzg2l_hw_params },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, rzg2l_adc_match);
> --
> 2.39.2
>
>
Powered by blists - more mailing lists