| Message ID | 20240717093034.9221-2-antoniu.miclaus@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] dt-bindings: iio: adf4377: add adf4378 link | expand |
On 17/07/2024 11:30, Antoniu Miclaus wrote: > Add separate handling for adf4378 within the driver. > > The main difference between adf4377 and adf4378 is that adf4378 has only > one output which is handled by only one gpio. > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com> > --- > drivers/iio/frequency/adf4377.c | 25 ++++++++++++++++++------- > 1 file changed, 18 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/frequency/adf4377.c b/drivers/iio/frequency/adf4377.c > index 9284c13f1abb..e02298a8b47f 100644 > --- a/drivers/iio/frequency/adf4377.c > +++ b/drivers/iio/frequency/adf4377.c > @@ -387,6 +387,11 @@ > #define ADF4377_FREQ_PFD_250MHZ (250 * HZ_PER_MHZ) > #define ADF4377_FREQ_PFD_320MHZ (320 * HZ_PER_MHZ) > > +enum adf4377_dev_type { > + ADF4377, > + ADF4378, > +}; > + > enum { > ADF4377_FREQ, > }; > @@ -402,6 +407,7 @@ enum muxout_select_mode { > > struct adf4377_state { > struct spi_device *spi; > + enum adf4377_dev_type type; > struct regmap *regmap; > struct clk *clkin; > /* Protect against concurrent accesses to the device and data content */ > @@ -687,7 +693,7 @@ static void adf4377_gpio_init(struct adf4377_state *st) > if (st->gpio_enclk1) > gpiod_set_value(st->gpio_enclk1, 1); > > - if (st->gpio_enclk2) > + if (st->gpio_enclk2 && st->type == ADF4377) Why? Isn't everything correct for NULL? > gpiod_set_value(st->gpio_enclk2, 1); > } > > @@ -889,11 +895,13 @@ static int adf4377_properties_parse(struct adf4377_state *st) > return dev_err_probe(&spi->dev, PTR_ERR(st->gpio_enclk1), > "failed to get the CE GPIO\n"); > > - st->gpio_enclk2 = devm_gpiod_get_optional(&st->spi->dev, "clk2-enable", > - GPIOD_OUT_LOW); > - if (IS_ERR(st->gpio_enclk2)) > - return dev_err_probe(&spi->dev, PTR_ERR(st->gpio_enclk2), > - "failed to get the CE GPIO\n"); > + if (st->type == ADF4377) { So the device does not have this pin? Then you should express it in the bindings. > + st->gpio_enclk2 = devm_gpiod_get_optional(&st->spi->dev, "clk2-enable", > + GPIOD_OUT_LOW); > + if (IS_ERR(st->gpio_enclk2)) > + return dev_err_probe(&spi->dev, PTR_ERR(st->gpio_enclk2), > + "failed to get the CE GPIO\n"); > + } > > ret = device_property_match_property_string(&spi->dev, "adi,muxout-select", > adf4377_muxout_modes, > @@ -945,6 +953,7 @@ static int adf4377_probe(struct spi_device *spi) > > st->regmap = regmap; > st->spi = spi; > + st->type = spi_get_device_id(spi)->driver_data; spi_get_device_match_data() > mutex_init(&st->lock); > > ret = adf4377_properties_parse(st); > @@ -964,13 +973,15 @@ static int adf4377_probe(struct spi_device *spi) > } > > static const struct spi_device_id adf4377_id[] = { > - { "adf4377", 0 }, > + { "adf4377", ADF4377 }, > + { "adf4378", ADF4378 }, > {} > }; > MODULE_DEVICE_TABLE(spi, adf4377_id); > > static const struct of_device_id adf4377_of_match[] = { > { .compatible = "adi,adf4377" }, > + { .compatible = "adi,adf4378" }, Your device ID tables have incoherent match data. Considering that one type is 0, this is error-prone and discouraged. Best regards, Krzysztof
On Wed, 17 Jul 2024 11:38:30 +0200 Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 17/07/2024 11:30, Antoniu Miclaus wrote: > > Add separate handling for adf4378 within the driver. > > > > The main difference between adf4377 and adf4378 is that adf4378 has only > > one output which is handled by only one gpio. > > > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com> Replying on top of Krzysztof's review as he is raising very similar points to those I was going to make. > > --- > > drivers/iio/frequency/adf4377.c | 25 ++++++++++++++++++------- > > 1 file changed, 18 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/iio/frequency/adf4377.c b/drivers/iio/frequency/adf4377.c > > index 9284c13f1abb..e02298a8b47f 100644 > > --- a/drivers/iio/frequency/adf4377.c > > +++ b/drivers/iio/frequency/adf4377.c > > @@ -387,6 +387,11 @@ > > #define ADF4377_FREQ_PFD_250MHZ (250 * HZ_PER_MHZ) > > #define ADF4377_FREQ_PFD_320MHZ (320 * HZ_PER_MHZ) > > > > +enum adf4377_dev_type { > > + ADF4377, > > + ADF4378, > > +}; See below - but using an enum for device type is normally a bad sign. It means you are adding a bunch of code paths that will need continual extension as new chips are added. Much better to add a description of chip features in a const structure. > > + > > enum { > > ADF4377_FREQ, > > }; > > @@ -402,6 +407,7 @@ enum muxout_select_mode { > > > > struct adf4377_state { > > struct spi_device *spi; > > + enum adf4377_dev_type type; > > struct regmap *regmap; > > struct clk *clkin; > > /* Protect against concurrent accesses to the device and data content */ > > @@ -687,7 +693,7 @@ static void adf4377_gpio_init(struct adf4377_state *st) > > if (st->gpio_enclk1) > > gpiod_set_value(st->gpio_enclk1, 1); > > > > - if (st->gpio_enclk2) > > + if (st->gpio_enclk2 && st->type == ADF4377) > > Why? Isn't everything correct for NULL? > > > gpiod_set_value(st->gpio_enclk2, 1); > > } > > > > @@ -889,11 +895,13 @@ static int adf4377_properties_parse(struct adf4377_state *st) > > return dev_err_probe(&spi->dev, PTR_ERR(st->gpio_enclk1), > > "failed to get the CE GPIO\n"); > > > > - st->gpio_enclk2 = devm_gpiod_get_optional(&st->spi->dev, "clk2-enable", > > - GPIOD_OUT_LOW); > > - if (IS_ERR(st->gpio_enclk2)) > > - return dev_err_probe(&spi->dev, PTR_ERR(st->gpio_enclk2), > > - "failed to get the CE GPIO\n"); > > + if (st->type == ADF4377) { > > So the device does not have this pin? Then you should express it in the > bindings. Agreed: That binding needs to ensure that there isn't a second pin expressed for a chip where it makes no sense. > > > + st->gpio_enclk2 = devm_gpiod_get_optional(&st->spi->dev, "clk2-enable", > > + GPIOD_OUT_LOW); > > + if (IS_ERR(st->gpio_enclk2)) > > + return dev_err_probe(&spi->dev, PTR_ERR(st->gpio_enclk2), > > + "failed to get the CE GPIO\n"); > > + } > > > > ret = device_property_match_property_string(&spi->dev, "adi,muxout-select", > > adf4377_muxout_modes, > > @@ -945,6 +953,7 @@ static int adf4377_probe(struct spi_device *spi) > > > > st->regmap = regmap; > > st->spi = spi; > > + st->type = spi_get_device_id(spi)->driver_data; > > > spi_get_device_match_data() > > > mutex_init(&st->lock); > > > > ret = adf4377_properties_parse(st); > > @@ -964,13 +973,15 @@ static int adf4377_probe(struct spi_device *spi) > > } > > > > static const struct spi_device_id adf4377_id[] = { > > - { "adf4377", 0 }, > > + { "adf4377", ADF4377 }, > > + { "adf4378", ADF4378 }, > > {} > > }; > > MODULE_DEVICE_TABLE(spi, adf4377_id); > > > > static const struct of_device_id adf4377_of_match[] = { > > { .compatible = "adi,adf4377" }, > > + { .compatible = "adi,adf4378" }, > > Your device ID tables have incoherent match data. Considering that one > type is 0, this is error-prone and discouraged. Agreed. Much better to use a pointer to a chip specific structure for these thus avoiding the accidental NULL value and turning chip differences into data, not code. > > Best regards, > Krzysztof >
diff --git a/drivers/iio/frequency/adf4377.c b/drivers/iio/frequency/adf4377.c index 9284c13f1abb..e02298a8b47f 100644 --- a/drivers/iio/frequency/adf4377.c +++ b/drivers/iio/frequency/adf4377.c @@ -387,6 +387,11 @@ #define ADF4377_FREQ_PFD_250MHZ (250 * HZ_PER_MHZ) #define ADF4377_FREQ_PFD_320MHZ (320 * HZ_PER_MHZ) +enum adf4377_dev_type { + ADF4377, + ADF4378, +}; + enum { ADF4377_FREQ, }; @@ -402,6 +407,7 @@ enum muxout_select_mode { struct adf4377_state { struct spi_device *spi; + enum adf4377_dev_type type; struct regmap *regmap; struct clk *clkin; /* Protect against concurrent accesses to the device and data content */ @@ -687,7 +693,7 @@ static void adf4377_gpio_init(struct adf4377_state *st) if (st->gpio_enclk1) gpiod_set_value(st->gpio_enclk1, 1); - if (st->gpio_enclk2) + if (st->gpio_enclk2 && st->type == ADF4377) gpiod_set_value(st->gpio_enclk2, 1); } @@ -889,11 +895,13 @@ static int adf4377_properties_parse(struct adf4377_state *st) return dev_err_probe(&spi->dev, PTR_ERR(st->gpio_enclk1), "failed to get the CE GPIO\n"); - st->gpio_enclk2 = devm_gpiod_get_optional(&st->spi->dev, "clk2-enable", - GPIOD_OUT_LOW); - if (IS_ERR(st->gpio_enclk2)) - return dev_err_probe(&spi->dev, PTR_ERR(st->gpio_enclk2), - "failed to get the CE GPIO\n"); + if (st->type == ADF4377) { + st->gpio_enclk2 = devm_gpiod_get_optional(&st->spi->dev, "clk2-enable", + GPIOD_OUT_LOW); + if (IS_ERR(st->gpio_enclk2)) + return dev_err_probe(&spi->dev, PTR_ERR(st->gpio_enclk2), + "failed to get the CE GPIO\n"); + } ret = device_property_match_property_string(&spi->dev, "adi,muxout-select", adf4377_muxout_modes, @@ -945,6 +953,7 @@ static int adf4377_probe(struct spi_device *spi) st->regmap = regmap; st->spi = spi; + st->type = spi_get_device_id(spi)->driver_data; mutex_init(&st->lock); ret = adf4377_properties_parse(st); @@ -964,13 +973,15 @@ static int adf4377_probe(struct spi_device *spi) } static const struct spi_device_id adf4377_id[] = { - { "adf4377", 0 }, + { "adf4377", ADF4377 }, + { "adf4378", ADF4378 }, {} }; MODULE_DEVICE_TABLE(spi, adf4377_id); static const struct of_device_id adf4377_of_match[] = { { .compatible = "adi,adf4377" }, + { .compatible = "adi,adf4378" }, {} }; MODULE_DEVICE_TABLE(of, adf4377_of_match);
Add separate handling for adf4378 within the driver. The main difference between adf4377 and adf4378 is that adf4378 has only one output which is handled by only one gpio. Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com> --- drivers/iio/frequency/adf4377.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)