On Sat, 2018-01-20 at 16:41 +0000, Jonathan Cameron wrote:
> On Thu, 18 Jan 2018 16:44:49 +0200
> <alexandru.ardelean@xxxxxxxxxx> wrote:
>
> > From: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> >
> > The external clock frequency was set only when selecting
> > the internal clock, which is fixed at 4.9152 Mhz.
> >
> > This is incorrect, since it should be set when any of
> > the external clock or crystal settings is selected.
> >
> > Added range validation for the validating the external
> > (crystal/clock) frequency setting.
> > Valid values are between 2.4576 and 5.12 Mhz.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
>
> A couple of minor points inline..
>
> Jonathan
>
> > ---
> >
> > Changes since v1:
> > * assign frequency from `pdata->ext_clk_hz`, not `pdata-
> > >ext_clk_hz`
>
> Err, my eye sight may be failing me, but those two look the same...
copy+paste goof ; correct is:
* assign frequency from `pdata->ext_clk_hz`, not `pdata-
>clock_source_sel`
>
> > * check frequency range for both crystal & (external) clock
> > * print error if clock range is invalid
> > * patch was split away from patch-series (for device-tree
> > bindings)
> >
> > drivers/staging/iio/adc/ad7192.c | 26 ++++++++++++++++++--------
> > 1 file changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7192.c
> > b/drivers/staging/iio/adc/ad7192.c
> > index aebbc3b58194..6d110f817b4e 100644
> > --- a/drivers/staging/iio/adc/ad7192.c
> > +++ b/drivers/staging/iio/adc/ad7192.c
> > @@ -141,6 +141,8 @@
> > #define AD7192_GPOCON_P1DAT BIT(1) /* P1 state */
> > #define AD7192_GPOCON_P0DAT BIT(0) /* P0 state */
> >
> > +#define AD7192_EXT_FREQ_MHZ_MIN 2457600
> > +#define AD7192_EXT_FREQ_MHZ_MAX 5120000
> > #define AD7192_INT_FREQ_MHZ 4915200
> >
> > /* NOTE:
> > @@ -217,6 +219,12 @@ static int ad7192_calibrate_all(struct
> > ad7192_state *st)
> > ARRAY_SIZE(ad7192_calib_arr));
> > }
> >
> > +static inline bool ad7192_valid_external_frequency(u32 freq)
> > +{
> > + return (freq >= AD7192_EXT_FREQ_MHZ_MIN &&
> > + freq <= AD7192_EXT_FREQ_MHZ_MAX);
> > +}
> > +
> > static int ad7192_setup(struct ad7192_state *st,
> > const struct ad7192_platform_data *pdata)
> > {
> > @@ -244,17 +252,19 @@ static int ad7192_setup(struct ad7192_state
> > *st,
> > id);
> >
> > switch (pdata->clock_source_sel) {
> > - case AD7192_CLK_EXT_MCLK1_2:
> > - case AD7192_CLK_EXT_MCLK2:
> > - st->mclk = AD7192_INT_FREQ_MHZ;
> > - break;
> > case AD7192_CLK_INT:
> > case AD7192_CLK_INT_CO:
> > - if (pdata->ext_clk_hz)
> > - st->mclk = pdata->ext_clk_hz;
> > - else
> > - st->mclk = AD7192_INT_FREQ_MHZ;
> > + st->mclk = AD7192_INT_FREQ_MHZ;
> > break;
> > + case AD7192_CLK_EXT_MCLK1_2:
> > + case AD7192_CLK_EXT_MCLK2:
> > + if (ad7192_valid_external_frequency(pdata-
> > >ext_clk_hz)) {
> > + st->mclk = pdata->ext_clk_hz;
> > + break;
> > + }
> > + dev_err(&st->sd.spi->dev, "Invalid frequency
> > setting %u\n",
> > + pdata->ext_clk_hz);
> > + /* FALLTHROUGH */
>
> Whilst it saves lines of code, the clarity of the code is reduced.
> Just put
> ret = -EINVAL;
> break;
ack
> > default:
> > ret = -EINVAL;
> > goto out;
>
> ��.n��������+%������w��{.n�����{��(��)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
