| Message ID | 09e3aa95-86ae-ca30-7bb5-a9704d296b43@huawei.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, Apr 26, 2018 at 02:49:49PM +0100, John Garry wrote: > diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c > index 2d4611e..b04425b 100644 > --- a/drivers/bus/hisi_lpc.c > +++ b/drivers/bus/hisi_lpc.c > @@ -18,6 +18,8 @@ > #include <linux/of_platform.h> > #include <linux/pci.h> > #include <linux/slab.h> > +#include <linux/serial_8250.h> > +#include "../tty/serial/8250/8250.h" > > #define DRV_NAME "hisi-lpc" > > @@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata, unsigned > long pio, > #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + sizeof(MFD_CHILD_NAME_PREFIX) - > 1) > > struct hisi_lpc_mfd_cell { > + struct plat_serial8250_port serial8250_port; > struct mfd_cell_acpi_match acpi_match; > char name[MFD_CHILD_NAME_LEN]; > char pnpid[ACPI_ID_LEN]; > @@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device *hostdev) > dev_warn(&child->dev, "set resource fail (%d)\n", ret); > return ret; > } > + if (!strcmp(acpi_device_hid(child), "HISI1031")) { Hmm, there is a way in struct mfd_cell to match child devices using _HID so is there something preventing you from using that? -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26/04/2018 15:08, Mika Westerberg wrote: > On Thu, Apr 26, 2018 at 02:49:49PM +0100, John Garry wrote: >> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c >> index 2d4611e..b04425b 100644 >> --- a/drivers/bus/hisi_lpc.c >> +++ b/drivers/bus/hisi_lpc.c >> @@ -18,6 +18,8 @@ >> #include <linux/of_platform.h> >> #include <linux/pci.h> >> #include <linux/slab.h> >> +#include <linux/serial_8250.h> >> +#include "../tty/serial/8250/8250.h" >> >> #define DRV_NAME "hisi-lpc" >> >> @@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata, unsigned >> long pio, >> #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + sizeof(MFD_CHILD_NAME_PREFIX) - >> 1) >> >> struct hisi_lpc_mfd_cell { >> + struct plat_serial8250_port serial8250_port; >> struct mfd_cell_acpi_match acpi_match; >> char name[MFD_CHILD_NAME_LEN]; >> char pnpid[ACPI_ID_LEN]; >> @@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device *hostdev) >> dev_warn(&child->dev, "set resource fail (%d)\n", ret); >> return ret; >> } >> + if (!strcmp(acpi_device_hid(child), "HISI1031")) { > Hi Mika, > Hmm, there is a way in struct mfd_cell to match child devices using _HID > so is there something preventing you from using that? Not that I know about. Can you describe this method? I guess I also don't need to set the mfd_cell pnpid either for this special case device. Thanks, John > > . > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 26, 2018 at 03:23:17PM +0100, John Garry wrote: > Not that I know about. Can you describe this method? I guess I also don't > need to set the mfd_cell pnpid either for this special case device. There is some documentation in "MFD devices" chapter of Documentation/acpi/enumeration.txt at least. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26/04/2018 15:23, John Garry wrote: + > On 26/04/2018 15:08, Mika Westerberg wrote: >> On Thu, Apr 26, 2018 at 02:49:49PM +0100, John Garry wrote: >>> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c >>> index 2d4611e..b04425b 100644 >>> --- a/drivers/bus/hisi_lpc.c >>> +++ b/drivers/bus/hisi_lpc.c >>> @@ -18,6 +18,8 @@ >>> #include <linux/of_platform.h> >>> #include <linux/pci.h> >>> #include <linux/slab.h> >>> +#include <linux/serial_8250.h> >>> +#include "../tty/serial/8250/8250.h" >>> >>> #define DRV_NAME "hisi-lpc" >>> >>> @@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata, >>> unsigned >>> long pio, >>> #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + >>> sizeof(MFD_CHILD_NAME_PREFIX) - >>> 1) >>> >>> struct hisi_lpc_mfd_cell { >>> + struct plat_serial8250_port serial8250_port; >>> struct mfd_cell_acpi_match acpi_match; >>> char name[MFD_CHILD_NAME_LEN]; >>> char pnpid[ACPI_ID_LEN]; >>> @@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device >>> *hostdev) >>> dev_warn(&child->dev, "set resource fail (%d)\n", ret); >>> return ret; >>> } >>> + if (!strcmp(acpi_device_hid(child), "HISI1031")) { >> > > Hi Mika, > >> Hmm, there is a way in struct mfd_cell to match child devices using _HID >> so is there something preventing you from using that? > > Not that I know about. Can you describe this method? I guess I also > don't need to set the mfd_cell pnpid either for this special case device. > So we using the mfd_cell to match child devices using _HID. At a glance, I don't actually see other drivers to use mfd_cell_acpi_match.pnpid . Anyway we don't use static tables as we need to update the resources of the cell dynamically. However I do look at a driver like intel_quark_i2c_gpio.c, and this dynamically modifies the value of global mfd_cell array here: https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266 I know the cell array is only used at probe time, but this did not look to be good standard practice to me. Thanks, John > Thanks, > John > > >> >> . >> > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 27 Apr 2018, John Garry wrote: > On 26/04/2018 15:23, John Garry wrote: > > On 26/04/2018 15:08, Mika Westerberg wrote: > > > On Thu, Apr 26, 2018 at 02:49:49PM +0100, John Garry wrote: > > > > diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c > > > > index 2d4611e..b04425b 100644 > > > > --- a/drivers/bus/hisi_lpc.c > > > > +++ b/drivers/bus/hisi_lpc.c > > > > @@ -18,6 +18,8 @@ > > > > #include <linux/of_platform.h> > > > > #include <linux/pci.h> > > > > #include <linux/slab.h> > > > > +#include <linux/serial_8250.h> > > > > +#include "../tty/serial/8250/8250.h" > > > > > > > > #define DRV_NAME "hisi-lpc" > > > > > > > > @@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata, > > > > unsigned > > > > long pio, > > > > #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + > > > > sizeof(MFD_CHILD_NAME_PREFIX) - > > > > 1) > > > > > > > > struct hisi_lpc_mfd_cell { > > > > + struct plat_serial8250_port serial8250_port; > > > > struct mfd_cell_acpi_match acpi_match; > > > > char name[MFD_CHILD_NAME_LEN]; > > > > char pnpid[ACPI_ID_LEN]; > > > > @@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device > > > > *hostdev) > > > > dev_warn(&child->dev, "set resource fail (%d)\n", ret); > > > > return ret; > > > > } > > > > + if (!strcmp(acpi_device_hid(child), "HISI1031")) { > > > > > > > Hi Mika, > > > > > Hmm, there is a way in struct mfd_cell to match child devices using _HID > > > so is there something preventing you from using that? > > > > Not that I know about. Can you describe this method? I guess I also > > don't need to set the mfd_cell pnpid either for this special case device. > > > > So we using the mfd_cell to match child devices using _HID. At a glance, I > don't actually see other drivers to use mfd_cell_acpi_match.pnpid . > > Anyway we don't use static tables as we need to update the resources of the > cell dynamically. However I do look at a driver like intel_quark_i2c_gpio.c, > and this dynamically modifies the value of global mfd_cell array here: > https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266 > > I know the cell array is only used at probe time, but this did not look to > be good standard practice to me. Lots of drivers do this to supply dynamic data. If there is no other sane way of providing such data, it's fine to do. Although each situation should be dealt with on a case-by-case basis.
On 30/04/2018 06:36, Lee Jones wrote: > On Fri, 27 Apr 2018, John Garry wrote: >> On 26/04/2018 15:23, John Garry wrote: >>> On 26/04/2018 15:08, Mika Westerberg wrote: >>>> On Thu, Apr 26, 2018 at 02:49:49PM +0100, John Garry wrote: >>>>> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c >>>>> index 2d4611e..b04425b 100644 >>>>> --- a/drivers/bus/hisi_lpc.c >>>>> +++ b/drivers/bus/hisi_lpc.c >>>>> @@ -18,6 +18,8 @@ >>>>> #include <linux/of_platform.h> >>>>> #include <linux/pci.h> >>>>> #include <linux/slab.h> >>>>> +#include <linux/serial_8250.h> >>>>> +#include "../tty/serial/8250/8250.h" >>>>> >>>>> #define DRV_NAME "hisi-lpc" >>>>> >>>>> @@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata, >>>>> unsigned >>>>> long pio, >>>>> #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + >>>>> sizeof(MFD_CHILD_NAME_PREFIX) - >>>>> 1) >>>>> >>>>> struct hisi_lpc_mfd_cell { >>>>> + struct plat_serial8250_port serial8250_port; >>>>> struct mfd_cell_acpi_match acpi_match; >>>>> char name[MFD_CHILD_NAME_LEN]; >>>>> char pnpid[ACPI_ID_LEN]; >>>>> @@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device >>>>> *hostdev) >>>>> dev_warn(&child->dev, "set resource fail (%d)\n", ret); >>>>> return ret; >>>>> } >>>>> + if (!strcmp(acpi_device_hid(child), "HISI1031")) { >>>> >>> >>> Hi Mika, >>> >>>> Hmm, there is a way in struct mfd_cell to match child devices using _HID >>>> so is there something preventing you from using that? >>> >>> Not that I know about. Can you describe this method? I guess I also >>> don't need to set the mfd_cell pnpid either for this special case device. >>> >> >> So we using the mfd_cell to match child devices using _HID. At a glance, I >> don't actually see other drivers to use mfd_cell_acpi_match.pnpid . >> >> Anyway we don't use static tables as we need to update the resources of the >> cell dynamically. However I do look at a driver like intel_quark_i2c_gpio.c, >> and this dynamically modifies the value of global mfd_cell array here: >> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266 >> >> I know the cell array is only used at probe time, but this did not look to >> be good standard practice to me. > > Lots of drivers do this to supply dynamic data. If there is no other > sane way of providing such data, it's fine to do. Although each > situation should be dealt with on a case-by-case basis. > Hi Lee, Thanks for your input. I do see others drivers which use dynamic mem for the mfd_cells (like cros_ec_dev.c), so what we're doing in this driver already is not totally unchartered territory. But creating the MFD cells from the ACPI table could be ... Anyway, I'll cc you in my next patchset and maybe you can kindly check it. Cheers, John -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 30 Apr 2018, John Garry wrote: > On 30/04/2018 06:36, Lee Jones wrote: > > On Fri, 27 Apr 2018, John Garry wrote: > > > On 26/04/2018 15:23, John Garry wrote: > > > > On 26/04/2018 15:08, Mika Westerberg wrote: > > > > > On Thu, Apr 26, 2018 at 02:49:49PM +0100, John Garry wrote: > > > > > > diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c > > > > > > index 2d4611e..b04425b 100644 > > > > > > --- a/drivers/bus/hisi_lpc.c > > > > > > +++ b/drivers/bus/hisi_lpc.c > > > > > > @@ -18,6 +18,8 @@ > > > > > > #include <linux/of_platform.h> > > > > > > #include <linux/pci.h> > > > > > > #include <linux/slab.h> > > > > > > +#include <linux/serial_8250.h> > > > > > > +#include "../tty/serial/8250/8250.h" > > > > > > > > > > > > #define DRV_NAME "hisi-lpc" > > > > > > > > > > > > @@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata, > > > > > > unsigned > > > > > > long pio, > > > > > > #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + > > > > > > sizeof(MFD_CHILD_NAME_PREFIX) - > > > > > > 1) > > > > > > > > > > > > struct hisi_lpc_mfd_cell { > > > > > > + struct plat_serial8250_port serial8250_port; > > > > > > struct mfd_cell_acpi_match acpi_match; > > > > > > char name[MFD_CHILD_NAME_LEN]; > > > > > > char pnpid[ACPI_ID_LEN]; > > > > > > @@ -513,10 +516,31 @@ static int hisi_lpc_acpi_probe(struct device > > > > > > *hostdev) > > > > > > dev_warn(&child->dev, "set resource fail (%d)\n", ret); > > > > > > return ret; > > > > > > } > > > > > > + if (!strcmp(acpi_device_hid(child), "HISI1031")) { > > > > > > > > > > > > > Hi Mika, > > > > > > > > > Hmm, there is a way in struct mfd_cell to match child devices using _HID > > > > > so is there something preventing you from using that? > > > > > > > > Not that I know about. Can you describe this method? I guess I also > > > > don't need to set the mfd_cell pnpid either for this special case device. > > > > > > > > > > So we using the mfd_cell to match child devices using _HID. At a glance, I > > > don't actually see other drivers to use mfd_cell_acpi_match.pnpid . > > > > > > Anyway we don't use static tables as we need to update the resources of the > > > cell dynamically. However I do look at a driver like intel_quark_i2c_gpio.c, > > > and this dynamically modifies the value of global mfd_cell array here: > > > https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266 > > > > > > I know the cell array is only used at probe time, but this did not look to > > > be good standard practice to me. > > > > Lots of drivers do this to supply dynamic data. If there is no other > > sane way of providing such data, it's fine to do. Although each > > situation should be dealt with on a case-by-case basis. > > > > Hi Lee, > > Thanks for your input. > > I do see others drivers which use dynamic mem for the mfd_cells (like > cros_ec_dev.c), so what we're doing in this driver already is not totally > unchartered territory. But creating the MFD cells from the ACPI table could > be ... Right. I don't normally like mixing platform data technologies (MFD, ACPI and DT). I normally NACK patches which take information from Device Tree and populate MFD cells with it. ACPI would be the same I guess. > Anyway, I'll cc you in my next patchset and maybe you can kindly check it. >
>>>> So we using the mfd_cell to match child devices using _HID. At a glance, I >>>> don't actually see other drivers to use mfd_cell_acpi_match.pnpid . >>>> >>>> Anyway we don't use static tables as we need to update the resources of the >>>> cell dynamically. However I do look at a driver like intel_quark_i2c_gpio.c, >>>> and this dynamically modifies the value of global mfd_cell array here: >>>> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266 >>>> >>>> I know the cell array is only used at probe time, but this did not look to >>>> be good standard practice to me. >>> >>> Lots of drivers do this to supply dynamic data. If there is no other >>> sane way of providing such data, it's fine to do. Although each >>> situation should be dealt with on a case-by-case basis. >>> >> >> Hi Lee, >> >> Thanks for your input. >> >> I do see others drivers which use dynamic mem for the mfd_cells (like >> cros_ec_dev.c), so what we're doing in this driver already is not totally >> unchartered territory. But creating the MFD cells from the ACPI table could >> be ... > > Right. I don't normally like mixing platform data technologies (MFD, > ACPI and DT). I normally NACK patches which take information from > Device Tree and populate MFD cells with it. ACPI would be the same I > guess. Oh, well that is what we have in this driver. So what's the preferred approach? Just not use MFD model at all if ACPI/DT needs to be scanned for data to create the cells? Thanks, John > >> Anyway, I'll cc you in my next patchset and maybe you can kindly check it. >> > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 30 Apr 2018, John Garry wrote: > > > > > So we using the mfd_cell to match child devices using _HID. At a glance, I > > > > > don't actually see other drivers to use mfd_cell_acpi_match.pnpid . > > > > > > > > > > Anyway we don't use static tables as we need to update the resources of the > > > > > cell dynamically. However I do look at a driver like intel_quark_i2c_gpio.c, > > > > > and this dynamically modifies the value of global mfd_cell array here: > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266 > > > > > > > > > > I know the cell array is only used at probe time, but this did not look to > > > > > be good standard practice to me. > > > > > > > > Lots of drivers do this to supply dynamic data. If there is no other > > > > sane way of providing such data, it's fine to do. Although each > > > > situation should be dealt with on a case-by-case basis. > > > > > > > > > > Hi Lee, > > > > > > Thanks for your input. > > > > > > I do see others drivers which use dynamic mem for the mfd_cells (like > > > cros_ec_dev.c), so what we're doing in this driver already is not totally > > > unchartered territory. But creating the MFD cells from the ACPI table could > > > be ... > > > > Right. I don't normally like mixing platform data technologies (MFD, > > ACPI and DT). I normally NACK patches which take information from > > Device Tree and populate MFD cells with it. ACPI would be the same I > > guess. > > Oh, well that is what we have in this driver. So what's the preferred > approach? Just not use MFD model at all if ACPI/DT needs to be scanned for > data to create the cells? I've just seen the driver - yuk! Why are you using the MFD API outside of MFD anyway?
On 30/04/2018 11:46, Lee Jones wrote: > On Mon, 30 Apr 2018, John Garry wrote: > >>>>>> So we using the mfd_cell to match child devices using _HID. At a glance, I >>>>>> don't actually see other drivers to use mfd_cell_acpi_match.pnpid . >>>>>> >>>>>> Anyway we don't use static tables as we need to update the resources of the >>>>>> cell dynamically. However I do look at a driver like intel_quark_i2c_gpio.c, >>>>>> and this dynamically modifies the value of global mfd_cell array here: >>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/intel_quark_i2c_gpio.c#L266 >>>>>> >>>>>> I know the cell array is only used at probe time, but this did not look to >>>>>> be good standard practice to me. >>>>> >>>>> Lots of drivers do this to supply dynamic data. If there is no other >>>>> sane way of providing such data, it's fine to do. Although each >>>>> situation should be dealt with on a case-by-case basis. >>>>> >>>> >>>> Hi Lee, >>>> >>>> Thanks for your input. >>>> >>>> I do see others drivers which use dynamic mem for the mfd_cells (like >>>> cros_ec_dev.c), so what we're doing in this driver already is not totally >>>> unchartered territory. But creating the MFD cells from the ACPI table could >>>> be ... >>> >>> Right. I don't normally like mixing platform data technologies (MFD, >>> ACPI and DT). I normally NACK patches which take information from >>> Device Tree and populate MFD cells with it. ACPI would be the same I >>> guess. >> >> Oh, well that is what we have in this driver. So what's the preferred >> approach? Just not use MFD model at all if ACPI/DT needs to be scanned for >> data to create the cells? > > I've just seen the driver - yuk! > > Why are you using the MFD API outside of MFD anyway? Hi Lee, This goes back a bit. The original thread was here: https://lkml.org/lkml/2017/6/13/581 Essentially a method was required to model this host to support platform device children for ACPI FW, and this did the job. In hindsight I think that there was a misunderstanding in recommending MFD since the devices attached are not fixed - hence the dynamic part. Cheers, John > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c index 2d4611e..b04425b 100644 --- a/drivers/bus/hisi_lpc.c +++ b/drivers/bus/hisi_lpc.c @@ -18,6 +18,8 @@ #include <linux/of_platform.h> #include <linux/pci.h> #include <linux/slab.h> +#include <linux/serial_8250.h> +#include "../tty/serial/8250/8250.h" #define DRV_NAME "hisi-lpc" @@ -345,6 +347,7 @@ static void hisi_lpc_comm_outs(void *hostdata, unsigned long pio, #define MFD_CHILD_NAME_LEN (ACPI_ID_LEN + sizeof(MFD_CHILD_NAME_PREFIX) - 1) struct hisi_lpc_mfd_cell { + struct plat_serial8250_port serial8250_port; struct mfd_cell_acpi_match acpi_match;