| Message ID | 20180205125436.24005-1-thierry.reding@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: mediatek: Explicitly include pinctrl headers | expand |
On Mon, Feb 5, 2018 at 4:54 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > > Include these headers explicitly to avoid the build failure. I don't think you need to include *both*. <linux/device.h> used to include just <linux/pinctrl/devinfo.h>. I'll edit your patches to include just that. <linux/pinctrl/consumer.h> will come in automatically through it. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 05, 2018 at 09:42:21AM -0800, Linus Torvalds wrote: > On Mon, Feb 5, 2018 at 4:54 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > > > > Include these headers explicitly to avoid the build failure. > > I don't think you need to include *both*. > > <linux/device.h> used to include just <linux/pinctrl/devinfo.h>. > > I'll edit your patches to include just that. > <linux/pinctrl/consumer.h> will come in automatically through it. I was trying to avoid any implicit inclusion, but looking at pinctrl/devinfo.h it has a comment right above the pinctrl/consumer.h include that makes it clear that pinctrl/devinfo.h is the consumer of pinctrl for the core, so I guess the implicit include is fine here. I do question, though, if drivers have any business including this pinctrl/devinfo.h in the first place. For the Mediatek ethernet it seems like selecting the default state is redundant (the core should already have taken care of that, and the driver never selects a different state anywhere). The same is true of drm/rockchip, which also only seems to select a state which the pinctrl core should've selected by default already. See arch/arm/boot/dts/rk3288.dtsi which sets up the "lcdc" state as the only state for the LVDS output. Anyway, I think going with the pinctrl/devinfo.h include only is fine for now. If it turns out that the Mediatek ethernet and Rockchip LVDS drivers can just omit the bits fiddling with struct dev_pin_info, we can swap out the pinctrl/devinfo.h include for pinctrl/consumer.h at that time. LinusW: what are your thoughts on the struct dev_pin_info usage by these drivers? Does their code seem redundant to you, too? Thierry
On Mon, Feb 5, 2018 at 6:59 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > Anyway, I think going with the pinctrl/devinfo.h include only is fine > for now. If it turns out that the Mediatek ethernet and Rockchip LVDS > drivers can just omit the bits fiddling with struct dev_pin_info, we can > swap out the pinctrl/devinfo.h include for pinctrl/consumer.h at that > time. > > LinusW: what are your thoughts on the struct dev_pin_info usage by these > drivers? Does their code seem redundant to you, too? I don't think they should use struct dev_pin_info at all, that thing is for the device core only. I like to think that <linux/pinctrl/consumer.h> is for drivers that explicitly grab and control pin control states so this driver should only include <linux/pinctrl/consumer.h>. Torvalds: can you do it like that instead? Either way will make the compile work again, we can also tidy it up later. (i.e. I will grep for includes of pinctrl/dev_info.h and replace it with consumer.h) at some point. It wasn't pretty that these drivers were relying on implicit #includes in the first place so either is anyway prettier than what it used to look like. In a way I kind of like this sideffect even if it broke some compiles. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 05, 2018 at 08:02:40PM +0100, Linus Walleij wrote: > On Mon, Feb 5, 2018 at 6:59 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > > > Anyway, I think going with the pinctrl/devinfo.h include only is fine > > for now. If it turns out that the Mediatek ethernet and Rockchip LVDS > > drivers can just omit the bits fiddling with struct dev_pin_info, we can > > swap out the pinctrl/devinfo.h include for pinctrl/consumer.h at that > > time. > > > > LinusW: what are your thoughts on the struct dev_pin_info usage by these > > drivers? Does their code seem redundant to you, too? > > I don't think they should use struct dev_pin_info at all, > that thing is for the device core only. > > I like to think that <linux/pinctrl/consumer.h> is for drivers that > explicitly grab and control pin control states so this driver should > only include <linux/pinctrl/consumer.h>. > > Torvalds: can you do it like that instead? Either way will > make the compile work again, we can also tidy it up later. > (i.e. I will grep for includes of pinctrl/dev_info.h and replace > it with consumer.h) at some point. That won't work, unfortunately, because these drivers actually try to dereference pointers to struct dev_pin_info and hence need that include for the structure's definition. Those are the only two drivers I can see that access this structure directly (other than the device core). Thierry
On Mon, Feb 5, 2018 at 8:08 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Feb 05, 2018 at 08:02:40PM +0100, Linus Walleij wrote: >> On Mon, Feb 5, 2018 at 6:59 PM, Thierry Reding <thierry.reding@gmail.com> wrote: >> >> > Anyway, I think going with the pinctrl/devinfo.h include only is fine >> > for now. If it turns out that the Mediatek ethernet and Rockchip LVDS >> > drivers can just omit the bits fiddling with struct dev_pin_info, we can >> > swap out the pinctrl/devinfo.h include for pinctrl/consumer.h at that >> > time. >> > >> > LinusW: what are your thoughts on the struct dev_pin_info usage by these >> > drivers? Does their code seem redundant to you, too? >> >> I don't think they should use struct dev_pin_info at all, >> that thing is for the device core only. >> >> I like to think that <linux/pinctrl/consumer.h> is for drivers that >> explicitly grab and control pin control states so this driver should >> only include <linux/pinctrl/consumer.h>. >> >> Torvalds: can you do it like that instead? Either way will >> make the compile work again, we can also tidy it up later. >> (i.e. I will grep for includes of pinctrl/dev_info.h and replace >> it with consumer.h) at some point. > > That won't work, unfortunately, because these drivers actually try to > dereference pointers to struct dev_pin_info and hence need that include > for the structure's definition. Those are the only two drivers I can see > that access this structure directly (other than the device core). Grrr OK I thought I was aiming for encapsulation here but then what can I do, sigh. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" 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/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c index 29826dd15204..9e74c165f3ca 100644 --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c @@ -23,6 +23,8 @@ #include <linux/reset.h> #include <linux/tcp.h> #include <linux/interrupt.h> +#include <linux/pinctrl/consumer.h> +#include <linux/pinctrl/devinfo.h> #include "mtk_eth_soc.h"