RE: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jonathan,

Thanks for the feedback,

> Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC
> 
> On Sun,  1 May 2022 09:15:23 +0100
> Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> 
> > ADC found on RZ/G2UL SoC is almost identical to RZ/G2L SoC, but
> > RZ/G2UL has 2 analog input channels compared to 8 channels on RZ/G2L.
> > Therefore, added a new compatible to handle this difference.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> 
> Hi.
> 
> Please keep the driver changes and DT update in a series with a short
> cover letter.  It makes much more sense to apply them when both ready than
> to end up with them being handled separately.

OK,  will send as a series on next version.
> 
> A request inline to do this in a slightly different way that will prove
> more flexible if we end up supporting more variants in the future.
> 
> > ---
> >  drivers/iio/adc/rzg2l_adc.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> > index 7585144b9715..703b08254c9f 100644
> > --- a/drivers/iio/adc/rzg2l_adc.c
> > +++ b/drivers/iio/adc/rzg2l_adc.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/io.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> > +#include <linux/of_device.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/reset.h>
> > @@ -61,6 +62,8 @@
> >  #define RZG2L_ADC_CHN_MASK		0x7
> >  #define RZG2L_ADC_TIMEOUT		usecs_to_jiffies(1 * 4)
> >
> > +#define RZG2UL_ADC_MAX_CHANNELS		2
> > +
> >  struct rzg2l_adc_data {
> >  	const struct iio_chan_spec *channels;
> >  	u8 num_channels;
> > @@ -76,6 +79,7 @@ struct rzg2l_adc {
> >  	const struct rzg2l_adc_data *data;
> >  	struct mutex lock;
> >  	u16 last_val[RZG2L_ADC_MAX_CHANNELS];
> > +	u8 max_channels;
> >  };
> >
> >  static const char * const rzg2l_adc_channel_name[] = { @@ -260,7
> > +264,9 @@ static int rzg2l_adc_read_label(struct iio_dev *iio_dev,
> >  				const struct iio_chan_spec *chan,
> >  				char *label)
> >  {
> > -	if (chan->channel >= RZG2L_ADC_MAX_CHANNELS)
> > +	struct rzg2l_adc *adc = iio_priv(iio_dev);
> > +
> > +	if (chan->channel >= adc->max_channels)
> >  		return -EINVAL;
> >
> >  	return sysfs_emit(label, "%s\n",
> > rzg2l_adc_channel_name[chan->channel]);
> > @@ -290,7 +296,7 @@ static irqreturn_t rzg2l_adc_isr(int irq, void
> *dev_id)
> >  	if (!intst)
> >  		return IRQ_NONE;
> >
> > -	for_each_set_bit(ch, &intst, RZG2L_ADC_MAX_CHANNELS)
> > +	for_each_set_bit(ch, &intst, adc->max_channels)
> >  		adc->last_val[ch] = rzg2l_adc_readl(adc, RZG2L_ADCR(ch)) &
> > RZG2L_ADCR_AD_MASK;
> >
> >  	/* clear the channel interrupt */
> > @@ -321,7 +327,7 @@ static int rzg2l_adc_parse_properties(struct
> platform_device *pdev, struct rzg2l
> >  		return -ENODEV;
> >  	}
> >
> > -	if (num_channels > RZG2L_ADC_MAX_CHANNELS) {
> > +	if (num_channels > adc->max_channels) {
> >  		dev_err(&pdev->dev, "num of channel children out of range\n");
> >  		return -EINVAL;
> >  	}
> > @@ -337,7 +343,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 >= adc->max_channels)
> >  			return -EINVAL;
> >
> >  		chan_array[i].type = IIO_VOLTAGE;
> > @@ -437,6 +443,7 @@ static int rzg2l_adc_probe(struct platform_device
> > *pdev)
> >
> >  	adc = iio_priv(indio_dev);
> >
> > +	adc->max_channels = (uintptr_t)of_device_get_match_data(dev);
> 
> For IIO drivers, where possible please use the generic firmware
> properties.
> The driver already uses them for everything else. Hence
> 
> device_get_match_data(dev);

OK, will use device_get_match_data(dev);

> 
> >  	ret = rzg2l_adc_parse_properties(pdev, adc);
> >  	if (ret)
> >  		return ret;
> > @@ -540,7 +547,8 @@ static int rzg2l_adc_probe(struct platform_device
> > *pdev)  }
> >
> >  static const struct of_device_id rzg2l_adc_match[] = {
> > -	{ .compatible = "renesas,rzg2l-adc",},
> > +	{ .compatible = "renesas,r9a07g043-adc", .data = (void
> *)RZG2UL_ADC_MAX_CHANNELS },
> > +	{ .compatible = "renesas,rzg2l-adc", .data = (void
> > +*)RZG2L_ADC_MAX_CHANNELS },
> Whilst it is more work, I'd prefer that you introduce a small structure to
> represent chip type specific information then have an array of those.
> Finally store a pointer to an element of that array in here.

OK. will use SoC info structure pointing to the compatible.

> 
> We almost always end up needing to add more chip type specific data over
> time and so it is better to provide the means to do so flexibly in the
> first patch where such support is added.

Agreed.

Thanks,
Biju
> 
> >  	{ /* sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(of, rzg2l_adc_match);





[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux