| Message ID | 20210927134153.12739-1-broonie@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: st_pressure_spi: Add missing entries SPI to device ID table | expand |
On Mon, 27 Sep 2021 14:41:53 +0100 Mark Brown <broonie@kernel.org> wrote: > Currently autoloading for SPI devices does not use the DT ID table, it uses > SPI modalises. Supporting OF modalises is going to be difficult if not > impractical, an attempt was made but has been reverted, so ensure that > module autoloading works for this driver by adding SPI IDs for parts that > only have a compatible listed. > > Fixes: 96c8395e2166 ("spi: Revert modalias changes") > Signed-off-by: Mark Brown <broonie@kernel.org> Whilst these IDs are deprecated, we should at least be consistent that they either work or not rather than current situation. +CC Denis as driver author. I'll let it sit on list a little longer so Denis can take a look. Thanks Jonathan > --- > drivers/iio/pressure/st_pressure_spi.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/iio/pressure/st_pressure_spi.c b/drivers/iio/pressure/st_pressure_spi.c > index 9b2523c5bc94..d6fc954e28f8 100644 > --- a/drivers/iio/pressure/st_pressure_spi.c > +++ b/drivers/iio/pressure/st_pressure_spi.c > @@ -97,6 +97,10 @@ static const struct spi_device_id st_press_id_table[] = { > { LPS33HW_PRESS_DEV_NAME }, > { LPS35HW_PRESS_DEV_NAME }, > { LPS22HH_PRESS_DEV_NAME }, > + { "lps001wp-press" }, > + { "lps25h-press", }, > + { "lps331ap-press" }, > + { "lps22hb-press" }, > {}, > }; > MODULE_DEVICE_TABLE(spi, st_press_id_table);
Hello Jonathan, Mark, I am not very familiar with how much the kernel would like to keep 'probing id' consistent. I perfectly understand the value of doing this (maintain ID compatibility) but I also see increase confusion in maintaining half in a way and half in another. I personally think that we should drop the '-press' thing for all the devices since they all are single-chip (meaning that the name used identify univocally that is a pressure sensor). If you think that compatibility is more important here, I think the patch is fine but this should be done in the i2c part as well so that it's at least congruent withing the driver. Let me know what you think. Thanks, Denis > -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Thursday, September 30, 2021 9:39 AM > To: Mark Brown <broonie@kernel.org> > Cc: Lars-Peter Clausen <lars@metafoo.de>; linux-iio@vger.kernel.org; Denis > CIOCCA <denis.ciocca@st.com> > Subject: Re: [PATCH] iio: st_pressure_spi: Add missing entries SPI to device ID > table > > On Mon, 27 Sep 2021 14:41:53 +0100 > Mark Brown <broonie@kernel.org> wrote: > > > Currently autoloading for SPI devices does not use the DT ID table, it > > uses SPI modalises. Supporting OF modalises is going to be difficult > > if not impractical, an attempt was made but has been reverted, so > > ensure that module autoloading works for this driver by adding SPI IDs > > for parts that only have a compatible listed. > > > > Fixes: 96c8395e2166 ("spi: Revert modalias changes") > > Signed-off-by: Mark Brown <broonie@kernel.org> > > Whilst these IDs are deprecated, we should at least be consistent that they > either work or not rather than current situation. > > +CC Denis as driver author. I'll let it sit on list a little longer so > +Denis can > take a look. > > Thanks > > Jonathan > > > --- > > drivers/iio/pressure/st_pressure_spi.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/iio/pressure/st_pressure_spi.c > > b/drivers/iio/pressure/st_pressure_spi.c > > index 9b2523c5bc94..d6fc954e28f8 100644 > > --- a/drivers/iio/pressure/st_pressure_spi.c > > +++ b/drivers/iio/pressure/st_pressure_spi.c > > @@ -97,6 +97,10 @@ static const struct spi_device_id st_press_id_table[] > = { > > { LPS33HW_PRESS_DEV_NAME }, > > { LPS35HW_PRESS_DEV_NAME }, > > { LPS22HH_PRESS_DEV_NAME }, > > + { "lps001wp-press" }, > > + { "lps25h-press", }, > > + { "lps331ap-press" }, > > + { "lps22hb-press" }, > > {}, > > }; > > MODULE_DEVICE_TABLE(spi, st_press_id_table);
On Thu, Sep 30, 2021 at 06:35:23PM +0000, Denis CIOCCA wrote: > I am not very familiar with how much the kernel would like to keep 'probing id' consistent. I perfectly understand the value of doing this (maintain ID compatibility) but I also see increase confusion in maintaining half in a way and half in another. The goal is not to maintain compatibility, the goal is to be able to load the driver as a module on DT systems. For historical reasons SPI uses the platform device IDs to load modules bound with DT, if there is no platform ID for a DT ID then userspace won't be able to find and load the module. > I personally think that we should drop the '-press' thing for all the devices since they all are single-chip (meaning that the name used identify univocally that is a pressure sensor). The DT bindings are an ABI, you can't really remove compatibles only deprecate them. > If you think that compatibility is more important here, I think the patch is fine but this should be done in the i2c part as well so that it's at least congruent withing the driver. I2C doesn't have this issue with modaliases so it's not an issue there. Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to.
Hello Mark, > -----Original Message----- > From: Mark Brown <broonie@kernel.org> > Sent: Friday, October 1, 2021 5:20 AM > To: Denis CIOCCA <denis.ciocca@st.com> > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > <lars@metafoo.de>; linux-iio@vger.kernel.org > Subject: Re: [PATCH] iio: st_pressure_spi: Add missing entries SPI to device > ID table > > On Thu, Sep 30, 2021 at 06:35:23PM +0000, Denis CIOCCA wrote: > > > I am not very familiar with how much the kernel would like to keep > 'probing id' consistent. I perfectly understand the value of doing this > (maintain ID compatibility) but I also see increase confusion in maintaining > half in a way and half in another. > > The goal is not to maintain compatibility, the goal is to be able to > load the driver as a module on DT systems. For historical reasons SPI > uses the platform device IDs to load modules bound with DT, if there is > no platform ID for a DT ID then userspace won't be able to find and load > the module. Ok now it is clear. I wasn't aware of that. In this case it is good to me (I didn't do any testing). > > > I personally think that we should drop the '-press' thing for all the devices > since they all are single-chip (meaning that the name used identify > univocally that is a pressure sensor). > > The DT bindings are an ABI, you can't really remove compatibles only > deprecate them. Yeah I guessed this was the case. > > > If you think that compatibility is more important here, I think the patch is > fine but this should be done in the i2c part as well so that it's at least > congruent withing the driver. > > I2C doesn't have this issue with modaliases so it's not an issue there. Clear. > > Please fix your mail client to word wrap within paragraphs at something > substantially less than 80 columns. Doing this makes your messages much > easier to read and reply to.
On Thu, 7 Oct 2021 00:47:56 +0000 Denis CIOCCA <denis.ciocca@st.com> wrote: > Hello Mark, > > > -----Original Message----- > > From: Mark Brown <broonie@kernel.org> > > Sent: Friday, October 1, 2021 5:20 AM > > To: Denis CIOCCA <denis.ciocca@st.com> > > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > > <lars@metafoo.de>; linux-iio@vger.kernel.org > > Subject: Re: [PATCH] iio: st_pressure_spi: Add missing entries SPI to device > > ID table > > > > On Thu, Sep 30, 2021 at 06:35:23PM +0000, Denis CIOCCA wrote: > > > > > I am not very familiar with how much the kernel would like to keep > > 'probing id' consistent. I perfectly understand the value of doing this > > (maintain ID compatibility) but I also see increase confusion in maintaining > > half in a way and half in another. > > > > The goal is not to maintain compatibility, the goal is to be able to > > load the driver as a module on DT systems. For historical reasons SPI > > uses the platform device IDs to load modules bound with DT, if there is > > no platform ID for a DT ID then userspace won't be able to find and load > > the module. > > Ok now it is clear. I wasn't aware of that. > In this case it is good to me (I didn't do any testing). Applied to the fixes-togreg branch of iio.git. I haven't explicitly marked it for stable but it may make sense to backport at somepoint. Thanks, Jonathan > > > > > > I personally think that we should drop the '-press' thing for all the devices > > since they all are single-chip (meaning that the name used identify > > univocally that is a pressure sensor). > > > > The DT bindings are an ABI, you can't really remove compatibles only > > deprecate them. > > Yeah I guessed this was the case. > > > > > > If you think that compatibility is more important here, I think the patch is > > fine but this should be done in the i2c part as well so that it's at least > > congruent withing the driver. > > > > I2C doesn't have this issue with modaliases so it's not an issue there. > > Clear. > > > > > Please fix your mail client to word wrap within paragraphs at something > > substantially less than 80 columns. Doing this makes your messages much > > easier to read and reply to.
diff --git a/drivers/iio/pressure/st_pressure_spi.c b/drivers/iio/pressure/st_pressure_spi.c index 9b2523c5bc94..d6fc954e28f8 100644 --- a/drivers/iio/pressure/st_pressure_spi.c +++ b/drivers/iio/pressure/st_pressure_spi.c @@ -97,6 +97,10 @@ static const struct spi_device_id st_press_id_table[] = { { LPS33HW_PRESS_DEV_NAME }, { LPS35HW_PRESS_DEV_NAME }, { LPS22HH_PRESS_DEV_NAME }, + { "lps001wp-press" }, + { "lps25h-press", }, + { "lps331ap-press" }, + { "lps22hb-press" }, {}, }; MODULE_DEVICE_TABLE(spi, st_press_id_table);
Currently autoloading for SPI devices does not use the DT ID table, it uses SPI modalises. Supporting OF modalises is going to be difficult if not impractical, an attempt was made but has been reverted, so ensure that module autoloading works for this driver by adding SPI IDs for parts that only have a compatible listed. Fixes: 96c8395e2166 ("spi: Revert modalias changes") Signed-off-by: Mark Brown <broonie@kernel.org> --- drivers/iio/pressure/st_pressure_spi.c | 4 ++++ 1 file changed, 4 insertions(+)