Message ID | 20210901201934.1084250-1-dianders@chromium.org |
---|---|
Headers | show |
Series | eDP: Support probing eDP panels dynamically instead of hardcoding | expand |
Hi Douglas, Le mer., sept. 1 2021 at 13:19:26 -0700, Douglas Anderson <dianders@chromium.org> a écrit : > In the patch ("drm/panel-simple-edp: Split eDP panels out of > panel-simple") we split the PANEL_SIMPLE driver in 2. By default let's > give everyone who had the old driver enabled the new driver too. If > folks want to opt-out of one or the other they always can later. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > (no changes since v1) > > arch/mips/configs/qi_lb60_defconfig | 1 + > arch/mips/configs/rs90_defconfig | 1 + The SoCs on these two boards don't support eDP, you can drop this patch. Cheers, -Paul > 2 files changed, 2 insertions(+) > > diff --git a/arch/mips/configs/qi_lb60_defconfig > b/arch/mips/configs/qi_lb60_defconfig > index b4448d0876d5..3e99e223ea02 100644 > --- a/arch/mips/configs/qi_lb60_defconfig > +++ b/arch/mips/configs/qi_lb60_defconfig > @@ -72,6 +72,7 @@ CONFIG_REGULATOR_FIXED_VOLTAGE=y > CONFIG_DRM=y > CONFIG_DRM_FBDEV_OVERALLOC=200 > CONFIG_DRM_PANEL_SIMPLE=y > +CONFIG_DRM_PANEL_SIMPLE_EDP=y > CONFIG_DRM_INGENIC=y > CONFIG_BACKLIGHT_CLASS_DEVICE=y > # CONFIG_VGA_CONSOLE is not set > diff --git a/arch/mips/configs/rs90_defconfig > b/arch/mips/configs/rs90_defconfig > index 7ce3b814fdc8..42b4f621cbfa 100644 > --- a/arch/mips/configs/rs90_defconfig > +++ b/arch/mips/configs/rs90_defconfig > @@ -94,6 +94,7 @@ CONFIG_REGULATOR_FIXED_VOLTAGE=y > CONFIG_DRM=y > CONFIG_DRM_FBDEV_OVERALLOC=300 > CONFIG_DRM_PANEL_SIMPLE=y > +CONFIG_DRM_PANEL_SIMPLE_EDP=y > CONFIG_DRM_INGENIC=y > CONFIG_BACKLIGHT_CLASS_DEVICE=y > CONFIG_BACKLIGHT_PWM=y > -- > 2.33.0.259.gc128427fd7-goog >
On Wed, Sep 1, 2021 at 1:20 PM Douglas Anderson <dianders@chromium.org> wrote: > > In the patch ("drm/panel-simple-edp: Split eDP panels out of > panel-simple") we split the PANEL_SIMPLE driver in 2. By default let's > give everyone who had the old driver enabled the new driver too. If > folks want to opt-out of one or the other they always can later. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> Isn't this a case where the new option should just have had the old option as the default value to avoid this kind of churn and possibly broken platforms? -Olof
Hi, On Wed, Sep 1, 2021 at 2:12 PM Olof Johansson <olof@lixom.net> wrote: > > On Wed, Sep 1, 2021 at 1:20 PM Douglas Anderson <dianders@chromium.org> wrote: > > > > In the patch ("drm/panel-simple-edp: Split eDP panels out of > > panel-simple") we split the PANEL_SIMPLE driver in 2. By default let's > > give everyone who had the old driver enabled the new driver too. If > > folks want to opt-out of one or the other they always can later. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > Isn't this a case where the new option should just have had the old > option as the default value to avoid this kind of churn and possibly > broken platforms? I'm happy to go either way. I guess I didn't do that originally because logically there's not any reason to link the two drivers going forward. Said another way, someone enabling the "simple panel" driver for non-eDP panels wouldn't expect that the "simple panel" driver for DP panels would also get enabled by default. They really have nothing to do with one another. Enabling by default for something like this also seems like it would lead to bloat. I could have sworn that periodically people get yelled at for marking drivers on by default when it doesn't make sense. ...that being said, I'm happy to change the default as you suggest. Just let me know. -Doug
Removed most CC: SMTP server protested. On 01.09.2021 22:19, Douglas Anderson wrote: > The goal of this patch series is to move away from hardcoding exact > eDP panels in device tree files. As discussed in the various patches > in this series (I'm not repeating everything here), most eDP panels > are 99% probable and we can get that last 1% by allowing two "power > up" delays to be specified in the device tree file and then using the > panel ID (found in the EDID) to look up additional power sequencing > delays for the panel. > > This patch series is the logical contiunation of a previous patch > series where I proposed solving this problem by adding a > board-specific compatible string [1]. In the discussion that followed > it sounded like people were open to something like the solution > proposed in this new series. > > In version 2 I got rid of the idea that we could have a "fallback" > compatible string that we'd use if we didn't recognize the ID in the > EDID. This simplifies the bindings a lot and the implementation > somewhat. As a result of not having a "fallback", though, I'm not > confident in transitioning any existing boards over to this since > we'll have to fallback to very conservative timings if we don't > recognize the ID from the EDID and I can't guarantee that I've seen > every panel that might have shipped on an existing product. The plan > is to use "edp-panel" only on new boards or new revisions of old > boards where we can guarantee that every EDID that ships out of the > factory has an ID in the table. > > Version 3 of this series now splits out all eDP panels to their own > driver and adds the generic eDP panel support to this new driver. I > believe this is what Sam was looking for [2]. > > [1] https://lore.kernel.org/r/YFKQaXOmOwYyeqvM@google.com/ > [2] https://lore.kernel.org/r/YRTsFNTn%2FT8fLxyB@ravnborg.org/ > I like the idea - if something can be configured dynamically lets do it. But I have few questions: 1. Have you read different real panels id's? In many cases (MIPI DSI, LVDS with EDID) manufacturers often forgot to set proper id fields. I do not know how EDID's ids are reliable in case of edp panels. 2. You are working with edp panels - beside EDID they have DPCD which contains things like IEEE_OUI and "Device Identification String", I guess they could be also used for detecting panels, have you considered it? I think DPCD Id should be assigned to EDP-Sink interface, and EDID Id to the actual panel behind it. With this assumption one could consider which timings should be property of which entity. Regards Andrzej
Hi, On Thu, Sep 2, 2021 at 3:10 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > > Removed most CC: SMTP server protested. Yeah, it was because of the dang defconfig patches. My general policy is to send the cover letter to everyone even if not everyone gets CCed on all patches, but it ended up as a lot... Speaking of which, I'd definitely be interested in your take on the defconfig topic: https://lore.kernel.org/r/CAD=FV=WPXAUyuAHb1jKx9F_aw+JGX4MWB3or=Eq5rXoKY=OQMw@mail.gmail.com Do you agree with Olof that I should set the "default" for the new config to be the same as the old config? It doesn't make sense going forward but it helps for anyone with old configs... > On 01.09.2021 22:19, Douglas Anderson wrote: > > The goal of this patch series is to move away from hardcoding exact > > eDP panels in device tree files. As discussed in the various patches > > in this series (I'm not repeating everything here), most eDP panels > > are 99% probable and we can get that last 1% by allowing two "power > > up" delays to be specified in the device tree file and then using the > > panel ID (found in the EDID) to look up additional power sequencing > > delays for the panel. > > > > This patch series is the logical contiunation of a previous patch > > series where I proposed solving this problem by adding a > > board-specific compatible string [1]. In the discussion that followed > > it sounded like people were open to something like the solution > > proposed in this new series. > > > > In version 2 I got rid of the idea that we could have a "fallback" > > compatible string that we'd use if we didn't recognize the ID in the > > EDID. This simplifies the bindings a lot and the implementation > > somewhat. As a result of not having a "fallback", though, I'm not > > confident in transitioning any existing boards over to this since > > we'll have to fallback to very conservative timings if we don't > > recognize the ID from the EDID and I can't guarantee that I've seen > > every panel that might have shipped on an existing product. The plan > > is to use "edp-panel" only on new boards or new revisions of old > > boards where we can guarantee that every EDID that ships out of the > > factory has an ID in the table. > > > > Version 3 of this series now splits out all eDP panels to their own > > driver and adds the generic eDP panel support to this new driver. I > > believe this is what Sam was looking for [2]. > > > > [1] https://lore.kernel.org/r/YFKQaXOmOwYyeqvM@google.com/ > > [2] https://lore.kernel.org/r/YRTsFNTn%2FT8fLxyB@ravnborg.org/ > > > I like the idea - if something can be configured dynamically lets do it. > But I have few questions: > 1. Have you read different real panels id's? In many cases (MIPI DSI, > LVDS with EDID) manufacturers often forgot to set proper id fields. I do > not know how EDID's ids are reliable in case of edp panels. I have read several and (so far) they have been quite reliable but I will admit that I haven't done an exhaustive sample. I guess my answer to whether we can trust it is: a) Presumably you'd only use this new "edp-panel" compatible on systems whose panel IDs are known to be reliable. Old systems can keep determining the panel by compatible string. The two schemes can co-exist. b) As we start using this new scheme then there will be more validation of panel ID strings and, presumably, they will become more reliable. It is still true that ID conflicts could exist. A vendor could ship two different panels with the same ID and maybe nobody would notice because they weren't ever hooked up to the same board. In that case we'd have a question of what to do upstream. If this really happens then I suppose (worst case) we could use the device tree to help differentiate and each board could say that "panel ID <x> hooked up to this board is actually panel <y>". I hope we don't have to do that because it feels dirty, but at least it gives us _something_ if we get backed into a corner. > 2. You are working with edp panels - beside EDID they have DPCD which > contains things like IEEE_OUI and "Device Identification String", I > guess they could be also used for detecting panels, have you considered > it? I think DPCD Id should be assigned to EDP-Sink interface, and EDID > Id to the actual panel behind it. With this assumption one could > consider which timings should be property of which entity. This could be another way to help us if we're backed into a corner in the future. Right now the driver requires that you give access to a full eDP AUX bus to use the "generic eDP" panel support even though it currently only relies on the EDID, so it would be pretty easy to utilize this info in the future. So far the ID has been reliable for me though. -Doug
On 02.09.2021 01:10, Doug Anderson wrote: > Hi, > > On Wed, Sep 1, 2021 at 2:12 PM Olof Johansson <olof@lixom.net> wrote: >> >> On Wed, Sep 1, 2021 at 1:20 PM Douglas Anderson <dianders@chromium.org> wrote: >>> >>> In the patch ("drm/panel-simple-edp: Split eDP panels out of >>> panel-simple") we split the PANEL_SIMPLE driver in 2. By default let's >>> give everyone who had the old driver enabled the new driver too. If >>> folks want to opt-out of one or the other they always can later. >>> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >> >> Isn't this a case where the new option should just have had the old >> option as the default value to avoid this kind of churn and possibly >> broken platforms? > > I'm happy to go either way. I guess I didn't do that originally > because logically there's not any reason to link the two drivers going > forward. Said another way, someone enabling the "simple panel" driver > for non-eDP panels wouldn't expect that the "simple panel" driver for > DP panels would also get enabled by default. They really have nothing > to do with one another. Enabling by default for something like this > also seems like it would lead to bloat. I could have sworn that > periodically people get yelled at for marking drivers on by default > when it doesn't make sense. > > ...that being said, I'm happy to change the default as you suggest. > Just let me know. I guess this is just misunderstanding. Symbol names: CONFIG_DRM_PANEL_SIMPLE=y CONFIG_DRM_PANEL_SIMPLE_EDP=y suggests that CONFIG_DRM_PANEL_SIMPLE_EDP is an 'suboption' of CONFIG_DRM_PANEL_SIMPLE, but these symbols are independent - old symbol has been split into two independent new symbols. So Doug's approach seems correct to me. Maybe one could change names of symbols to avoid confusion(?). One more thing, I suspect previous patch can break platforms with EDP panels. Even if this patch fixes it, maybe it would be better to squash these patches? Or add temporal solution to save bisecatability. Regards Andrzej > > -Doug >
On Wed, 01 Sep 2021, Douglas Anderson <dianders@chromium.org> wrote: > A future change wants to be able to read just block 0 of the EDID, so > break it out of drm_do_get_edid() into a sub-function. > > This is intended to be a no-op change--just code movement. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > (no changes since v1) > > drivers/gpu/drm/drm_edid.c | 62 +++++++++++++++++++++++++++----------- > 1 file changed, 44 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 6325877c5fd6..a22c38482a90 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -1905,6 +1905,43 @@ int drm_add_override_edid_modes(struct drm_connector *connector) > } > EXPORT_SYMBOL(drm_add_override_edid_modes); > > +static struct edid *drm_do_get_edid_blk0( Maybe base_block instead of blk0? > + int (*get_edid_block)(void *data, u8 *buf, unsigned int block, > + size_t len), > + void *data, bool *edid_corrupt, int *null_edid_counter) > +{ > + int i; > + u8 *edid; With void *edid, this function wouldn't need the cast internally. > + > + if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) > + return NULL; Could split the allocation and NULL check to two separate lines per coding style, while at it? BR, Jani. > + > + /* base block fetch */ > + for (i = 0; i < 4; i++) { > + if (get_edid_block(data, edid, 0, EDID_LENGTH)) > + goto out; > + if (drm_edid_block_valid(edid, 0, false, edid_corrupt)) > + break; > + if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) { > + if (null_edid_counter) > + (*null_edid_counter)++; > + goto carp; > + } > + } > + if (i == 4) > + goto carp; > + > + return (struct edid *)edid; > + > +carp: > + kfree(edid); > + return ERR_PTR(-EINVAL); > + > +out: > + kfree(edid); > + return NULL; > +} > + > /** > * drm_do_get_edid - get EDID data using a custom EDID block read function > * @connector: connector we're probing > @@ -1938,25 +1975,16 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, > if (override) > return override; > > - if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) > + edid = (u8 *)drm_do_get_edid_blk0(get_edid_block, data, > + &connector->edid_corrupt, > + &connector->null_edid_counter); > + if (IS_ERR_OR_NULL(edid)) { > + if (IS_ERR(edid)) > + connector_bad_edid(connector, edid, 1); > return NULL; > - > - /* base block fetch */ > - for (i = 0; i < 4; i++) { > - if (get_edid_block(data, edid, 0, EDID_LENGTH)) > - goto out; > - if (drm_edid_block_valid(edid, 0, false, > - &connector->edid_corrupt)) > - break; > - if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) { > - connector->null_edid_counter++; > - goto carp; > - } > } > - if (i == 4) > - goto carp; > > - /* if there's no extensions, we're done */ > + /* if there's no extensions or no connector, we're done */ > valid_extensions = edid[0x7e]; > if (valid_extensions == 0) > return (struct edid *)edid; > @@ -2010,8 +2038,6 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, > > return (struct edid *)edid; > > -carp: > - connector_bad_edid(connector, edid, 1); > out: > kfree(edid); > return NULL;
On Wed, 01 Sep 2021, Douglas Anderson <dianders@chromium.org> wrote: > EDIDs have 32-bits worth of data which is intended to be used to > uniquely identify the make/model of a panel. This has historically > been used only internally in the EDID processing code to identify > quirks with panels. > > We'd like to use this panel ID in panel-simple to identify which panel > is hooked up and from that information figure out power sequence > timings. Let's expose this information from the EDID code and also > allow it to be accessed early, before a connector has been created. > > To make matching in the panel-simple code easier, we'll return the > panel ID as a 32-bit value. We'll provide some functions for > converting this value back and forth to something more human readable. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > Changes in v3: > - Decode hex product ID w/ same endianness as everyone else. > > drivers/gpu/drm/drm_edid.c | 59 ++++++++++++++++++++++++++++++++++++++ > include/drm/drm_edid.h | 47 ++++++++++++++++++++++++++++++ > 2 files changed, 106 insertions(+) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index a22c38482a90..ac128bc3478a 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -2086,6 +2086,65 @@ struct edid *drm_get_edid(struct drm_connector *connector, > } > EXPORT_SYMBOL(drm_get_edid); > > +/** > + * drm_get_panel_id - Get a panel's ID through DDC > + * @adapter: I2C adapter to use for DDC > + * > + * This function reads the first block of the EDID of a panel and (assuming > + * that the EDID is valid) extracts the ID out of it. The ID is a 32-bit value > + * (16 bits of manufacturer ID and 16 bits of per-manufacturer ID) that's > + * supposed to be different for each different modem of panel. > + * > + * This function is intended to be used during early probing on devices where > + * more than one panel might be present. Because of its intended use it must > + * assume that the EDID of the panel is correct, at least as far as the ID > + * is concerned (in other words, we don't process any overrides here). > + * > + * NOTE: it's expected that this function and drm_do_get_edid() will both > + * be read the EDID, but there is no caching between them. Since we're only > + * reading the first block, hopefully this extra overhead won't be too big. > + * > + * Return: A 32-bit ID that should be different for each make/model of panel. > + * See the functions encode_edid_id() and decode_edid_id() for some > + * details on the structure of this ID. > + */ > +u32 drm_get_panel_id(struct i2c_adapter *adapter) Please call it drm_edid_get_panel_id() because that's what it is, and this is in drm_edid.[ch]. > +{ > + struct edid *edid; > + u32 val; > + > + edid = drm_do_get_edid_blk0(drm_do_probe_ddc_edid, adapter, NULL, NULL); > + > + /* > + * There are no manufacturer IDs of 0, so if there is a problem reading > + * the EDID then we'll just return 0. > + */ > + if (IS_ERR_OR_NULL(edid)) > + return 0; > + > + /* > + * In theory we could try to de-obfuscate this like edid_get_quirks() > + * does, but it's easier to just deal with a 32-bit number. Hmm, but is it, really? AFAICT this is just an internal representation for a table, where it could just as well be stored in a struct that could be just as compact now, but extensible later. You populate the table via an encoding macro, then decode the id using a function - while it could be in a format that's directly usable without the decode. If suitably chosen, the struct could perhaps be reused between the quirks code and your code. > + * > + * NOTE that we deal with endianness differently for the top half > + * of this ID than for the bottom half. The bottom half (the product > + * id) gets decoded as little endian by the EDID_PRODUCT_ID because > + * that's how everyone seems to interpret it. The top half (the mfg_id) > + * gets stored as big endian because that makes encode_edid_id() and > + * decode_edid_id() easier to write (it's easier to extract the ASCII). > + * It doesn't really matter, though, as long as the number here is > + * unique. > + */ > + val = (u32)edid->mfg_id[0] << 24 | > + (u32)edid->mfg_id[1] << 16 | > + (u32)EDID_PRODUCT_ID(edid); > + > + kfree(edid); > + > + return val; > +} > +EXPORT_SYMBOL(drm_get_panel_id); > + > /** > * drm_get_edid_switcheroo - get EDID data for a vga_switcheroo output > * @connector: connector we're probing > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > index deccfd39e6db..73da40d0b5d1 100644 > --- a/include/drm/drm_edid.h > +++ b/include/drm/drm_edid.h > @@ -508,6 +508,52 @@ static inline u8 drm_eld_get_conn_type(const uint8_t *eld) > return eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK; > } > > +/** > + * encode_edid_id - Encode an ID for matching against drm_get_panel_id() > + * @vend_chr_0: First character of the vendor string. > + * @vend_chr_2: Second character of the vendor string. > + * @vend_chr_3: Third character of the vendor string. > + * @product_id: The 16-bit product ID. > + * > + * This is a macro so that it can be calculated at compile time and used > + * as an initializer. > + * > + * For instance: > + * encode_edid_id('B', 'O', 'E', 0x2d08) => 0x09e52d08 > + * > + * Return: a 32-bit ID per panel. > + */ > +#define encode_edid_id(vend_chr_0, vend_chr_1, vend_chr_2, product_id) \ > + ((((u32)(vend_chr_0) - '@') & 0x1f) << 26 | \ > + (((u32)(vend_chr_1) - '@') & 0x1f) << 21 | \ > + (((u32)(vend_chr_2) - '@') & 0x1f) << 16 | \ > + ((product_id) & 0xffff)) > + > +/** > + * decode_edid_id - Decode a panel ID from encode_edid_id() > + * @panel_id: The panel ID to decode. > + * @vend: A 4-byte buffer to store the 3-letter vendor string plus a '\0' > + * termination > + * @product_id: The product ID will be returned here. > + * > + * For instance, after: > + * decode_edid_id(0x09e52d08, vend, &product_id) > + * These will be true: > + * vend[0] = 'B' > + * vend[1] = 'O' > + * vend[2] = 'E' > + * vend[3] = '\0' > + * product_id = 0x2d08 > + */ > +static inline void decode_edid_id(u32 panel_id, char vend[4], u16 *product_id) > +{ > + *product_id = (u16)(panel_id & 0xffff); > + vend[0] = '@' + ((panel_id >> 26) & 0x1f); > + vend[1] = '@' + ((panel_id >> 21) & 0x1f); > + vend[2] = '@' + ((panel_id >> 16) & 0x1f); > + vend[3] = '\0'; > +} I think the names here could use a drm_edid_ prefix too. Maybe drm_edid_encode_panel_id and drm_edid_decode_panel_id, aligning nicely with drm_edid_get_panel_id. BR, Jani. > + > bool drm_probe_ddc(struct i2c_adapter *adapter); > struct edid *drm_do_get_edid(struct drm_connector *connector, > int (*get_edid_block)(void *data, u8 *buf, unsigned int block, > @@ -515,6 +561,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, > void *data); > struct edid *drm_get_edid(struct drm_connector *connector, > struct i2c_adapter *adapter); > +u32 drm_get_panel_id(struct i2c_adapter *adapter); > struct edid *drm_get_edid_switcheroo(struct drm_connector *connector, > struct i2c_adapter *adapter); > struct edid *drm_edid_duplicate(const struct edid *edid);
Hi, On Sun, Sep 5, 2021 at 11:46 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > On Wed, Sep 01, 2021 at 01:19:28PM -0700, Douglas Anderson wrote: > > All of the "HPD" handling added to panel-simple recently was for eDP > > panels. Remove it from panel-simple now that panel-simple-edp handles > > eDP panels. The "prepare_to_enable" delay only makes sense in the > > context of HPD, so remove it too. No non-eDP panels used it anyway. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > Maybe merge this with the patch that moved all the functionality > from panel-simple to panel-edp? Unless you feel strongly about it, I'm going to keep it separate still in the next version. To try to make diffing easier, I tried hard to make the minimal changes in the "split the driver in two" patch. -Doug
Hi, On Fri, Sep 3, 2021 at 1:38 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Doug Anderson (2021-09-01 16:10:15) > > Hi, > > > > On Wed, Sep 1, 2021 at 2:12 PM Olof Johansson <olof@lixom.net> wrote: > > > > > > On Wed, Sep 1, 2021 at 1:20 PM Douglas Anderson <dianders@chromium.org> wrote: > > > > > > > > In the patch ("drm/panel-simple-edp: Split eDP panels out of > > > > panel-simple") we split the PANEL_SIMPLE driver in 2. By default let's > > > > give everyone who had the old driver enabled the new driver too. If > > > > folks want to opt-out of one or the other they always can later. > > > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > > > Isn't this a case where the new option should just have had the old > > > option as the default value to avoid this kind of churn and possibly > > > broken platforms? > > > > I'm happy to go either way. I guess I didn't do that originally > > because logically there's not any reason to link the two drivers going > > forward. Said another way, someone enabling the "simple panel" driver > > for non-eDP panels wouldn't expect that the "simple panel" driver for > > DP panels would also get enabled by default. They really have nothing > > to do with one another. Enabling by default for something like this > > also seems like it would lead to bloat. I could have sworn that > > periodically people get yelled at for marking drivers on by default > > when it doesn't make sense. > > > > ...that being said, I'm happy to change the default as you suggest. > > Just let me know. > > Having the default will help olddefconfig users seamlessly migrate to > the new Kconfig. Sadly they don't notice that they should probably > disable the previous Kconfig symbol, but oh well. At least with the > default they don't go on a hunt/bisect to figure out that some Kconfig > needed to be enabled now that they're using a new kernel version. > > Maybe the default should have a TODO comment next to it indicating we > should remove the default in a year or two. OK, so I'm trying to figure out how to do this without just "kicking the can" down the road. I guess your idea is that for the next year this will be the default and that anyone who really wants "CONFIG_DRM_PANEL_EDP" will "opt-in" to keep it by adding "CONFIG_DRM_PANEL_EDP=y" to their config? ...and then after a year passes we remove the default? ...but that won't work, will it? Since "CONFIG_DRM_PANEL_EDP" will be the default for the next year then you really can't add it to the "defconfig", at least if you ever "normalize" it. The "defconfig" by definition has everything stripped from it that's already the "default", so for the next year anyone who tries to opt-in will get their preference stripped. Hrm, so let me explain options as I see them. Maybe someone can point out something that I missed. I'll assume that we'll change the config option from CONFIG_DRM_PANEL_SIMPLE_EDP to CONFIG_DRM_PANEL_EDP (remove the "SIMPLE" part). == Where we were before my series: * One config "CONFIG_DRM_PANEL_SIMPLE" and it enables simple non-eDP and eDP drivers. == Option 1: update everyone's configs (this patch) * Keep old config "CONFIG_DRM_PANEL_SIMPLE" but it now only means enable the panel-simple (non-eDP) driver. * Anyone who wants eDP panels must opt-in to "CONFIG_DRM_PANEL_EDP" * Update all configs in mainline; any out-of mainline configs must figure this out themselves. Pros: * no long term baggage Cons: * patch upstream is a bit of "churn" * anyone with downstream config will have to figure out what happened. == Option 2: kick the can down the road + accept cruft * Keep old config "CONFIG_DRM_PANEL_SIMPLE" and it means enable the panel-simple (non-eDP) driver. * Anyone with "CONFIG_DRM_PANEL_SIMPLE" is opted in by default to "CONFIG_DRM_PANEL_EDP" AKA: config DRM_PANEL_EDP default DRM_PANEL_SIMPLE Pros: * no config patches needed upstream at all and everything just works! Cons: * people are opted in to extra cruft by default and need to know to turn it off. * unclear if we can change the default without the same problems. == Option 3: try to be clever * Add _two_ new configs. CONFIG_DRM_PANEL_SIMPLE_V2 and CONFIG_DRM_PANEL_EDP. * Old config "CONFIG_DRM_PANEL_SIMPLE" gets marked as "deprecated". * Both new configs have "default CONFIG_DRM_PANEL_SIMPLE" Now anyone old will magically get both the new config options by default. Anyone looking at this in the future _won't_ set the deprecated CONFIG_DRM_PANEL_SIMPLE but will instead choose if they want either the eDP or "simple" driver. Pros: * No long term baggage. * Everyone is transitioned automatically by default with no cruft patches. Cons: * I can't think of a better name than "CONFIG_DRM_PANEL_SIMPLE_V2" and that name is ugly. == Option 4: shave a yak When thinking about this I came up with a clever idea of stashing the kernel version in a defconfig when it's generated. Then you could do something like: config DRM_PANEL_EDP default DRM_PANEL_SIMPLE if DEFCONFIG_GENERATED_AT <= 0x00050f00 That feels like a good idea to me but who knows what others would think. In general I think this series already shaves enough yaks. This isn't a new problem we're trying to solve so it seems like we should pick one of the options above. == Unless I get an explicit NAK from someone like Olof or Arnd or I hear that everyone loves Option #3 I'll probably just stick with the existing approach since: * Olof's wording didn't make it sound like a strong objection. * From git history it looks as if config patches don't necessarily land through the SoC tree and thus I'd by default follow the suggestions of the DRM folks. Andrzej suggested going with the existing approach as long as I changed the symbol names and re-ordered the patches. Please yell if anything above sounds wrong! I'll probably try to send out a new version tomorrow or the next day, but I won't land it right away to give people time to yell. -Doug
On Wed, Sep 8, 2021 at 3:36 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Fri, Sep 3, 2021 at 1:38 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting Doug Anderson (2021-09-01 16:10:15) > > > Hi, > > > > > > On Wed, Sep 1, 2021 at 2:12 PM Olof Johansson <olof@lixom.net> wrote: > > > > > > > > On Wed, Sep 1, 2021 at 1:20 PM Douglas Anderson <dianders@chromium.org> wrote: > > > > > > > > > > In the patch ("drm/panel-simple-edp: Split eDP panels out of > > > > > panel-simple") we split the PANEL_SIMPLE driver in 2. By default let's > > > > > give everyone who had the old driver enabled the new driver too. If > > > > > folks want to opt-out of one or the other they always can later. > > > > > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > > > > > Isn't this a case where the new option should just have had the old > > > > option as the default value to avoid this kind of churn and possibly > > > > broken platforms? > > > > > > I'm happy to go either way. I guess I didn't do that originally > > > because logically there's not any reason to link the two drivers going > > > forward. Said another way, someone enabling the "simple panel" driver > > > for non-eDP panels wouldn't expect that the "simple panel" driver for > > > DP panels would also get enabled by default. They really have nothing > > > to do with one another. Enabling by default for something like this > > > also seems like it would lead to bloat. I could have sworn that > > > periodically people get yelled at for marking drivers on by default > > > when it doesn't make sense. > > > > > > ...that being said, I'm happy to change the default as you suggest. > > > Just let me know. > > > > Having the default will help olddefconfig users seamlessly migrate to > > the new Kconfig. Sadly they don't notice that they should probably > > disable the previous Kconfig symbol, but oh well. At least with the > > default they don't go on a hunt/bisect to figure out that some Kconfig > > needed to be enabled now that they're using a new kernel version. > > > > Maybe the default should have a TODO comment next to it indicating we > > should remove the default in a year or two. > > OK, so I'm trying to figure out how to do this without just "kicking > the can" down the road. I guess your idea is that for the next year > this will be the default and that anyone who really wants > "CONFIG_DRM_PANEL_EDP" will "opt-in" to keep it by adding > "CONFIG_DRM_PANEL_EDP=y" to their config? ...and then after a year > passes we remove the default? ...but that won't work, will it? Since > "CONFIG_DRM_PANEL_EDP" will be the default for the next year then you > really can't add it to the "defconfig", at least if you ever > "normalize" it. The "defconfig" by definition has everything stripped > from it that's already the "default", so for the next year anyone who > tries to opt-in will get their preference stripped. > > Hrm, so let me explain options as I see them. Maybe someone can point > out something that I missed. I'll assume that we'll change the config > option from CONFIG_DRM_PANEL_SIMPLE_EDP to CONFIG_DRM_PANEL_EDP > (remove the "SIMPLE" part). > > == > > Where we were before my series: > > * One config "CONFIG_DRM_PANEL_SIMPLE" and it enables simple non-eDP > and eDP drivers. > > == > > Option 1: update everyone's configs (this patch) > > * Keep old config "CONFIG_DRM_PANEL_SIMPLE" but it now only means > enable the panel-simple (non-eDP) driver. > * Anyone who wants eDP panels must opt-in to "CONFIG_DRM_PANEL_EDP" > * Update all configs in mainline; any out-of mainline configs must > figure this out themselves. > > Pros: > * no long term baggage > > Cons: > * patch upstream is a bit of "churn" > * anyone with downstream config will have to figure out what happened. > > == > > Option 2: kick the can down the road + accept cruft > > * Keep old config "CONFIG_DRM_PANEL_SIMPLE" and it means enable the > panel-simple (non-eDP) driver. > * Anyone with "CONFIG_DRM_PANEL_SIMPLE" is opted in by default to > "CONFIG_DRM_PANEL_EDP" > > AKA: > config DRM_PANEL_EDP > default DRM_PANEL_SIMPLE > > Pros: > * no config patches needed upstream at all and everything just works! > > Cons: > * people are opted in to extra cruft by default and need to know to turn it off. > * unclear if we can change the default without the same problems. > > == > > Option 3: try to be clever > > * Add _two_ new configs. CONFIG_DRM_PANEL_SIMPLE_V2 and CONFIG_DRM_PANEL_EDP. > * Old config "CONFIG_DRM_PANEL_SIMPLE" gets marked as "deprecated". > * Both new configs have "default CONFIG_DRM_PANEL_SIMPLE" > > Now anyone old will magically get both the new config options by > default. Anyone looking at this in the future _won't_ set the > deprecated CONFIG_DRM_PANEL_SIMPLE but will instead choose if they > want either the eDP or "simple" driver. > > Pros: > * No long term baggage. > * Everyone is transitioned automatically by default with no cruft patches. > > Cons: > * I can't think of a better name than "CONFIG_DRM_PANEL_SIMPLE_V2" and > that name is ugly. > > == > > Option 4: shave a yak > > When thinking about this I came up with a clever idea of stashing the > kernel version in a defconfig when it's generated. Then you could do > something like: > > config DRM_PANEL_EDP > default DRM_PANEL_SIMPLE if DEFCONFIG_GENERATED_AT <= 0x00050f00 > > That feels like a good idea to me but who knows what others would > think. In general I think this series already shaves enough yaks. This > isn't a new problem we're trying to solve so it seems like we should > pick one of the options above. > > == > > Unless I get an explicit NAK from someone like Olof or Arnd or I hear > that everyone loves Option #3 I'll probably just stick with the > existing approach since: > > * Olof's wording didn't make it sound like a strong objection. Yeah, not a strong objection but an enquiry if there's a better way to handle it. TL;DR: I don't think there really is. My comment mostly came from the fact that when olddefconfig gets broken like this, we tend to have a bunch of patches trickle in over time as downstream users discover the need to turn on the new option. You covered (most) of that by doing the appropriate defconfigs to this patch series, so it won't be as bad (besides any newly added defconfigs during the same release, and we're quite careful about doing that these days). I think most of the other options, besides 2, are just more overhead than needed here. So I'd be fine with just picking up option 1. What's clear is that this is not a very convenient activity that scales, but we don't do it all that often. This is where something like a "HAVE_EDP" type config that the platform can provide helps, but adding it just for this rework seems to be more work than it's worth. > * From git history it looks as if config patches don't necessarily > land through the SoC tree and thus I'd by default follow the > suggestions of the DRM folks. Andrzej suggested going with the > existing approach as long as I changed the symbol names and re-ordered > the patches. Right, Kconfig changes usually go with the driver. dts and defconfig changes go to the SoC tree though since otherwise we end up with a bunch of churn and conflicts. > Please yell if anything above sounds wrong! I'll probably try to send > out a new version tomorrow or the next day, but I won't land it right > away to give people time to yell. I'd leave it up to you if you want to do option 1 or 2, since there's no really convenient way to do it better. 3 seems to be a bigger hammer than what this situation calls for IMHO. -Olof
Hi, On Mon, Sep 6, 2021 at 3:05 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > > +{ > > + struct edid *edid; > > + u32 val; > > + > > + edid = drm_do_get_edid_blk0(drm_do_probe_ddc_edid, adapter, NULL, NULL); > > + > > + /* > > + * There are no manufacturer IDs of 0, so if there is a problem reading > > + * the EDID then we'll just return 0. > > + */ > > + if (IS_ERR_OR_NULL(edid)) > > + return 0; > > + > > + /* > > + * In theory we could try to de-obfuscate this like edid_get_quirks() > > + * does, but it's easier to just deal with a 32-bit number. > > Hmm, but is it, really? AFAICT this is just an internal representation > for a table, where it could just as well be stored in a struct that > could be just as compact now, but extensible later. You populate the > table via an encoding macro, then decode the id using a function - while > it could be in a format that's directly usable without the decode. If > suitably chosen, the struct could perhaps be reused between the quirks > code and your code. I'm not 100% sure, but I think you're suggesting having this function return a `struct edid_panel_id` or something like that. Is that right? Maybe that would look something like this? struct edid_panel_id { char vendor[4]; u16 product_id; } ...or perhaps this (untested, but I think it works): struct edid_panel_id { u16 vend_c1:5; u16 vend_c2:5; u16 vend_c3:5; u16 product_id; } ...and then change `struct edid_quirk` to something like this: static const struct edid_quirk { struct edid_panel_id panel_id; u32 quirks; } ... Is that correct? There are a few downsides that I can see: a) I think the biggest downside is the inability compare with "==". I don't believe it's legal to compare structs with "==" in C. Yeah, we can use memcmp() but that feels more awkward to me. b) Unless you use the bitfield approach, it takes up more space. I know it's not a huge deal, but the format in the EDID is pretty much _forced_ to fit in 32-bits. The bitfield approach seems like it'd be more awkward than my encoding macros. -Doug
Hi, On Sun, Sep 5, 2021 at 11:55 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Douglas, > > On Wed, Sep 01, 2021 at 01:19:18PM -0700, Douglas Anderson wrote: > > The goal of this patch series is to move away from hardcoding exact > > eDP panels in device tree files. As discussed in the various patches > > in this series (I'm not repeating everything here), most eDP panels > > are 99% probable and we can get that last 1% by allowing two "power > > up" delays to be specified in the device tree file and then using the > > panel ID (found in the EDID) to look up additional power sequencing > > delays for the panel. > > > > This patch series is the logical contiunation of a previous patch > > series where I proposed solving this problem by adding a > > board-specific compatible string [1]. In the discussion that followed > > it sounded like people were open to something like the solution > > proposed in this new series. > > > > In version 2 I got rid of the idea that we could have a "fallback" > > compatible string that we'd use if we didn't recognize the ID in the > > EDID. This simplifies the bindings a lot and the implementation > > somewhat. As a result of not having a "fallback", though, I'm not > > confident in transitioning any existing boards over to this since > > we'll have to fallback to very conservative timings if we don't > > recognize the ID from the EDID and I can't guarantee that I've seen > > every panel that might have shipped on an existing product. The plan > > is to use "edp-panel" only on new boards or new revisions of old > > boards where we can guarantee that every EDID that ships out of the > > factory has an ID in the table. > > > > Version 3 of this series now splits out all eDP panels to their own > > driver and adds the generic eDP panel support to this new driver. I > > believe this is what Sam was looking for [2]. > > > > [1] https://lore.kernel.org/r/YFKQaXOmOwYyeqvM@google.com/ > > [2] https://lore.kernel.org/r/YRTsFNTn%2FT8fLxyB@ravnborg.org/ > > > > Changes in v3: > > - Decode hex product ID w/ same endianness as everyone else. > > - ("Reorder logicpd_type_28...") patch new for v3. > > - Split eDP panels patch new for v3. > > - Move wayward panels patch new for v3. > > - ("Non-eDP panels don't need "HPD" handling") new for v3. > > - Split the delay structure out patch just on eDP now. > > - ("Better describe eDP panel delays") new for v3. > > - Fix "prepare_to_enable" patch new for v3. > > - ("Don't re-read the EDID every time") moved to eDP only patch. > > - Generic "edp-panel" handled by the eDP panel driver now. > > - Change init order to we power at the end. > > - Adjust endianness of product ID. > > - Fallback to conservative delays if panel not recognized. > > - Add Sharp LQ116M1JW10 to table. > > - Add AUO B116XAN06.1 to table. > > - Rename delays more generically so they can be reused. > > > > Changes in v2: > > - No longer allow fallback to panel-simple. > > - Add "-ms" suffix to delays. > > - Don't support a "fallback" panel. Probed panels must be probed. > > - Not based on patch to copy "desc"--just allocate for probed panels. > > - Add "-ms" suffix to delays. > > > > Douglas Anderson (16): > > dt-bindings: drm/panel-simple-edp: Introduce generic eDP panels > > drm/edid: Break out reading block 0 of the EDID > > drm/edid: Allow the querying/working with the panel ID from the EDID > > drm/panel-simple: Reorder logicpd_type_28 / mitsubishi_aa070mc01 > > drm/panel-simple-edp: Split eDP panels out of panel-simple > > ARM: configs: Everyone who had PANEL_SIMPLE now gets PANEL_SIMPLE_EDP > > arm64: defconfig: Everyone who had PANEL_SIMPLE now gets > > PANEL_SIMPLE_EDP > > MIPS: configs: Everyone who had PANEL_SIMPLE now gets PANEL_SIMPLE_EDP > > drm/panel-simple-edp: Move some wayward panels to the eDP driver > > drm/panel-simple: Non-eDP panels don't need "HPD" handling > > drm/panel-simple-edp: Split the delay structure out > > drm/panel-simple-edp: Better describe eDP panel delays > > drm/panel-simple-edp: hpd_reliable shouldn't be subtraced from > > hpd_absent > > drm/panel-simple-edp: Fix "prepare_to_enable" if panel doesn't handle > > HPD > > drm/panel-simple-edp: Don't re-read the EDID every time we power off > > the panel > > drm/panel-simple-edp: Implement generic "edp-panel"s probed by EDID > > Thanks for looking into this. I really like the outcome. > We have panel-simple that now (mostly) handle simple panels, > and thus all the eDP functionality is in a separate driver. > > I have provided a few nits. > My only take on this is the naming - as we do not want to confuse > panel-simple and panel-edp I strongly suggest renaming the driver to > panel-edp. Sure, I'll do that. I was trying to express the fact that the new "panel-edp" driver won't actually handle _all_ eDP panels, only the eDP panels that are (comparatively) simpler. For instance, I'm not planning to handle panel-samsung-atna33xc20.c in "panel-edp". I guess people will figure it out, though. > And then rename the corresponding Kconfig entry. > > With these few changes all patches are: > Acked-by: Sam Ravnborg <sam@ravnborg.org> Thanks, I'll add it to the patches. If there's anything major I need to change I'll give you a yell to make sure you see it. > For bisectability I suggest to move the defconfig patches up before you > introduce the new Kconfig symbol. Or maybe they will be added via > another tree and then this is not possible to control Yup, I'll do that. There was some question about the defconfig patch but they are hopefully cleared up now. > I assume you will apply the patches yourself. Sure, I can do that with your Ack. I'll also make sure that patches that Jani commented on get resolved. -Doug
Hi, On Sun, Sep 5, 2021 at 11:42 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > > +++ b/drivers/gpu/drm/panel/panel-simple-edp.c > > @@ -0,0 +1,1298 @@ > > +/* > > + * Copyright (C) 2013, NVIDIA Corporation. All rights reserved. > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a > > + * copy of this software and associated documentation files (the "Software"), > > + * to deal in the Software without restriction, including without limitation > > + * the rights to use, copy, modify, merge, publish, distribute, sub license, > > + * and/or sell copies of the Software, and to permit persons to whom the > > + * Software is furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice (including the > > + * next paragraph) shall be included in all copies or substantial portions > > + * of the Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > > + * DEALINGS IN THE SOFTWARE. > > + */ > Would be nice if you could use the SPDX thingy for the license. I'm going to leave this alone. I definitely started this driver by copy-pasting the panel-simple.c file and it still shares a lot of lines of code with that driver. It feels like that qualifies for the "substantial portions of the Software" portion which tells me to retain the license. I also kept Thierry as the author since, again, it's really a splitting of the existing driver and not the creation of a new driver. In fact, if I were to assign a new author/license to panel-edp, one could also make the argument that I should assign a new author/license to panel-simple. panel-simple got ~50% of the old panels and panel-edp got the other ~50% of the old panels plus a search-and-replace of "simple" for "edp" and some code deletion. I don't think search-and-replace name change nor code deletion is justification for claiming authorship. ;-) If Thierry wants to chime in and say that I should put down a different license for either of the two files, though, I'd be glad to change it. -Doug
Hi, On Wed, Sep 1, 2021 at 1:20 PM Douglas Anderson <dianders@chromium.org> wrote: > > The "logicpd_type_28" panel data was splitting up the > mitsubishi_aa070mc01 panel data. Reorganize it so that the panel descs > and modes are kept together. > > This is a no-op code-cleanup change, found by code inspection. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > Changes in v3: > - ("Reorder logicpd_type_28...") patch new for v3. > > drivers/gpu/drm/panel/panel-simple.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) I've pushed just this one patch (with Sam's Ack from the cover letter) just to simplify future posts. It's pretty much a no-brainer patch and there are no dependencies anywhere for it. c8527b9ad3cf (drm-misc/drm-misc-next) drm/panel-simple: Reorder logicpd_type_28 / mitsubishi_aa070mc01 -Doug
On Wed, 08 Sep 2021, Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Mon, Sep 6, 2021 at 3:05 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: >> >> > +{ >> > + struct edid *edid; >> > + u32 val; >> > + >> > + edid = drm_do_get_edid_blk0(drm_do_probe_ddc_edid, adapter, NULL, NULL); >> > + >> > + /* >> > + * There are no manufacturer IDs of 0, so if there is a problem reading >> > + * the EDID then we'll just return 0. >> > + */ >> > + if (IS_ERR_OR_NULL(edid)) >> > + return 0; >> > + >> > + /* >> > + * In theory we could try to de-obfuscate this like edid_get_quirks() >> > + * does, but it's easier to just deal with a 32-bit number. >> >> Hmm, but is it, really? AFAICT this is just an internal representation >> for a table, where it could just as well be stored in a struct that >> could be just as compact now, but extensible later. You populate the >> table via an encoding macro, then decode the id using a function - while >> it could be in a format that's directly usable without the decode. If >> suitably chosen, the struct could perhaps be reused between the quirks >> code and your code. > > I'm not 100% sure, but I think you're suggesting having this function > return a `struct edid_panel_id` or something like that. Is that right? > Maybe that would look something like this? > > struct edid_panel_id { > char vendor[4]; > u16 product_id; > } > > ...or perhaps this (untested, but I think it works): > > struct edid_panel_id { > u16 vend_c1:5; > u16 vend_c2:5; > u16 vend_c3:5; > u16 product_id; > } > > ...and then change `struct edid_quirk` to something like this: > > static const struct edid_quirk { > struct edid_panel_id panel_id; > u32 quirks; > } ... > > Is that correct? There are a few downsides that I can see: > > a) I think the biggest downside is the inability compare with "==". I > don't believe it's legal to compare structs with "==" in C. Yeah, we > can use memcmp() but that feels more awkward to me. > > b) Unless you use the bitfield approach, it takes up more space. I > know it's not a huge deal, but the format in the EDID is pretty much > _forced_ to fit in 32-bits. The bitfield approach seems like it'd be > more awkward than my encoding macros. Sorry for the delayed response. Fair enough, let's go with the u32 for now. It's not like we can't change this later. BR, Jani.