| Message ID | 20210329174928.18816-3-henning.schild@siemens.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
Am Tue, 30 Mar 2021 14:04:35 +0300 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild > <henning.schild@siemens.com> wrote: > > > > This driver adds initial support for several devices from Siemens. > > It is based on a platform driver introduced in an earlier commit. > > ... > > > +#define SIMATIC_IPC_LED_PORT_BASE 0x404E > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > + {1 << 15, "green:" LED_FUNCTION_STATUS "-1" }, > > + {1 << 7, "yellow:" LED_FUNCTION_STATUS "-1" }, > > + {1 << 14, "red:" LED_FUNCTION_STATUS "-2" }, > > + {1 << 6, "yellow:" LED_FUNCTION_STATUS "-2" }, > > + {1 << 13, "red:" LED_FUNCTION_STATUS "-3" }, > > + {1 << 5, "yellow:" LED_FUNCTION_STATUS "-3" }, > > + { } > > +}; > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"}, > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"}, > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"}, > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"}, > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"}, > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"}, > > + { } > > +}; > > It seems to me like poking GPIO controller registers directly. This > is not good. The question still remains: Can we simply register a > GPIO (pin control) driver and use an LED GPIO driver with an > additional board file that instantiates it? I wrote about that in reply to the cover letter. My view is still that it would be an abstraction with only one user, just causing work and likely not ending up as generic as it might eventually have to be. The region is reserved, not sure what the problem with the "poking" is. Maybe i do not understand all the benefits of such a split at this point in time. At the moment i only see work with hardly any benefit, not just work for me but also for maintainers. I sure do not mean to be ignorant. Maybe you go into details and convince me or we wait for other peoples opinions on how to proceed, maybe there is a second user that i am not aware of? Until i am convinced otherwise i will try to argue that a single-user-abstraction is needless work/code, and should be done only when actually needed. regards, Henning
On Tue, Mar 30, 2021 at 2:58 PM Henning Schild <henning.schild@siemens.com> wrote: > Am Tue, 30 Mar 2021 14:04:35 +0300 > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild > > <henning.schild@siemens.com> wrote: > > > > > > This driver adds initial support for several devices from Siemens. > > > It is based on a platform driver introduced in an earlier commit. > > > > ... > > > > > +#define SIMATIC_IPC_LED_PORT_BASE 0x404E > > > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > > + {1 << 15, "green:" LED_FUNCTION_STATUS "-1" }, > > > + {1 << 7, "yellow:" LED_FUNCTION_STATUS "-1" }, > > > + {1 << 14, "red:" LED_FUNCTION_STATUS "-2" }, > > > + {1 << 6, "yellow:" LED_FUNCTION_STATUS "-2" }, > > > + {1 << 13, "red:" LED_FUNCTION_STATUS "-3" }, > > > + {1 << 5, "yellow:" LED_FUNCTION_STATUS "-3" }, > > > + { } > > > +}; > > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"}, > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"}, > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"}, > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"}, > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"}, > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"}, > > > + { } > > > +}; > > > > It seems to me like poking GPIO controller registers directly. This > > is not good. The question still remains: Can we simply register a > > GPIO (pin control) driver and use an LED GPIO driver with an > > additional board file that instantiates it? > > I wrote about that in reply to the cover letter. My view is still that > it would be an abstraction with only one user, just causing work and > likely not ending up as generic as it might eventually have to be. > > The region is reserved, not sure what the problem with the "poking" is. > Maybe i do not understand all the benefits of such a split at this > point in time. At the moment i only see work with hardly any benefit, > not just work for me but also for maintainers. I sure do not mean to be > ignorant. Maybe you go into details and convince me or we wait for other > peoples opinions on how to proceed, maybe there is a second user that i > am not aware of? > Until i am convinced otherwise i will try to argue that a > single-user-abstraction is needless work/code, and should be done only > when actually needed. I have just read your messages (there is a cover letter and additional email which was sent lately). I would like to know what the CPU model number on that board is. Than we can continue to see what possibilities we have here. -- With Best Regards, Andy Shevchenko
Am Tue, 30 Mar 2021 15:15:16 +0300 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild > <henning.schild@siemens.com> wrote: > > Am Tue, 30 Mar 2021 14:04:35 +0300 > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild > > > <henning.schild@siemens.com> wrote: > > > > > > > > This driver adds initial support for several devices from > > > > Siemens. It is based on a platform driver introduced in an > > > > earlier commit. > > > > > > ... > > > > > > > +#define SIMATIC_IPC_LED_PORT_BASE 0x404E > > > > > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > > > + {1 << 15, "green:" LED_FUNCTION_STATUS "-1" }, > > > > + {1 << 7, "yellow:" LED_FUNCTION_STATUS "-1" }, > > > > + {1 << 14, "red:" LED_FUNCTION_STATUS "-2" }, > > > > + {1 << 6, "yellow:" LED_FUNCTION_STATUS "-2" }, > > > > + {1 << 13, "red:" LED_FUNCTION_STATUS "-3" }, > > > > + {1 << 5, "yellow:" LED_FUNCTION_STATUS "-3" }, > > > > + { } > > > > +}; > > > > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"}, > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"}, > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"}, > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"}, > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"}, > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"}, > > > > + { } > > > > +}; > > > > > > It seems to me like poking GPIO controller registers directly. > > > This is not good. The question still remains: Can we simply > > > register a GPIO (pin control) driver and use an LED GPIO driver > > > with an additional board file that instantiates it? > > > > I wrote about that in reply to the cover letter. My view is still > > that it would be an abstraction with only one user, just causing > > work and likely not ending up as generic as it might eventually > > have to be. > > > > The region is reserved, not sure what the problem with the "poking" > > is. > > > > Maybe i do not understand all the benefits of such a split at this > > point in time. At the moment i only see work with hardly any > > benefit, not just work for me but also for maintainers. I sure do > > not mean to be ignorant. Maybe you go into details and convince me > > or we wait for other peoples opinions on how to proceed, maybe > > there is a second user that i am not aware of? > > Until i am convinced otherwise i will try to argue that a > > single-user-abstraction is needless work/code, and should be done > > only when actually needed. > > I have just read your messages (there is a cover letter and additional > email which was sent lately). > > I would like to know what the CPU model number on that board is. Than > we can continue to see what possibilities we have here. I guess we are talking about the one that uses memory mapped, that is called an "IPC127E" and seems to have either Intel Atom E3940 or E3930 which seems to be Apollo Lake. Henning
On Tue, Mar 30, 2021 at 3:35 PM Henning Schild <henning.schild@siemens.com> wrote: > Am Tue, 30 Mar 2021 15:15:16 +0300 > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild > > <henning.schild@siemens.com> wrote: > > > Am Tue, 30 Mar 2021 14:04:35 +0300 > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild > > > > <henning.schild@siemens.com> wrote: > > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"}, > > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"}, > > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"}, > > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"}, > > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"}, > > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"}, > > > > > + { } > > > > > +}; > > > > > > > > It seems to me like poking GPIO controller registers directly. > > > > This is not good. The question still remains: Can we simply > > > > register a GPIO (pin control) driver and use an LED GPIO driver > > > > with an additional board file that instantiates it? > > > > > > I wrote about that in reply to the cover letter. My view is still > > > that it would be an abstraction with only one user, just causing > > > work and likely not ending up as generic as it might eventually > > > have to be. > > > > > > The region is reserved, not sure what the problem with the "poking" > > > is. > > > > > > > Maybe i do not understand all the benefits of such a split at this > > > point in time. At the moment i only see work with hardly any > > > benefit, not just work for me but also for maintainers. I sure do > > > not mean to be ignorant. Maybe you go into details and convince me > > > or we wait for other peoples opinions on how to proceed, maybe > > > there is a second user that i am not aware of? > > > Until i am convinced otherwise i will try to argue that a > > > single-user-abstraction is needless work/code, and should be done > > > only when actually needed. > > > > I have just read your messages (there is a cover letter and additional > > email which was sent lately). > > > > I would like to know what the CPU model number on that board is. Than > > we can continue to see what possibilities we have here. > > I guess we are talking about the one that uses memory mapped, that is > called an "IPC127E" and seems to have either Intel Atom E3940 or E3930 > which seems to be Apollo Lake. Yep. And now the question, in my patch series you should have got the apollolake-pinctrl driver loaded (if not, we have to investigate why it's not being instantiated). This will give you a read GPIO driver. So, you may use regular LED GPIO on top of it (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c). I would like to understand why it can't be achieved. -- With Best Regards, Andy Shevchenko
Am Tue, 30 Mar 2021 15:41:53 +0300 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild > <henning.schild@siemens.com> wrote: > > Am Tue, 30 Mar 2021 15:15:16 +0300 > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild > > > <henning.schild@siemens.com> wrote: > > > > Am Tue, 30 Mar 2021 14:04:35 +0300 > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild > > > > > <henning.schild@siemens.com> wrote: > > > > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > > > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"}, > > > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"}, > > > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"}, > > > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"}, > > > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"}, > > > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"}, > > > > > > + { } > > > > > > +}; > > > > > > > > > > It seems to me like poking GPIO controller registers directly. > > > > > This is not good. The question still remains: Can we simply > > > > > register a GPIO (pin control) driver and use an LED GPIO > > > > > driver with an additional board file that instantiates it? > > > > > > > > I wrote about that in reply to the cover letter. My view is > > > > still that it would be an abstraction with only one user, just > > > > causing work and likely not ending up as generic as it might > > > > eventually have to be. > > > > > > > > The region is reserved, not sure what the problem with the > > > > "poking" is. > > > > > > > > > > Maybe i do not understand all the benefits of such a split at > > > > this point in time. At the moment i only see work with hardly > > > > any benefit, not just work for me but also for maintainers. I > > > > sure do not mean to be ignorant. Maybe you go into details and > > > > convince me or we wait for other peoples opinions on how to > > > > proceed, maybe there is a second user that i am not aware of? > > > > Until i am convinced otherwise i will try to argue that a > > > > single-user-abstraction is needless work/code, and should be > > > > done only when actually needed. > > > > > > I have just read your messages (there is a cover letter and > > > additional email which was sent lately). > > > > > > I would like to know what the CPU model number on that board is. > > > Than we can continue to see what possibilities we have here. > > > > I guess we are talking about the one that uses memory mapped, that > > is called an "IPC127E" and seems to have either Intel Atom E3940 or > > E3930 which seems to be Apollo Lake. > > Yep. And now the question, in my patch series you should have got the > apollolake-pinctrl driver loaded (if not, we have to investigate why > it's not being instantiated). This will give you a read GPIO driver. Ok, so there is the existing driver i asked about several times. Thanks for pointing it out. > So, you may use regular LED GPIO on top of it > (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c). > I would like to understand why it can't be achieved. Will have a look. Unfortunately this one box is missing in my personal collection, but let us assume that one can be converted to that existing driver. I guess that will still mean the PIO-based part of the LED driver will have to stay as is. regards, Henning
On Tue, Mar 30, 2021 at 6:33 PM Henning Schild <henning.schild@siemens.com> wrote: > Am Tue, 30 Mar 2021 15:41:53 +0300 > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild > > <henning.schild@siemens.com> wrote: > > > Am Tue, 30 Mar 2021 15:15:16 +0300 > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild > > > > <henning.schild@siemens.com> wrote: > > > > > Am Tue, 30 Mar 2021 14:04:35 +0300 > > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild > > > > > > <henning.schild@siemens.com> wrote: > > > > > > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > > > > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"}, > > > > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"}, > > > > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"}, > > > > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"}, > > > > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"}, > > > > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"}, > > > > > > > + { } > > > > > > > +}; > > > > > > > > > > > > It seems to me like poking GPIO controller registers directly. > > > > > > This is not good. The question still remains: Can we simply > > > > > > register a GPIO (pin control) driver and use an LED GPIO > > > > > > driver with an additional board file that instantiates it? > > > > > > > > > > I wrote about that in reply to the cover letter. My view is > > > > > still that it would be an abstraction with only one user, just > > > > > causing work and likely not ending up as generic as it might > > > > > eventually have to be. > > > > > > > > > > The region is reserved, not sure what the problem with the > > > > > "poking" is. > > > > > > > > > > > > > Maybe i do not understand all the benefits of such a split at > > > > > this point in time. At the moment i only see work with hardly > > > > > any benefit, not just work for me but also for maintainers. I > > > > > sure do not mean to be ignorant. Maybe you go into details and > > > > > convince me or we wait for other peoples opinions on how to > > > > > proceed, maybe there is a second user that i am not aware of? > > > > > Until i am convinced otherwise i will try to argue that a > > > > > single-user-abstraction is needless work/code, and should be > > > > > done only when actually needed. > > > > > > > > I have just read your messages (there is a cover letter and > > > > additional email which was sent lately). > > > > > > > > I would like to know what the CPU model number on that board is. > > > > Than we can continue to see what possibilities we have here. > > > > > > I guess we are talking about the one that uses memory mapped, that > > > is called an "IPC127E" and seems to have either Intel Atom E3940 or > > > E3930 which seems to be Apollo Lake. > > > > Yep. And now the question, in my patch series you should have got the > > apollolake-pinctrl driver loaded (if not, we have to investigate why > > it's not being instantiated). This will give you a read GPIO driver. > > Ok, so there is the existing driver i asked about several times. Thanks > for pointing it out. If you remember, I asked you about the chip twice :-) I assumed that we were talking about Apollo Lake and that's why I insisted that the driver is in the kernel source tree. > > So, you may use regular LED GPIO on top of it > > (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c). > > I would like to understand why it can't be achieved. > > Will have a look. Unfortunately this one box is missing in my personal > collection, but let us assume that one can be converted to that > existing driver. OK! > I guess that will still mean the PIO-based part of the LED driver will > have to stay as is. Probably yes. I haven't looked into that part and I have no idea what's going on on that platform(s). -- With Best Regards, Andy Shevchenko
On Thu, Apr 1, 2021 at 1:44 PM Henning Schild <henning.schild@siemens.com> wrote: > > Am Wed, 31 Mar 2021 18:40:23 +0300 > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > On Tue, Mar 30, 2021 at 6:33 PM Henning Schild > > <henning.schild@siemens.com> wrote: > > > Am Tue, 30 Mar 2021 15:41:53 +0300 > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild > > > > <henning.schild@siemens.com> wrote: > > > > > Am Tue, 30 Mar 2021 15:15:16 +0300 > > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild > > > > > > <henning.schild@siemens.com> wrote: > > > > > > > Am Tue, 30 Mar 2021 14:04:35 +0300 > > > > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild > > > > > > > > <henning.schild@siemens.com> wrote: > > > > > > > > > > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = > > > > > > > > > { > > > > > > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS > > > > > > > > > "-1"}, > > > > > > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS > > > > > > > > > "-1"}, > > > > > > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS > > > > > > > > > "-2"}, > > > > > > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS > > > > > > > > > "-2"}, > > > > > > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS > > > > > > > > > "-3"}, > > > > > > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS > > > > > > > > > "-3"}, > > > > > > > > > + { } > > > > > > > > > +}; > > > > > > > > > > > > > > > > It seems to me like poking GPIO controller registers > > > > > > > > directly. This is not good. The question still remains: > > > > > > > > Can we simply register a GPIO (pin control) driver and > > > > > > > > use an LED GPIO driver with an additional board file that > > > > > > > > instantiates it? > > > > > > > > > > > > > > I wrote about that in reply to the cover letter. My view is > > > > > > > still that it would be an abstraction with only one user, > > > > > > > just causing work and likely not ending up as generic as it > > > > > > > might eventually have to be. > > > > > > > > > > > > > > The region is reserved, not sure what the problem with the > > > > > > > "poking" is. > > > > > > > > > > > > > > > > > > > Maybe i do not understand all the benefits of such a split > > > > > > > at this point in time. At the moment i only see work with > > > > > > > hardly any benefit, not just work for me but also for > > > > > > > maintainers. I sure do not mean to be ignorant. Maybe you > > > > > > > go into details and convince me or we wait for other > > > > > > > peoples opinions on how to proceed, maybe there is a second > > > > > > > user that i am not aware of? Until i am convinced otherwise > > > > > > > i will try to argue that a single-user-abstraction is > > > > > > > needless work/code, and should be done only when actually > > > > > > > needed. > > > > > > > > > > > > I have just read your messages (there is a cover letter and > > > > > > additional email which was sent lately). > > > > > > > > > > > > I would like to know what the CPU model number on that board > > > > > > is. Than we can continue to see what possibilities we have > > > > > > here. > > > > > > > > > > I guess we are talking about the one that uses memory mapped, > > > > > that is called an "IPC127E" and seems to have either Intel Atom > > > > > E3940 or E3930 which seems to be Apollo Lake. > > > > > > > > Yep. And now the question, in my patch series you should have got > > > > the apollolake-pinctrl driver loaded (if not, we have to > > > > investigate why it's not being instantiated). This will give you > > > > a read GPIO driver. > > > > > > Ok, so there is the existing driver i asked about several times. > > > Thanks for pointing it out. > > > > If you remember, I asked you about the chip twice :-) > > I assumed that we were talking about Apollo Lake and that's why I > > insisted that the driver is in the kernel source tree. > > Sorry, maybe i did not get the context of your question and which of > the machines you asked about. Now it is clear i guess. > > > > > > > So, you may use regular LED GPIO on top of it > > > > (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c). > > > > I would like to understand why it can't be achieved. > > > > > > Will have a look. Unfortunately this one box is missing in my > > > personal collection, but let us assume that one can be converted to > > > that existing driver. > > > > OK! > > > > > I guess that will still mean the PIO-based part of the LED driver > > > will have to stay as is. > > > > Probably yes. I haven't looked into that part and I have no idea > > what's going on on that platform(s). > > > > Which i guess means the series can be reviewed as if the mmio bits for > that apollo lake would not be in it, maybe i will even send a version > without that one box. We have others in the "backlog" might as well > delay that one if it helps sorting out a base-line. It depends on the role of P2SB in this case. Shouldn't you drop that completely out from this series? Otherwise we have to understand what to do with it. It seems the best approach can be to expose the P2SB device to Linux, but we have to answer to Bjorn's request about region reservations.
Am Thu, 1 Apr 2021 14:04:51 +0300 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Thu, Apr 1, 2021 at 1:44 PM Henning Schild > <henning.schild@siemens.com> wrote: > > > > Am Wed, 31 Mar 2021 18:40:23 +0300 > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > > On Tue, Mar 30, 2021 at 6:33 PM Henning Schild > > > <henning.schild@siemens.com> wrote: > > > > Am Tue, 30 Mar 2021 15:41:53 +0300 > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild > > > > > <henning.schild@siemens.com> wrote: > > > > > > Am Tue, 30 Mar 2021 15:15:16 +0300 > > > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild > > > > > > > <henning.schild@siemens.com> wrote: > > > > > > > > Am Tue, 30 Mar 2021 14:04:35 +0300 > > > > > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild > > > > > > > > > <henning.schild@siemens.com> wrote: > > > > > > > > > > > > > > > +static struct simatic_ipc_led > > > > > > > > > > simatic_ipc_leds_mem[] = { > > > > > > > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS > > > > > > > > > > "-1"}, > > > > > > > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS > > > > > > > > > > "-1"}, > > > > > > > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS > > > > > > > > > > "-2"}, > > > > > > > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS > > > > > > > > > > "-2"}, > > > > > > > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS > > > > > > > > > > "-3"}, > > > > > > > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS > > > > > > > > > > "-3"}, > > > > > > > > > > + { } > > > > > > > > > > +}; > > > > > > > > > > > > > > > > > > It seems to me like poking GPIO controller registers > > > > > > > > > directly. This is not good. The question still > > > > > > > > > remains: Can we simply register a GPIO (pin control) > > > > > > > > > driver and use an LED GPIO driver with an additional > > > > > > > > > board file that instantiates it? > > > > > > > > > > > > > > > > I wrote about that in reply to the cover letter. My > > > > > > > > view is still that it would be an abstraction with only > > > > > > > > one user, just causing work and likely not ending up as > > > > > > > > generic as it might eventually have to be. > > > > > > > > > > > > > > > > The region is reserved, not sure what the problem with > > > > > > > > the "poking" is. > > > > > > > > > > > > > > > > > > > > > > Maybe i do not understand all the benefits of such a > > > > > > > > split at this point in time. At the moment i only see > > > > > > > > work with hardly any benefit, not just work for me but > > > > > > > > also for maintainers. I sure do not mean to be > > > > > > > > ignorant. Maybe you go into details and convince me or > > > > > > > > we wait for other peoples opinions on how to proceed, > > > > > > > > maybe there is a second user that i am not aware of? > > > > > > > > Until i am convinced otherwise i will try to argue that > > > > > > > > a single-user-abstraction is needless work/code, and > > > > > > > > should be done only when actually needed. > > > > > > > > > > > > > > I have just read your messages (there is a cover letter > > > > > > > and additional email which was sent lately). > > > > > > > > > > > > > > I would like to know what the CPU model number on that > > > > > > > board is. Than we can continue to see what possibilities > > > > > > > we have here. > > > > > > > > > > > > I guess we are talking about the one that uses memory > > > > > > mapped, that is called an "IPC127E" and seems to have > > > > > > either Intel Atom E3940 or E3930 which seems to be Apollo > > > > > > Lake. > > > > > > > > > > Yep. And now the question, in my patch series you should have > > > > > got the apollolake-pinctrl driver loaded (if not, we have to > > > > > investigate why it's not being instantiated). This will give > > > > > you a read GPIO driver. > > > > > > > > Ok, so there is the existing driver i asked about several times. > > > > Thanks for pointing it out. > > > > > > If you remember, I asked you about the chip twice :-) > > > I assumed that we were talking about Apollo Lake and that's why I > > > insisted that the driver is in the kernel source tree. > > > > Sorry, maybe i did not get the context of your question and which of > > the machines you asked about. Now it is clear i guess. > > > > > > > > > > So, you may use regular LED GPIO on top of it > > > > > (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c). > > > > > I would like to understand why it can't be achieved. > > > > > > > > Will have a look. Unfortunately this one box is missing in my > > > > personal collection, but let us assume that one can be > > > > converted to that existing driver. > > > > > > OK! > > > > > > > I guess that will still mean the PIO-based part of the LED > > > > driver will have to stay as is. > > > > > > Probably yes. I haven't looked into that part and I have no idea > > > what's going on on that platform(s). > > > > > > > Which i guess means the series can be reviewed as if the mmio bits > > for that apollo lake would not be in it, maybe i will even send a > > version without that one box. We have others in the "backlog" might > > as well delay that one if it helps sorting out a base-line. > > It depends on the role of P2SB in this case. > Shouldn't you drop that completely out from this series? We still have one such GPIO bit in one wdt, might be "sunrisepoint". Still have to clarify if that can maybe be dropped for a first step. > Otherwise we have to understand what to do with it. > It seems the best approach can be to expose the P2SB device to Linux, > but we have to answer to Bjorn's request about region reservations. I now have such an apollolake to play with. Having your series applied my LEDs driver fails to alloc that mmio memory, so far so correct. Still have to figure out how to use those GPIOs. I was hoping to find a gpiochip in sysfs and be able to export gpios to userland. Enrico, does "gpio_amd_fch" show up under /sys/class/gpio as a gpiochip? Or how to interact with that driver before basing another one on top? I am afraid that pinctrl-broxton might be loaded but not working as expected. Henning
Am Thu, 1 Apr 2021 14:04:51 +0300 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Thu, Apr 1, 2021 at 1:44 PM Henning Schild > <henning.schild@siemens.com> wrote: > > > > Am Wed, 31 Mar 2021 18:40:23 +0300 > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > > On Tue, Mar 30, 2021 at 6:33 PM Henning Schild > > > <henning.schild@siemens.com> wrote: > > > > Am Tue, 30 Mar 2021 15:41:53 +0300 > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild > > > > > <henning.schild@siemens.com> wrote: > > > > > > Am Tue, 30 Mar 2021 15:15:16 +0300 > > > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild > > > > > > > <henning.schild@siemens.com> wrote: > > > > > > > > Am Tue, 30 Mar 2021 14:04:35 +0300 > > > > > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild > > > > > > > > > <henning.schild@siemens.com> wrote: > > > > > > > > > > > > > > > +static struct simatic_ipc_led > > > > > > > > > > simatic_ipc_leds_mem[] = { > > > > > > > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS > > > > > > > > > > "-1"}, > > > > > > > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS > > > > > > > > > > "-1"}, > > > > > > > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS > > > > > > > > > > "-2"}, > > > > > > > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS > > > > > > > > > > "-2"}, > > > > > > > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS > > > > > > > > > > "-3"}, > > > > > > > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS > > > > > > > > > > "-3"}, > > > > > > > > > > + { } > > > > > > > > > > +}; > > > > > > > > > > > > > > > > > > It seems to me like poking GPIO controller registers > > > > > > > > > directly. This is not good. The question still > > > > > > > > > remains: Can we simply register a GPIO (pin control) > > > > > > > > > driver and use an LED GPIO driver with an additional > > > > > > > > > board file that instantiates it? > > > > > > > > > > > > > > > > I wrote about that in reply to the cover letter. My > > > > > > > > view is still that it would be an abstraction with only > > > > > > > > one user, just causing work and likely not ending up as > > > > > > > > generic as it might eventually have to be. > > > > > > > > > > > > > > > > The region is reserved, not sure what the problem with > > > > > > > > the "poking" is. > > > > > > > > > > > > > > > > > > > > > > Maybe i do not understand all the benefits of such a > > > > > > > > split at this point in time. At the moment i only see > > > > > > > > work with hardly any benefit, not just work for me but > > > > > > > > also for maintainers. I sure do not mean to be > > > > > > > > ignorant. Maybe you go into details and convince me or > > > > > > > > we wait for other peoples opinions on how to proceed, > > > > > > > > maybe there is a second user that i am not aware of? > > > > > > > > Until i am convinced otherwise i will try to argue that > > > > > > > > a single-user-abstraction is needless work/code, and > > > > > > > > should be done only when actually needed. > > > > > > > > > > > > > > I have just read your messages (there is a cover letter > > > > > > > and additional email which was sent lately). > > > > > > > > > > > > > > I would like to know what the CPU model number on that > > > > > > > board is. Than we can continue to see what possibilities > > > > > > > we have here. > > > > > > > > > > > > I guess we are talking about the one that uses memory > > > > > > mapped, that is called an "IPC127E" and seems to have > > > > > > either Intel Atom E3940 or E3930 which seems to be Apollo > > > > > > Lake. > > > > > > > > > > Yep. And now the question, in my patch series you should have > > > > > got the apollolake-pinctrl driver loaded (if not, we have to > > > > > investigate why it's not being instantiated). This will give > > > > > you a read GPIO driver. > > > > > > > > Ok, so there is the existing driver i asked about several times. > > > > Thanks for pointing it out. > > > > > > If you remember, I asked you about the chip twice :-) > > > I assumed that we were talking about Apollo Lake and that's why I > > > insisted that the driver is in the kernel source tree. > > > > Sorry, maybe i did not get the context of your question and which of > > the machines you asked about. Now it is clear i guess. > > > > > > > > > > So, you may use regular LED GPIO on top of it > > > > > (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c). > > > > > I would like to understand why it can't be achieved. > > > > > > > > Will have a look. Unfortunately this one box is missing in my > > > > personal collection, but let us assume that one can be > > > > converted to that existing driver. > > > > > > OK! > > > > > > > I guess that will still mean the PIO-based part of the LED > > > > driver will have to stay as is. > > > > > > Probably yes. I haven't looked into that part and I have no idea > > > what's going on on that platform(s). > > > > > > > Which i guess means the series can be reviewed as if the mmio bits > > for that apollo lake would not be in it, maybe i will even send a > > version without that one box. We have others in the "backlog" might > > as well delay that one if it helps sorting out a base-line. > > It depends on the role of P2SB in this case. > Shouldn't you drop that completely out from this series? Unfortunately the WDT uses one P2SB-GPIO pin as well (for 1 of the two machine types it supports). Dropping would mean loosing 1/5 machines in LED and 2/4 in WDT. So i rather let this series sit until the P2SB stuff is sorted out. But that would just be my personal "preference". We could go "divide and conquer", shrink the number of supported devices and drop all that needs P2SB ... also a valid way, we have the platform-abstraction to build upon ... and we would get p4 out of the way. Henning > Otherwise we have to understand what to do with it. > It seems the best approach can be to expose the P2SB device to Linux, > but we have to answer to Bjorn's request about region reservations. > >
On 12.04.21 13:56, Henning Schild wrote: > Enrico, does "gpio_amd_fch" show up under /sys/class/gpio as a > gpiochip? Or how to interact with that driver before basing another one > on top? It's not probed on its own, but explicitly by a board specific driver, as it needs board specific data. See drivers/platform/x86/pcengines-apuv2.c --mtx
Am Mon, 29 Mar 2021 19:49:26 +0200 schrieb Henning Schild <henning.schild@siemens.com>: > This driver adds initial support for several devices from Siemens. It > is based on a platform driver introduced in an earlier commit. > > Signed-off-by: Henning Schild <henning.schild@siemens.com> > --- > drivers/leds/Kconfig | 3 + > drivers/leds/Makefile | 3 + > drivers/leds/simple/Kconfig | 11 ++ > drivers/leds/simple/Makefile | 2 + > drivers/leds/simple/simatic-ipc-leds.c | 202 > +++++++++++++++++++++++++ 5 files changed, 221 insertions(+) > create mode 100644 drivers/leds/simple/Kconfig > create mode 100644 drivers/leds/simple/Makefile > create mode 100644 drivers/leds/simple/simatic-ipc-leds.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index b6742b4231bf..5c8558a4fa60 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -937,4 +937,7 @@ source "drivers/leds/trigger/Kconfig" > comment "LED Blink" > source "drivers/leds/blink/Kconfig" > > +comment "Simple LED drivers" > +source "drivers/leds/simple/Kconfig" > + > endif # NEW_LEDS > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 2a698df9da57..2de7fdd8d629 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -111,3 +111,6 @@ obj-$(CONFIG_LEDS_TRIGGERS) += > trigger/ > # LED Blink > obj-$(CONFIG_LEDS_BLINK) += blink/ > + > +# Simple LED drivers > +obj-y += simple/ > diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig > new file mode 100644 > index 000000000000..9f6a68336659 > --- /dev/null > +++ b/drivers/leds/simple/Kconfig > @@ -0,0 +1,11 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +config LEDS_SIEMENS_SIMATIC_IPC > + tristate "LED driver for Siemens Simatic IPCs" > + depends on LEDS_CLASS > + depends on SIEMENS_SIMATIC_IPC > + help > + This option enables support for the LEDs of several > Industrial PCs > + from Siemens. > + > + To compile this driver as a module, choose M here: the > module > + will be called simatic-ipc-leds. > diff --git a/drivers/leds/simple/Makefile > b/drivers/leds/simple/Makefile new file mode 100644 > index 000000000000..8481f1e9e360 > --- /dev/null > +++ b/drivers/leds/simple/Makefile > @@ -0,0 +1,2 @@ > +# SPDX-License-Identifier: GPL-2.0 > +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds.o > diff --git a/drivers/leds/simple/simatic-ipc-leds.c > b/drivers/leds/simple/simatic-ipc-leds.c new file mode 100644 > index 000000000000..043edbf81b76 > --- /dev/null > +++ b/drivers/leds/simple/simatic-ipc-leds.c > @@ -0,0 +1,202 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Siemens SIMATIC IPC driver for LEDs > + * > + * Copyright (c) Siemens AG, 2018-2021 > + * > + * Authors: > + * Henning Schild <henning.schild@siemens.com> > + * Jan Kiszka <jan.kiszka@siemens.com> > + * Gerd Haeussler <gerd.haeussler.ext@siemens.com> > + */ > + > +#include <linux/ioport.h> > +#include <linux/kernel.h> > +#include <linux/leds.h> > +#include <linux/module.h> > +#include <linux/pci.h> > +#include <linux/platform_data/x86/simatic-ipc-base.h> > +#include <linux/platform_device.h> > +#include <linux/sizes.h> > +#include <linux/spinlock.h> > + > +#define SIMATIC_IPC_LED_PORT_BASE 0x404E > + > +struct simatic_ipc_led { > + unsigned int value; /* mask for io and offset for mem */ > + char *name; > + struct led_classdev cdev; > +}; > + > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > + {1 << 15, "green:" LED_FUNCTION_STATUS "-1" }, > + {1 << 7, "yellow:" LED_FUNCTION_STATUS "-1" }, > + {1 << 14, "red:" LED_FUNCTION_STATUS "-2" }, > + {1 << 6, "yellow:" LED_FUNCTION_STATUS "-2" }, > + {1 << 13, "red:" LED_FUNCTION_STATUS "-3" }, > + {1 << 5, "yellow:" LED_FUNCTION_STATUS "-3" }, > + { } > +}; > + > +/* the actual start will be discovered with PCI, 0 is a placeholder > */ +struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, > SZ_4K, KBUILD_MODNAME); + > +static void *simatic_ipc_led_memory; > + > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"}, > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"}, > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"}, > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"}, > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"}, > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"}, > + { } > +}; > + > +static struct resource simatic_ipc_led_io_res = > + DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_1, > KBUILD_MODNAME); Should be SZ_2 Henning > +static DEFINE_SPINLOCK(reg_lock); > + > +static inline struct simatic_ipc_led *cdev_to_led(struct > led_classdev *led_cd) +{ > + return container_of(led_cd, struct simatic_ipc_led, cdev); > +} > + > +static void simatic_ipc_led_set_io(struct led_classdev *led_cd, > + enum led_brightness brightness) > +{ > + struct simatic_ipc_led *led = cdev_to_led(led_cd); > + unsigned long flags; > + unsigned int val; > + > + spin_lock_irqsave(®_lock, flags); > + > + val = inw(SIMATIC_IPC_LED_PORT_BASE); > + if (brightness == LED_OFF) > + outw(val | led->value, SIMATIC_IPC_LED_PORT_BASE); > + else > + outw(val & ~led->value, SIMATIC_IPC_LED_PORT_BASE); > + > + spin_unlock_irqrestore(®_lock, flags); > +} > + > +static enum led_brightness simatic_ipc_led_get_io(struct > led_classdev *led_cd) +{ > + struct simatic_ipc_led *led = cdev_to_led(led_cd); > + > + return inw(SIMATIC_IPC_LED_PORT_BASE) & led->value ? LED_OFF > : led_cd->max_brightness; +} > + > +static void simatic_ipc_led_set_mem(struct led_classdev *led_cd, > + enum led_brightness brightness) > +{ > + struct simatic_ipc_led *led = cdev_to_led(led_cd); > + > + u32 *p; > + > + p = simatic_ipc_led_memory + led->value; > + *p = (*p & ~1) | (brightness == LED_OFF); > +} > + > +static enum led_brightness simatic_ipc_led_get_mem(struct > led_classdev *led_cd) +{ > + struct simatic_ipc_led *led = cdev_to_led(led_cd); > + > + u32 *p; > + > + p = simatic_ipc_led_memory + led->value; > + return (*p & 1) ? LED_OFF : led_cd->max_brightness; > +} > + > +static int simatic_ipc_leds_probe(struct platform_device *pdev) > +{ > + const struct simatic_ipc_platform *plat = > pdev->dev.platform_data; > + struct device *dev = &pdev->dev; > + struct simatic_ipc_led *ipcled; > + struct led_classdev *cdev; > + struct resource *res; > + int err, type; > + u32 *p; > + > + switch (plat->devmode) { > + case SIMATIC_IPC_DEVICE_227D: > + case SIMATIC_IPC_DEVICE_427E: > + res = &simatic_ipc_led_io_res; > + ipcled = simatic_ipc_leds_io; > + /* on 227D the two bytes work the other way araound > */ > + if (plat->devmode == SIMATIC_IPC_DEVICE_227D) { > + while (ipcled->value) { > + ipcled->value = > swab16(ipcled->value); > + ipcled++; > + } > + ipcled = simatic_ipc_leds_io; > + } > + type = IORESOURCE_IO; > + if (!devm_request_region(dev, res->start, > resource_size(res), KBUILD_MODNAME)) { > + dev_err(dev, "Unable to register IO resource > at %pR\n", res); > + return -EBUSY; > + } > + break; > + case SIMATIC_IPC_DEVICE_127E: > + res = &simatic_ipc_led_mem_res; > + ipcled = simatic_ipc_leds_mem; > + type = IORESOURCE_MEM; > + > + /* get GPIO base from PCI */ > + res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, > 0)); > + if (res->start == 0) > + return -ENODEV; > + > + /* do the final address calculation */ > + res->start = res->start + (0xC5 << 16); > + res->end += res->start; > + > + simatic_ipc_led_memory = devm_ioremap_resource(dev, > res); > + if (IS_ERR(simatic_ipc_led_memory)) > + return PTR_ERR(simatic_ipc_led_memory); > + > + /* initialize power/watchdog LED */ > + p = simatic_ipc_led_memory + 0x500 + 0x1D8; /* > PM_WDT_OUT */ > + *p = (*p & ~1); > + p = simatic_ipc_led_memory + 0x500 + 0x1C0; /* > PM_BIOS_BOOT_N */ > + *p = (*p | 1); > + > + break; > + default: > + return -ENODEV; > + } > + > + while (ipcled->value) { > + cdev = &ipcled->cdev; > + if (type == IORESOURCE_MEM) { > + cdev->brightness_set = > simatic_ipc_led_set_mem; > + cdev->brightness_get = > simatic_ipc_led_get_mem; > + } else { > + cdev->brightness_set = > simatic_ipc_led_set_io; > + cdev->brightness_get = > simatic_ipc_led_get_io; > + } > + cdev->max_brightness = LED_ON; > + cdev->name = ipcled->name; > + > + err = devm_led_classdev_register(dev, cdev); > + if (err < 0) > + return err; > + ipcled++; > + } > + > + return 0; > +} > + > +static struct platform_driver simatic_ipc_led_driver = { > + .probe = simatic_ipc_leds_probe, > + .driver = { > + .name = KBUILD_MODNAME, > + } > +}; > + > +module_platform_driver(simatic_ipc_led_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:" KBUILD_MODNAME); > +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");
Am Tue, 30 Mar 2021 14:04:35 +0300 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild > <henning.schild@siemens.com> wrote: > > > > This driver adds initial support for several devices from Siemens. > > It is based on a platform driver introduced in an earlier commit. > > ... > > > +#define SIMATIC_IPC_LED_PORT_BASE 0x404E > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = { > > + {1 << 15, "green:" LED_FUNCTION_STATUS "-1" }, > > + {1 << 7, "yellow:" LED_FUNCTION_STATUS "-1" }, > > + {1 << 14, "red:" LED_FUNCTION_STATUS "-2" }, > > + {1 << 6, "yellow:" LED_FUNCTION_STATUS "-2" }, > > + {1 << 13, "red:" LED_FUNCTION_STATUS "-3" }, > > + {1 << 5, "yellow:" LED_FUNCTION_STATUS "-3" }, > > + { } > > +}; > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"}, > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"}, > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"}, > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"}, > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"}, > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"}, > > + { } > > +}; > > It seems to me like poking GPIO controller registers directly. This > is not good. The question still remains: Can we simply register a > GPIO (pin control) driver and use an LED GPIO driver with an > additional board file that instantiates it? The short answer for v4 will be "No we can not!". The pinctrl drivers do not currently probe on any of the devices and attempts to fix that have failed or gut stuck. I tried to help out where i could and waited for a long time. Now my take is to turn the order around. We go in like that and will happily switch to pinctrl if that ever comes up on the machines. Meaning P2SB series on top of this, no more delays please. We do use request_region so have a mutex in place. Meaning we really only touch GPIO while pinctrl does not! I see no issue here, waited for a long time and now expect to be allowed to get merged first. Henning
On Fri, Nov 26, 2021 at 3:28 PM Henning Schild <henning.schild@siemens.com> wrote: > Am Tue, 30 Mar 2021 14:04:35 +0300 > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild > > <henning.schild@siemens.com> wrote: ... > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"}, > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"}, > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"}, > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"}, > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"}, > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"}, > > > + { } > > > +}; > > > > It seems to me like poking GPIO controller registers directly. This > > is not good. The question still remains: Can we simply register a > > GPIO (pin control) driver and use an LED GPIO driver with an > > additional board file that instantiates it? > > The short answer for v4 will be "No we can not!". The pinctrl drivers > do not currently probe on any of the devices and attempts to fix that > have failed or gut stuck. I tried to help out where i could and waited > for a long time. I see, unfortunately I have stuck with some other (more important tasks) and can't fulfil this, but I still consider it's no go for driver poking pin control registers directly. Lemme see if I can prioritize this for next week. > Now my take is to turn the order around. We go in like that and will > happily switch to pinctrl if that ever comes up on the machines. > Meaning P2SB series on top of this, no more delays please. I don't want to slip bad code into the kernel where we can avoid that. > We do use request_region so have a mutex in place. Meaning we really > only touch GPIO while pinctrl does not! I haven't got this. On Intel SoCs GPIO is a part of pin control registers. You can't touch GPIO without touching pin control. > I see no issue here, waited for a long time and now expect to be > allowed to get merged first. Okay, I have these questions / asks so far: 1) Can firmware be fixed in order to provide an ACPI table for the pin control devices? 2) Can you share firmware (BIOS ROM file I suppose) that I may flash on an Apollo Lake machine and see if I can reproduce the issue? 3) As may be a last resort, can you share (remotely) or even send to us the device in question to try?
Am Fri, 26 Nov 2021 16:02:48 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Fri, Nov 26, 2021 at 3:28 PM Henning Schild > <henning.schild@siemens.com> wrote: > > Am Tue, 30 Mar 2021 14:04:35 +0300 > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild > > > <henning.schild@siemens.com> wrote: > > ... > > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"}, > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"}, > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"}, > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"}, > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"}, > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"}, > > > > + { } > > > > +}; > > > > > > It seems to me like poking GPIO controller registers directly. > > > This is not good. The question still remains: Can we simply > > > register a GPIO (pin control) driver and use an LED GPIO driver > > > with an additional board file that instantiates it? > > > > The short answer for v4 will be "No we can not!". The pinctrl > > drivers do not currently probe on any of the devices and attempts > > to fix that have failed or gut stuck. I tried to help out where i > > could and waited for a long time. > > I see, unfortunately I have stuck with some other (more important > tasks) and can't fulfil this, but I still consider it's no go for > driver poking pin control registers directly. Lemme see if I can > prioritize this for next week. I just sent v4. And am sick of waiting on you. Sorry to be that clear here. I want that order changed! If you still end up being fast, perfect. But i want to be faster! > > Now my take is to turn the order around. We go in like that and will > > happily switch to pinctrl if that ever comes up on the machines. > > Meaning P2SB series on top of this, no more delays please. > > I don't want to slip bad code into the kernel where we can avoid that. It is not bad code! That is unfair to say. It can be improved on and that is what we have a FIXME line for. The worst code is code that is not there ... devices without drivers! That is bad, not i minor poke of parts of a resource no other driver claimed! > > We do use request_region so have a mutex in place. Meaning we really > > only touch GPIO while pinctrl does not! > > I haven't got this. On Intel SoCs GPIO is a part of pin control > registers. You can't touch GPIO without touching pin control. i meant pin control, if it ever did probe it would reserve the region and push our hack out, or the other way around ... no conflict! To get both we just need a simple patch and switch to pinctrl, just notify me once your stuff is ready and i will write that patch. > > I see no issue here, waited for a long time and now expect to be > > allowed to get merged first. > > Okay, I have these questions / asks so far: > 1) Can firmware be fixed in order to provide an ACPI table for the pin > control devices? No. The firmware will only receive security but no feature updates ... > 2) Can you share firmware (BIOS ROM file I suppose) that I may flash > on an Apollo Lake machine and see if I can reproduce the issue? I do not have access. But all you need is a firware with no ACPI entry and P2SB hidden. Or simply patch out the two probe paths ;). > 3) As may be a last resort, can you share (remotely) or even send to > us the device in question to try? We are talking about multiple devices. Not just that one apollo lake on which your patches kind of worked. But showed some weirdness which could really become a problem if someone decided to add an ACPI entry .. It pin 42 name could be GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 42 or GPIO_LOOKUP_IDX("INT3452:01", 42 I guess that conflict will have to be dealt with before your can switch to probing pinctrl drivers based on cpu model and not only ACPI/P2SB any longer. regards, Henning
On Fri, Nov 26, 2021 at 4:44 PM Henning Schild <henning.schild@siemens.com> wrote: > Am Fri, 26 Nov 2021 16:02:48 +0200 > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > On Fri, Nov 26, 2021 at 3:28 PM Henning Schild > > <henning.schild@siemens.com> wrote: > > > Am Tue, 30 Mar 2021 14:04:35 +0300 > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild > > > > <henning.schild@siemens.com> wrote: ... > > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"}, > > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"}, > > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"}, > > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"}, > > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"}, > > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"}, > > > > > + { } > > > > > +}; > > > > > > > > It seems to me like poking GPIO controller registers directly. > > > > This is not good. The question still remains: Can we simply > > > > register a GPIO (pin control) driver and use an LED GPIO driver > > > > with an additional board file that instantiates it? > > > > > > The short answer for v4 will be "No we can not!". The pinctrl > > > drivers do not currently probe on any of the devices and attempts > > > to fix that have failed or gut stuck. I tried to help out where i > > > could and waited for a long time. > > > > I see, unfortunately I have stuck with some other (more important > > tasks) and can't fulfil this, but I still consider it's no go for > > driver poking pin control registers directly. Lemme see if I can > > prioritize this for next week. > > I just sent v4. And am sick of waiting on you. Sorry to be that clear > here. I want that order changed! If you still end up being fast, > perfect. But i want to be faster! It's good that you are honest, honesty is what we missed a lot! > > > Now my take is to turn the order around. We go in like that and will > > > happily switch to pinctrl if that ever comes up on the machines. > > > Meaning P2SB series on top of this, no more delays please. > > > > I don't want to slip bad code into the kernel where we can avoid that. > > It is not bad code! That is unfair to say. It can be improved on and > that is what we have a FIXME line for. The worst code is code that is > not there ... devices without drivers! Okay, that's how you interpret the term "bad". Probably I had to use something else to explain that it's racy with the very same case if one adds an ACPI support to it. > That is bad, not i minor poke of parts of a resource no other driver > claimed! > > > > We do use request_region so have a mutex in place. Meaning we really > > > only touch GPIO while pinctrl does not! > > > > I haven't got this. On Intel SoCs GPIO is a part of pin control > > registers. You can't touch GPIO without touching pin control. > > i meant pin control, if it ever did probe it would reserve the region > and push our hack out, or the other way around ... no conflict! > To get both we just need a simple patch and switch to pinctrl, just > notify me once your stuff is ready and i will write that patch. While thinking more on it, the quickest solution here is to do a P2SB game based on DMI strings in the board code for the platform (somewhere under PDx86). > > > I see no issue here, waited for a long time and now expect to be > > > allowed to get merged first. > > > > Okay, I have these questions / asks so far: > > 1) Can firmware be fixed in order to provide an ACPI table for the pin > > control devices? > > No. The firmware will only receive security but no feature updates ... > > > 2) Can you share firmware (BIOS ROM file I suppose) that I may flash > > on an Apollo Lake machine and see if I can reproduce the issue? > > I do not have access. But all you need is a firware with no ACPI entry > and P2SB hidden. Or simply patch out the two probe paths ;). Yes, probably that will work. > > 3) As may be a last resort, can you share (remotely) or even send to > > us the device in question to try? > > We are talking about multiple devices. Not just that one apollo lake on > which your patches kind of worked. > > But showed some weirdness which could really become a problem if > someone decided to add an ACPI entry .. Then it should have different DMI strings or so, it won't be the _same_ platform anymore. > It pin 42 name could be > GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 42 > or > GPIO_LOOKUP_IDX("INT3452:01", 42 > I guess that conflict will have to be dealt with before your can switch > to probing pinctrl drivers based on cpu model and not only ACPI/P2SB any > longer. Not gonna happen.
Am Fri, 26 Nov 2021 16:59:54 +0200 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > On Fri, Nov 26, 2021 at 4:44 PM Henning Schild > <henning.schild@siemens.com> wrote: > > Am Fri, 26 Nov 2021 16:02:48 +0200 > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > On Fri, Nov 26, 2021 at 3:28 PM Henning Schild > > > <henning.schild@siemens.com> wrote: > > > > Am Tue, 30 Mar 2021 14:04:35 +0300 > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>: > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild > > > > > <henning.schild@siemens.com> wrote: > > ... > > > > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { > > > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"}, > > > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"}, > > > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"}, > > > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"}, > > > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"}, > > > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"}, > > > > > > + { } > > > > > > +}; > > > > > > > > > > It seems to me like poking GPIO controller registers directly. > > > > > This is not good. The question still remains: Can we simply > > > > > register a GPIO (pin control) driver and use an LED GPIO > > > > > driver with an additional board file that instantiates it? > > > > > > > > The short answer for v4 will be "No we can not!". The pinctrl > > > > drivers do not currently probe on any of the devices and > > > > attempts to fix that have failed or gut stuck. I tried to help > > > > out where i could and waited for a long time. > > > > > > I see, unfortunately I have stuck with some other (more important > > > tasks) and can't fulfil this, but I still consider it's no go for > > > driver poking pin control registers directly. Lemme see if I can > > > prioritize this for next week. > > > > I just sent v4. And am sick of waiting on you. Sorry to be that > > clear here. I want that order changed! If you still end up being > > fast, perfect. But i want to be faster! > > It's good that you are honest, honesty is what we missed a lot! I was always honest, hope that was not missed from my side ... let alone a lot. > > > > Now my take is to turn the order around. We go in like that and > > > > will happily switch to pinctrl if that ever comes up on the > > > > machines. Meaning P2SB series on top of this, no more delays > > > > please. > > > > > > I don't want to slip bad code into the kernel where we can avoid > > > that. > > > > It is not bad code! That is unfair to say. It can be improved on and > > that is what we have a FIXME line for. The worst code is code that > > is not there ... devices without drivers! > > Okay, that's how you interpret the term "bad". Probably I had to use > something else to explain that it's racy with the very same case if > one adds an ACPI support to it. It is only racy when the firmware would change (which i am unfortunately pretty sure it will not), or if pinctrl would probe without P2SB or ACPI. (where you say "Not gonna happen.") Or i could say "fortunately pretty sure" because that means pinctrl will never probe, hence no race! > > That is bad, not i minor poke of parts of a resource no other driver > > claimed! > > > > > > We do use request_region so have a mutex in place. Meaning we > > > > really only touch GPIO while pinctrl does not! > > > > > > I haven't got this. On Intel SoCs GPIO is a part of pin control > > > registers. You can't touch GPIO without touching pin control. > > > > i meant pin control, if it ever did probe it would reserve the > > region and push our hack out, or the other way around ... no > > conflict! To get both we just need a simple patch and switch to > > pinctrl, just notify me once your stuff is ready and i will write > > that patch. > > While thinking more on it, the quickest solution here is to do a P2SB > game based on DMI strings in the board code for the platform > (somewhere under PDx86). Not sure what you suggest here. p1 does pretty fancy DMI to be really sure to only match specific devices, and only then we do our own P2SB base address discover and region reservation. > > > > I see no issue here, waited for a long time and now expect to be > > > > allowed to get merged first. > > > > > > Okay, I have these questions / asks so far: > > > 1) Can firmware be fixed in order to provide an ACPI table for > > > the pin control devices? > > > > No. The firmware will only receive security but no feature updates > > ... > > > 2) Can you share firmware (BIOS ROM file I suppose) that I may > > > flash on an Apollo Lake machine and see if I can reproduce the > > > issue? > > > > I do not have access. But all you need is a firware with no ACPI > > entry and P2SB hidden. Or simply patch out the two probe paths ;). > > Yes, probably that will work. I wonder how you would probe without the two with your "Not gonna happen.". But maybe your patches will open my eyes and i have been blind all the time. > > > 3) As may be a last resort, can you share (remotely) or even send > > > to us the device in question to try? > > > > We are talking about multiple devices. Not just that one apollo > > lake on which your patches kind of worked. > > > > But showed some weirdness which could really become a problem if > > someone decided to add an ACPI entry .. > > Then it should have different DMI strings or so, it won't be the > _same_ platform anymore. There is different DMI in place. p1 introduces "enum simatic_ipc_station_ids" with currently 7 different devices matched with not a string but a "binary" behind SIMATIC_IPC_DMI_ENTRY_OEM. The struct can be found in simatic_ipc_get_station_id Our strings could be custom, but that binary allows for real DMI identifaction of those currently proposed 7 "platforms". See p4 where i revert string-based DMI matching with SIMATIC_IPC_DMI_ENTRY_OEM-based. Make sure to look at my answer to a question in v4 p4 on that DMI topic. regards, Henning > > It pin 42 name could be > > GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 42 > > or > > GPIO_LOOKUP_IDX("INT3452:01", 42 > > > I guess that conflict will have to be dealt with before your can > > switch to probing pinctrl drivers based on cpu model and not only > > ACPI/P2SB any longer. > > Not gonna happen. >
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index b6742b4231bf..5c8558a4fa60 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -937,4 +937,7 @@ source "drivers/leds/trigger/Kconfig" comment "LED Blink" source "drivers/leds/blink/Kconfig" +comment "Simple LED drivers" +source "drivers/leds/simple/Kconfig" + endif # NEW_LEDS diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index 2a698df9da57..2de7fdd8d629 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -111,3 +111,6 @@ obj-$(CONFIG_LEDS_TRIGGERS) += trigger/ # LED Blink obj-$(CONFIG_LEDS_BLINK) += blink/ + +# Simple LED drivers +obj-y += simple/ diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig new file mode 100644 index 000000000000..9f6a68336659 --- /dev/null +++ b/drivers/leds/simple/Kconfig @@ -0,0 +1,11 @@ +# SPDX-License-Identifier: GPL-2.0-only +config LEDS_SIEMENS_SIMATIC_IPC + tristate "LED driver for Siemens Simatic IPCs" + depends on LEDS_CLASS + depends on SIEMENS_SIMATIC_IPC + help + This option enables support for the LEDs of several Industrial PCs + from Siemens. + + To compile this driver as a module, choose M here: the module + will be called simatic-ipc-leds. diff --git a/drivers/leds/simple/Makefile b/drivers/leds/simple/Makefile new file mode 100644 index 000000000000..8481f1e9e360 --- /dev/null +++ b/drivers/leds/simple/Makefile @@ -0,0 +1,2 @@ +# SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds.o diff --git a/drivers/leds/simple/simatic-ipc-leds.c b/drivers/leds/simple/simatic-ipc-leds.c new file mode 100644 index 000000000000..043edbf81b76 --- /dev/null +++ b/drivers/leds/simple/simatic-ipc-leds.c @@ -0,0 +1,202 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Siemens SIMATIC IPC driver for LEDs + * + * Copyright (c) Siemens AG, 2018-2021 + * + * Authors: + * Henning Schild <henning.schild@siemens.com> + * Jan Kiszka <jan.kiszka@siemens.com> + * Gerd Haeussler <gerd.haeussler.ext@siemens.com> + */ + +#include <linux/ioport.h> +#include <linux/kernel.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/platform_data/x86/simatic-ipc-base.h> +#include <linux/platform_device.h> +#include <linux/sizes.h> +#include <linux/spinlock.h> + +#define SIMATIC_IPC_LED_PORT_BASE 0x404E + +struct simatic_ipc_led { + unsigned int value; /* mask for io and offset for mem */ + char *name; + struct led_classdev cdev; +}; + +static struct simatic_ipc_led simatic_ipc_leds_io[] = { + {1 << 15, "green:" LED_FUNCTION_STATUS "-1" }, + {1 << 7, "yellow:" LED_FUNCTION_STATUS "-1" }, + {1 << 14, "red:" LED_FUNCTION_STATUS "-2" }, + {1 << 6, "yellow:" LED_FUNCTION_STATUS "-2" }, + {1 << 13, "red:" LED_FUNCTION_STATUS "-3" }, + {1 << 5, "yellow:" LED_FUNCTION_STATUS "-3" }, + { } +}; + +/* the actual start will be discovered with PCI, 0 is a placeholder */ +struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME); + +static void *simatic_ipc_led_memory; + +static struct simatic_ipc_led simatic_ipc_leds_mem[] = { + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"}, + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"}, + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"}, + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"}, + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"}, + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"}, + { } +}; + +static struct resource simatic_ipc_led_io_res = + DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_1, KBUILD_MODNAME); + +static DEFINE_SPINLOCK(reg_lock); + +static inline struct simatic_ipc_led *cdev_to_led(struct led_classdev *led_cd) +{ + return container_of(led_cd, struct simatic_ipc_led, cdev); +} + +static void simatic_ipc_led_set_io(struct led_classdev *led_cd, + enum led_brightness brightness) +{ + struct simatic_ipc_led *led = cdev_to_led(led_cd); + unsigned long flags; + unsigned int val; + + spin_lock_irqsave(®_lock, flags); + + val = inw(SIMATIC_IPC_LED_PORT_BASE); + if (brightness == LED_OFF) + outw(val | led->value, SIMATIC_IPC_LED_PORT_BASE); + else + outw(val & ~led->value, SIMATIC_IPC_LED_PORT_BASE); + + spin_unlock_irqrestore(®_lock, flags); +} + +static enum led_brightness simatic_ipc_led_get_io(struct led_classdev *led_cd) +{ + struct simatic_ipc_led *led = cdev_to_led(led_cd); + + return inw(SIMATIC_IPC_LED_PORT_BASE) & led->value ? LED_OFF : led_cd->max_brightness; +} + +static void simatic_ipc_led_set_mem(struct led_classdev *led_cd, + enum led_brightness brightness) +{ + struct simatic_ipc_led *led = cdev_to_led(led_cd); + + u32 *p; + + p = simatic_ipc_led_memory + led->value; + *p = (*p & ~1) | (brightness == LED_OFF); +} + +static enum led_brightness simatic_ipc_led_get_mem(struct led_classdev *led_cd) +{ + struct simatic_ipc_led *led = cdev_to_led(led_cd); + + u32 *p; + + p = simatic_ipc_led_memory + led->value; + return (*p & 1) ? LED_OFF : led_cd->max_brightness; +} + +static int simatic_ipc_leds_probe(struct platform_device *pdev) +{ + const struct simatic_ipc_platform *plat = pdev->dev.platform_data; + struct device *dev = &pdev->dev; + struct simatic_ipc_led *ipcled; + struct led_classdev *cdev; + struct resource *res; + int err, type; + u32 *p; + + switch (plat->devmode) { + case SIMATIC_IPC_DEVICE_227D: + case SIMATIC_IPC_DEVICE_427E: + res = &simatic_ipc_led_io_res; + ipcled = simatic_ipc_leds_io; + /* on 227D the two bytes work the other way araound */ + if (plat->devmode == SIMATIC_IPC_DEVICE_227D) { + while (ipcled->value) { + ipcled->value = swab16(ipcled->value); + ipcled++; + } + ipcled = simatic_ipc_leds_io; + } + type = IORESOURCE_IO; + if (!devm_request_region(dev, res->start, resource_size(res), KBUILD_MODNAME)) { + dev_err(dev, "Unable to register IO resource at %pR\n", res); + return -EBUSY; + } + break; + case SIMATIC_IPC_DEVICE_127E: + res = &simatic_ipc_led_mem_res; + ipcled = simatic_ipc_leds_mem; + type = IORESOURCE_MEM; + + /* get GPIO base from PCI */ + res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0)); + if (res->start == 0) + return -ENODEV; + + /* do the final address calculation */ + res->start = res->start + (0xC5 << 16); + res->end += res->start; + + simatic_ipc_led_memory = devm_ioremap_resource(dev, res); + if (IS_ERR(simatic_ipc_led_memory)) + return PTR_ERR(simatic_ipc_led_memory); + + /* initialize power/watchdog LED */ + p = simatic_ipc_led_memory + 0x500 + 0x1D8; /* PM_WDT_OUT */ + *p = (*p & ~1); + p = simatic_ipc_led_memory + 0x500 + 0x1C0; /* PM_BIOS_BOOT_N */ + *p = (*p | 1); + + break; + default: + return -ENODEV; + } + + while (ipcled->value) { + cdev = &ipcled->cdev; + if (type == IORESOURCE_MEM) { + cdev->brightness_set = simatic_ipc_led_set_mem; + cdev->brightness_get = simatic_ipc_led_get_mem; + } else { + cdev->brightness_set = simatic_ipc_led_set_io; + cdev->brightness_get = simatic_ipc_led_get_io; + } + cdev->max_brightness = LED_ON; + cdev->name = ipcled->name; + + err = devm_led_classdev_register(dev, cdev); + if (err < 0) + return err; + ipcled++; + } + + return 0; +} + +static struct platform_driver simatic_ipc_led_driver = { + .probe = simatic_ipc_leds_probe, + .driver = { + .name = KBUILD_MODNAME, + } +}; + +module_platform_driver(simatic_ipc_led_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:" KBUILD_MODNAME); +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");
This driver adds initial support for several devices from Siemens. It is based on a platform driver introduced in an earlier commit. Signed-off-by: Henning Schild <henning.schild@siemens.com> --- drivers/leds/Kconfig | 3 + drivers/leds/Makefile | 3 + drivers/leds/simple/Kconfig | 11 ++ drivers/leds/simple/Makefile | 2 + drivers/leds/simple/simatic-ipc-leds.c | 202 +++++++++++++++++++++++++ 5 files changed, 221 insertions(+) create mode 100644 drivers/leds/simple/Kconfig create mode 100644 drivers/leds/simple/Makefile create mode 100644 drivers/leds/simple/simatic-ipc-leds.c