| Message ID | 20211031164603.4343-1-maslovdmitry@seeed.cc (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] iio: light: ltr501: Added ltr303 driver support | expand |
On Sun, Oct 31, 2021 at 6:46 PM Maslov Dmitry <maslovdmitry@seeed.cc> wrote: > > Previously ltr501 driver supported a number of light and, > proximity sensors including ltr501, ltr559 and ltr301. > This adds support for another light sensor ltr303 > used in Seeed Studio reTerminal, a carrier board > for Raspberry Pi 4 CM. > Signed-off-by: Maslov Dmitry <maslovdmitry@seeed.cc> I believe it should be Dmitry Maslov, i.e. First name Last name. ... > @@ -1597,6 +1610,7 @@ static const struct acpi_device_id ltr_acpi_match[] = { > {"LTER0501", ltr501}, > {"LTER0559", ltr559}, > {"LTER0301", ltr301}, > + {"LTER0303", ltr303}, Any evidence of this ACPI ID being in the wild, please?
> -----Original Messages----- > From: "Andy Shevchenko" <andy.shevchenko@gmail.com> > Sent Time: 2021-10-31 22:07:09 (Sunday) > To: "Maslov Dmitry" <maslovdmitry@seeed.cc> > Cc: "Jonathan Cameron" <jic23@kernel.org>, linux-iio <linux-iio@vger.kernel.org>, "Lars-Peter Clausen" <lars@metafoo.de>, north_sea@qq.com, baozhu.zuo@seeed.cc, jian.xiong@seeed.cc > Subject: Re: [PATCH v3] iio: light: ltr501: Added ltr303 driver support > > On Sun, Oct 31, 2021 at 6:46 PM Maslov Dmitry <maslovdmitry@seeed.cc> wrote: > > > > Previously ltr501 driver supported a number of light and, > > proximity sensors including ltr501, ltr559 and ltr301. > > This adds support for another light sensor ltr303 > > used in Seeed Studio reTerminal, a carrier board > > for Raspberry Pi 4 CM. > > > Signed-off-by: Maslov Dmitry <maslovdmitry@seeed.cc> > > I believe it should be Dmitry Maslov, i.e. First name Last name. I could change that, but I do not think there is an international standard for that. The reason I put Maslov Dmitry is to be in line with corporate email address, which belongs to a Chinese company - in China surname is put first on the legal documents. > ... > > > @@ -1597,6 +1610,7 @@ static const struct acpi_device_id ltr_acpi_match[] = { > > {"LTER0501", ltr501}, > > {"LTER0559", ltr559}, > > {"LTER0301", ltr301}, > > + {"LTER0303", ltr303}, > > Any evidence of this ACPI ID being in the wild, please? I'm sorry, I do not exactly understand the question. Do you mean where that particular sensor is used? > > -- > With Best Regards, > Andy Shevchenko
On Mon, Nov 1, 2021 at 3:57 PM Dmitry Maslov <maslovdmitry@seeed.cc> wrote: > > From: "Andy Shevchenko" <andy.shevchenko@gmail.com> > > Sent Time: 2021-10-31 22:07:09 (Sunday) > > On Sun, Oct 31, 2021 at 6:46 PM Maslov Dmitry <maslovdmitry@seeed.cc> wrote: ... > > > Signed-off-by: Maslov Dmitry <maslovdmitry@seeed.cc> > > > > I believe it should be Dmitry Maslov, i.e. First name Last name. > > I could change that, but I do not think there is an international standard for that. The reason I put Maslov Dmitry > is to be in line with corporate email address, which belongs to a Chinese company - in China surname is put first > on the legal documents. Hmm... You are right, there is no such requirement in the Submitting Patches document. ... > > > @@ -1597,6 +1610,7 @@ static const struct acpi_device_id ltr_acpi_match[] = { > > > {"LTER0501", ltr501}, > > > {"LTER0559", ltr559}, > > > {"LTER0301", ltr301}, > > > + {"LTER0303", ltr303}, > > > > Any evidence of this ACPI ID being in the wild, please? > > I'm sorry, I do not exactly understand the question. Do you mean where that particular sensor is used? Can you provide a name of the machine which has this ID in its DSDT table, please?
> -----Original Messages----- > From: "Andy Shevchenko" <andy.shevchenko@gmail.com> > Sent Time: 2021-11-01 16:20:35 (Monday) > To: "Dmitry Maslov" <maslovdmitry@seeed.cc> > Cc: "Jonathan Cameron" <jic23@kernel.org>, linux-iio <linux-iio@vger.kernel.org>, "Lars-Peter Clausen" <lars@metafoo.de>, north_sea@qq.com, baozhu.zuo@seeed.cc, jian.xiong@seeed.cc > Subject: Re: Re: [PATCH v3] iio: light: ltr501: Added ltr303 driver support > > On Mon, Nov 1, 2021 at 3:57 PM Dmitry Maslov <maslovdmitry@seeed.cc> wrote: > > > From: "Andy Shevchenko" <andy.shevchenko@gmail.com> > > > Sent Time: 2021-10-31 22:07:09 (Sunday) > > > On Sun, Oct 31, 2021 at 6:46 PM Maslov Dmitry <maslovdmitry@seeed.cc> wrote: > > ... > > > > > Signed-off-by: Maslov Dmitry <maslovdmitry@seeed.cc> > > > > > > I believe it should be Dmitry Maslov, i.e. First name Last name. > > > > I could change that, but I do not think there is an international standard for that. The reason I put Maslov Dmitry > > is to be in line with corporate email address, which belongs to a Chinese company - in China surname is put first > > on the legal documents. > > Hmm... You are right, there is no such requirement in the Submitting > Patches document. > ... > > > > > @@ -1597,6 +1610,7 @@ static const struct acpi_device_id ltr_acpi_match[] = { > > > > {"LTER0501", ltr501}, > > > > {"LTER0559", ltr559}, > > > > {"LTER0301", ltr301}, > > > > + {"LTER0303", ltr303}, > > > > > > Any evidence of this ACPI ID being in the wild, please? > > > > I'm sorry, I do not exactly understand the question. Do you mean where that particular sensor is used? > > Can you provide a name of the machine which has this ID in its DSDT > table, please? We're submitting this patch specifically for reTerminal. Here is DTS file for the reTerminal, currently awaiting merge in Raspberry Pi Linux kernel repository. https://github.com/raspberrypi/linux/blob/6ef732875d705ff15cc4c25d4d0a0eee87dc2a58/arch/arm/boot/dts/overlays/seeed-reterminal-core-overlay.dts#L99 So, while at the moment ACPI part is not being used, later we might use this sensor for other, x86 based boards, for example ODYSSEY - X86J4125800. Is there a particular reason you think this part should be omitted for LTR303?
On Mon, 1 Nov 2021 16:20:35 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Nov 1, 2021 at 3:57 PM Dmitry Maslov <maslovdmitry@seeed.cc> wrote: > > > From: "Andy Shevchenko" <andy.shevchenko@gmail.com> > > > Sent Time: 2021-10-31 22:07:09 (Sunday) > > > On Sun, Oct 31, 2021 at 6:46 PM Maslov Dmitry <maslovdmitry@seeed.cc> wrote: > > ... > > > > > Signed-off-by: Maslov Dmitry <maslovdmitry@seeed.cc> > > > > > > I believe it should be Dmitry Maslov, i.e. First name Last name. > > > > I could change that, but I do not think there is an international standard for that. The reason I put Maslov Dmitry > > is to be in line with corporate email address, which belongs to a Chinese company - in China surname is put first > > on the legal documents. > > Hmm... You are right, there is no such requirement in the Submitting > Patches document. Curiously we (Huawei) got push back on using conventional Chinese name ordering, so now submit everything with names swapped. I can't remember exactly which maintainer was determined we should do that... After some internal discussions we decided it was easier to just swap people's names than to argue about it. No one expressed any particularly strong opinions about the decision but perhaps people were just being polite. E.g. A random patch with sign-off from leizhen and xuwei. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b2bbc8687e43df456a6d194bc2c3ab9431b770cb Still I don't care or consider it a problem so accept whatever people prefer as long as it's in some sense their legal name for purposes of DCO. Jonathan > ... > > > > > @@ -1597,6 +1610,7 @@ static const struct acpi_device_id ltr_acpi_match[] = { > > > > {"LTER0501", ltr501}, > > > > {"LTER0559", ltr559}, > > > > {"LTER0301", ltr301}, > > > > + {"LTER0303", ltr303}, > > > > > > Any evidence of this ACPI ID being in the wild, please? > > > > I'm sorry, I do not exactly understand the question. Do you mean where that particular sensor is used? > > Can you provide a name of the machine which has this ID in its DSDT > table, please? >
On Tue, 2 Nov 2021 03:55:35 +0800 (GMT+08:00) "Dmitry Maslov" <maslovdmitry@seeed.cc> wrote: > > -----Original Messages----- > > From: "Andy Shevchenko" <andy.shevchenko@gmail.com> > > Sent Time: 2021-11-01 16:20:35 (Monday) > > To: "Dmitry Maslov" <maslovdmitry@seeed.cc> > > Cc: "Jonathan Cameron" <jic23@kernel.org>, linux-iio <linux-iio@vger.kernel.org>, "Lars-Peter Clausen" <lars@metafoo.de>, north_sea@qq.com, baozhu.zuo@seeed.cc, jian.xiong@seeed.cc > > Subject: Re: Re: [PATCH v3] iio: light: ltr501: Added ltr303 driver support > > > > On Mon, Nov 1, 2021 at 3:57 PM Dmitry Maslov <maslovdmitry@seeed.cc> wrote: > > > > From: "Andy Shevchenko" <andy.shevchenko@gmail.com> > > > > Sent Time: 2021-10-31 22:07:09 (Sunday) > > > > On Sun, Oct 31, 2021 at 6:46 PM Maslov Dmitry <maslovdmitry@seeed.cc> wrote: > > > > ... > > > > > > > Signed-off-by: Maslov Dmitry <maslovdmitry@seeed.cc> > > > > > > > > I believe it should be Dmitry Maslov, i.e. First name Last name. > > > > > > I could change that, but I do not think there is an international standard for that. The reason I put Maslov Dmitry > > > is to be in line with corporate email address, which belongs to a Chinese company - in China surname is put first > > > on the legal documents. > > > > Hmm... You are right, there is no such requirement in the Submitting > > Patches document. > > ... > > > > > > > @@ -1597,6 +1610,7 @@ static const struct acpi_device_id ltr_acpi_match[] = { > > > > > {"LTER0501", ltr501}, > > > > > {"LTER0559", ltr559}, > > > > > {"LTER0301", ltr301}, > > > > > + {"LTER0303", ltr303}, > > > > > > > > Any evidence of this ACPI ID being in the wild, please? > > > > > > I'm sorry, I do not exactly understand the question. Do you mean where that particular sensor is used? > > > > Can you provide a name of the machine which has this ID in its DSDT > > table, please? > > We're submitting this patch specifically for reTerminal. > Here is DTS file for the reTerminal, currently awaiting merge in Raspberry Pi Linux kernel > repository. > https://github.com/raspberrypi/linux/blob/6ef732875d705ff15cc4c25d4d0a0eee87dc2a58/arch/arm/boot/dts/overlays/seeed-reterminal-core-overlay.dts#L99 > > So, while at the moment ACPI part is not being used, later we might use this sensor for other, x86 based > boards, for example ODYSSEY - X86J4125800. > > Is there a particular reason you think this part should be omitted for LTR303? > ACPI IDs are supposed to be made up of either a PNP id or ACPI ID registered with UEFI forum. A 4 letter version is an ACPI ID (3 letters in PNP), so it should be in this table https://uefi.org/acpi_id_list It's not. So that means the proper process wasn't followed. If you were using this ID on a server, chances are we'd just tell you go fix your firmware (it's happened to me and we fixed it). However the sad truth is in consumer / embedded devices that may not be a practical solution. As such, if the ID was known to be in the wild we would probably let it in. Unfortunately I only really got familiar with ACPI specs in the last 4 years and before that time I didn't know what the rules were - so let a load of IDs in. Some of those IDs are in use on hardware that is out there so we have to continue supporting them or introduce a regression on that hardware. The process of applying for a PNP or ACPI ID isn't that bad for a company - you ask for a particular ID and request is then sent for a round of 'has anyone an objection' to the ASWG (I'm a rather inactive member so I see these every week or so). Instructions at https://uefi.org/PNP_ACPI_Registry Note that there would be two options for Seeed. Either you persuade liteon to apply and then issue an appropriate number (which may well not be the part number - often people just start counting from 0, or assign ranges to different people in the company so there doesn't need to be a central registry) or seeed applies for an ACPI or PNP ID and then issues IDs for any part you want to support on an ACPI platform that doesn't yet have an ID issued by the supplier. Note that it's also common to use someone else's ID. Once it's assigned to a device it can't be reused anyway so if say, Intel or Hisilicon had assigned one to a part already then you could just reuse it for your ACPI platforms. Hope that clears up how this is supposed to work. Also note that every now and then we 'guess' that IDs are just cut and paste jobs and remove them. So far we've only hit ones that were in actual use a few times - the majority of invalid ones weren't in use. Jonathan > > >
> From: "Jonathan Cameron" <jic23@kernel.org> > Sent Time: 2021-11-03 20:21:02 (Wednesday) > To: "Dmitry Maslov" <maslovdmitry@seeed.cc> > Cc: "Andy Shevchenko" <andy.shevchenko@gmail.com>, linux-iio <linux-iio@vger.kernel.org>, "Lars-Peter Clausen" <lars@metafoo.de>, north_sea@qq.com, baozhu.zuo@seeed.cc, jian.xiong@seeed.cc > Subject: Re: [PATCH v3] iio: light: ltr501: Added ltr303 driver support > > > > > > @@ -1597,6 +1610,7 @@ static const struct acpi_device_id ltr_acpi_match[] = { > > > > > > {"LTER0501", ltr501}, > > > > > > {"LTER0559", ltr559}, > > > > > > {"LTER0301", ltr301}, > > > > > > + {"LTER0303", ltr303}, > > > > > > > > > > Any evidence of this ACPI ID being in the wild, please? > > > > > > > > I'm sorry, I do not exactly understand the question. Do you mean where that particular sensor is used? > > > > > > Can you provide a name of the machine which has this ID in its DSDT > > > table, please? > > > > We're submitting this patch specifically for reTerminal. > > Here is DTS file for the reTerminal, currently awaiting merge in Raspberry Pi Linux kernel > > repository. > > https://github.com/raspberrypi/linux/blob/6ef732875d705ff15cc4c25d4d0a0eee87dc2a58/arch/arm/boot/dts/overlays/seeed-reterminal-core-overlay.dts#L99 > > > > So, while at the moment ACPI part is not being used, later we might use this sensor for other, x86 based > > boards, for example ODYSSEY - X86J4125800. > > > > Is there a particular reason you think this part should be omitted for LTR303? > > > ACPI IDs are supposed to be made up of either a PNP id or ACPI ID registered with > UEFI forum. > > A 4 letter version is an ACPI ID (3 letters in PNP), so it should be in this table > https://uefi.org/acpi_id_list > > It's not. So that means the proper process wasn't followed. If you were using this > ID on a server, chances are we'd just tell you go fix your firmware (it's happened > to me and we fixed it). However the sad truth is in consumer / embedded devices that > may not be a practical solution. As such, if the ID was known to be in the wild > we would probably let it in. > > Unfortunately I only really got familiar with ACPI specs in the last 4 years > and before that time I didn't know what the rules were - so let a load of IDs in. > Some of those IDs are in use on hardware that is out there so we have to continue > supporting them or introduce a regression on that hardware. > > The process of applying for a PNP or ACPI ID isn't that bad for a company - you ask > for a particular ID and request is then sent for a round of 'has anyone an objection' > to the ASWG (I'm a rather inactive member so I see these every week or so). > Instructions at https://uefi.org/PNP_ACPI_Registry > > Note that there would be two options for Seeed. Either you persuade liteon to apply > and then issue an appropriate number (which may well not be the part number - often > people just start counting from 0, or assign ranges to different people in the company > so there doesn't need to be a central registry) or seeed applies for an ACPI or PNP ID > and then issues IDs for any part you want to support on an ACPI platform that doesn't > yet have an ID issued by the supplier. > > Note that it's also common to use someone else's ID. Once it's assigned to a device > it can't be reused anyway so if say, Intel or Hisilicon had assigned one to a part > already then you could just reuse it for your ACPI platforms. > > Hope that clears up how this is supposed to work. > > Also note that every now and then we 'guess' that IDs are just cut and paste > jobs and remove them. So far we've only hit ones that were in actual use a > few times - the majority of invalid ones weren't in use. > Thank you for taking your time to write such detailed explanation! The whole situation with ACPI/PNP ID is much more clear to me now. As I mentioned above, currently our main goal is adding drivers necessary to support reTerminal, which is ARM processor based device. It means that as of now, we won't be using ACPI. Do you think it is valid option to just remove that part? That shouldn't affect ARM based devices ability to interact with the sensor.
On Fri, 5 Nov 2021 06:25:29 +0800 Dmitry Maslov <maslovdmitry@seeed.cc> wrote: > > From: "Jonathan Cameron" <jic23@kernel.org> > > Sent Time: 2021-11-03 20:21:02 (Wednesday) > > To: "Dmitry Maslov" <maslovdmitry@seeed.cc> > > Cc: "Andy Shevchenko" <andy.shevchenko@gmail.com>, linux-iio <linux-iio@vger.kernel.org>, "Lars-Peter Clausen" <lars@metafoo.de>, north_sea@qq.com, baozhu.zuo@seeed.cc, jian.xiong@seeed.cc > > Subject: Re: [PATCH v3] iio: light: ltr501: Added ltr303 driver support > > > > > > > > @@ -1597,6 +1610,7 @@ static const struct acpi_device_id ltr_acpi_match[] = { > > > > > > > {"LTER0501", ltr501}, > > > > > > > {"LTER0559", ltr559}, > > > > > > > {"LTER0301", ltr301}, > > > > > > > + {"LTER0303", ltr303}, > > > > > > > > > > > > Any evidence of this ACPI ID being in the wild, please? > > > > > > > > > > I'm sorry, I do not exactly understand the question. Do you mean where that particular sensor is used? > > > > > > > > Can you provide a name of the machine which has this ID in its DSDT > > > > table, please? > > > > > > We're submitting this patch specifically for reTerminal. > > > Here is DTS file for the reTerminal, currently awaiting merge in Raspberry Pi Linux kernel > > > repository. > > > https://github.com/raspberrypi/linux/blob/6ef732875d705ff15cc4c25d4d0a0eee87dc2a58/arch/arm/boot/dts/overlays/seeed-reterminal-core-overlay.dts#L99 > > > > > > So, while at the moment ACPI part is not being used, later we might use this sensor for other, x86 based > > > boards, for example ODYSSEY - X86J4125800. > > > > > > Is there a particular reason you think this part should be omitted for LTR303? > > > > > ACPI IDs are supposed to be made up of either a PNP id or ACPI ID registered with > > UEFI forum. > > > > A 4 letter version is an ACPI ID (3 letters in PNP), so it should be in this table > > https://uefi.org/acpi_id_list > > > > It's not. So that means the proper process wasn't followed. If you were using this > > ID on a server, chances are we'd just tell you go fix your firmware (it's happened > > to me and we fixed it). However the sad truth is in consumer / embedded devices that > > may not be a practical solution. As such, if the ID was known to be in the wild > > we would probably let it in. > > > > Unfortunately I only really got familiar with ACPI specs in the last 4 years > > and before that time I didn't know what the rules were - so let a load of IDs in. > > Some of those IDs are in use on hardware that is out there so we have to continue > > supporting them or introduce a regression on that hardware. > > > > The process of applying for a PNP or ACPI ID isn't that bad for a company - you ask > > for a particular ID and request is then sent for a round of 'has anyone an objection' > > to the ASWG (I'm a rather inactive member so I see these every week or so). > > Instructions at https://uefi.org/PNP_ACPI_Registry > > > > Note that there would be two options for Seeed. Either you persuade liteon to apply > > and then issue an appropriate number (which may well not be the part number - often > > people just start counting from 0, or assign ranges to different people in the company > > so there doesn't need to be a central registry) or seeed applies for an ACPI or PNP ID > > and then issues IDs for any part you want to support on an ACPI platform that doesn't > > yet have an ID issued by the supplier. > > > > Note that it's also common to use someone else's ID. Once it's assigned to a device > > it can't be reused anyway so if say, Intel or Hisilicon had assigned one to a part > > already then you could just reuse it for your ACPI platforms. > > > > Hope that clears up how this is supposed to work. > > > > Also note that every now and then we 'guess' that IDs are just cut and paste > > jobs and remove them. So far we've only hit ones that were in actual use a > > few times - the majority of invalid ones weren't in use. > > > Thank you for taking your time to write such detailed explanation! The whole situation > with ACPI/PNP ID is much more clear to me now. > As I mentioned above, currently our main goal is adding drivers necessary to support > reTerminal, which is ARM processor based device. It means that as of now, we > won't be using ACPI. Do you think it is valid option to just remove that part? > That shouldn't affect ARM based devices ability to interact with the sensor. > Absolutely. Just drop the ACPI table entry. Thanks, Jonathan
diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c index 7e51aaac0bf..4c8672547fd 100644 --- a/drivers/iio/light/ltr501.c +++ b/drivers/iio/light/ltr501.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * ltr501.c - Support for Lite-On LTR501 ambient light and proximity sensor + * Support for Lite-On LTR501 and similar ambient light and proximity sensors. * * Copyright 2014 Peter Meerwald <pmeerw@pmeerw.net> * @@ -98,6 +98,7 @@ enum { ltr501 = 0, ltr559, ltr301, + ltr303, }; struct ltr501_gain { @@ -1231,6 +1232,18 @@ static const struct ltr501_chip_info ltr501_chip_info_tbl[] = { .channels = ltr301_channels, .no_channels = ARRAY_SIZE(ltr301_channels), }, + [ltr303] = { + .partid = 0x0A, + .als_gain = ltr559_als_gain_tbl, + .als_gain_tbl_size = ARRAY_SIZE(ltr559_als_gain_tbl), + .als_mode_active = BIT(0), + .als_gain_mask = BIT(2) | BIT(3) | BIT(4), + .als_gain_shift = 2, + .info = <r301_info, + .info_no_irq = <r301_info_no_irq, + .channels = ltr301_channels, + .no_channels = ARRAY_SIZE(ltr301_channels), + }, }; static int ltr501_write_contr(struct ltr501_data *data, u8 als_val, u8 ps_val) @@ -1597,6 +1610,7 @@ static const struct acpi_device_id ltr_acpi_match[] = { {"LTER0501", ltr501}, {"LTER0559", ltr559}, {"LTER0301", ltr301}, + {"LTER0303", ltr303}, { }, }; MODULE_DEVICE_TABLE(acpi, ltr_acpi_match); @@ -1605,6 +1619,7 @@ static const struct i2c_device_id ltr501_id[] = { { "ltr501", ltr501}, { "ltr559", ltr559}, { "ltr301", ltr301}, + { "ltr303", ltr303}, { } }; MODULE_DEVICE_TABLE(i2c, ltr501_id); @@ -1613,6 +1628,7 @@ static const struct of_device_id ltr501_of_match[] = { { .compatible = "liteon,ltr501", }, { .compatible = "liteon,ltr559", }, { .compatible = "liteon,ltr301", }, + { .compatible = "liteon,ltr303", }, {} }; MODULE_DEVICE_TABLE(of, ltr501_of_match);
Previously ltr501 driver supported a number of light and, proximity sensors including ltr501, ltr559 and ltr301. This adds support for another light sensor ltr303 used in Seeed Studio reTerminal, a carrier board for Raspberry Pi 4 CM. Signed-off-by: Maslov Dmitry <maslovdmitry@seeed.cc> --- drivers/iio/light/ltr501.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) It is a fix of previous commit, that simplified ltr303 support and removed a lot of unnecessary code. Filename has also been removed from the file. Additionally, in v3 of the patch, unused ltr_max was removed and description of the driver was changed to more general, i.e. "LTR501 and similar ambient light and proximity sensors."