| Message ID | 20240411090628.2436389-1-ckeepax@opensource.cirrus.com |
|---|---|
| Headers | show |
| Series | Add bridged amplifiers to cs42l43 | expand |
On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > SPI devices can specify a cs-gpios property to enumerate their > chip selects. Under device tree, a zero entry in this property can > be used to specify that a particular chip select is using the SPI > controllers native chip select, for example: > > cs-gpios = <&gpio1 0 0>, <0>; > > Here the second chip select is native. However, when using swnodes Here, the > there is currently no way to specify a native chip select. The > proposal here is to register a swnode_gpio_undefined software node, > that can be specified to allow the indication of a native chip > select. For example: > > static const struct software_node_ref_args device_cs_refs[] = { > { > .node = &device_gpiochip_swnode, > .nargs = 2, > .args = { 0, GPIO_ACTIVE_LOW }, > }, > { > .node = &swnode_gpio_undefined, > .nargs = 0, > }, > }; > > Register the swnode as the gpiolib is initialised and check in > swnode_get_gpio_device() if the returned node matches > swnode_gpio_undefined and return -ENOENT, which matches the > behaviour of the device tree system when it encounters a 0 phandle. ... > +config GPIO_SWNODE_UNDEFINED > + bool But why did you remove the help? Please, put it back. ... > + /* > + * Check for special node that identifies undefined GPIOs, this is for a special > + * primarily used as a key for internal chip selects in SPI bindings. > + */ > + if (!strcmp(gdev_node->name, GPIOLIB_SWNODE_UNDEFINED_NAME)) > + return ERR_PTR(-ENOENT); This is a dead code when the respective config option is not selected. Or actually a potential flaw if somebody else names their swnode like this. ... > + ret = software_node_register(&swnode_gpio_undefined); > + if (ret < 0) > + pr_err("gpiolib: failed to register swnode: %d\n", ret); Instead of this prefix, define pr_fmt() > + return ret; ... > --- a/include/linux/gpio/consumer.h > +++ b/include/linux/gpio/consumer.h Why? I already said that the best place is gpio/property.h as it's exactly for swnode related code.
On Thu, Apr 11, 2024 at 5:04 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax > <ckeepax@opensource.cirrus.com> wrote: ... > > + fwnode_for_each_child_node(fwnode, child_fwnode) { > > + acpi_handle handle = ACPI_HANDLE_FWNODE(child_fwnode); > > > + if (!handle) > > + continue; > > This is _almost_ redundant check. handle == NULL is for the global > root object which quite unlikely will have the _ADR method. The > specification is clear about this: "The _ADR object is valid only > within an Augmented Device Descriptor." That said, the check makes > sense against the (very) ill-formed DSDT. > > > + ret = acpi_get_local_address(handle, &function); > > + if (ret || function != func_smart_amp) > > + continue; > > + > > + ext_fwnode = fwnode_get_named_child_node(child_fwnode, > > + "mipi-sdca-function-expansion-subproperties"); > > + if (!ext_fwnode) > > + continue; > > + > > + ret = fwnode_property_read_u32(ext_fwnode, > > + "01fa-cirrus-sidecar-instances", > > + &val); > > + > > + fwnode_handle_put(ext_fwnode); > > + fwnode_handle_put(child_fwnode); And still this leftover... > > + if (!ret) > > + return !!val;
On Thu, Apr 11, 2024 at 04:25:25PM +0300, Andy Shevchenko wrote: > On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax > <ckeepax@opensource.cirrus.com> wrote: > > Here the second chip select is native. However, when using swnodes > > Here, the > Can do. > > +config GPIO_SWNODE_UNDEFINED > > + bool > > But why did you remove the help? Please, put it back. > Sorry didn't realise you still wanted it will pop it back. > ... > > > + /* > > + * Check for special node that identifies undefined GPIOs, this is > > for a special Can do. > > > + * primarily used as a key for internal chip selects in SPI bindings. > > + */ > > + if (!strcmp(gdev_node->name, GPIOLIB_SWNODE_UNDEFINED_NAME)) > > + return ERR_PTR(-ENOENT); > > This is a dead code when the respective config option is not selected. > Or actually a potential flaw if somebody else names their swnode like > this. > Can add a check for the config. > ... > > > + ret = software_node_register(&swnode_gpio_undefined); > > + if (ret < 0) > > + pr_err("gpiolib: failed to register swnode: %d\n", ret); > > Instead of this prefix, define pr_fmt() > Little iffy on this, there are other prints in gpiolib that do it this way as well, I guess I could add a patch to convert everything but its starting to get a little out of the thrust of what I am doing here. > > + return ret; > > ... > > > --- a/include/linux/gpio/consumer.h > > +++ b/include/linux/gpio/consumer.h > > Why? I already said that the best place is gpio/property.h as it's > exactly for swnode related code. > Oh I see that's what you meant by those comments, as per my email I wasn't exactly sure. Will move them I don't mind where they go. Thanks, Charles
On Thu, Apr 11, 2024 at 04:30:13PM +0300, Andy Shevchenko wrote: > On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax > <ckeepax@opensource.cirrus.com> wrote: > > > > Use the more modern is_acpi_device_node() rather than checking > > ACPI_COMPANION(). > > I don't think it's valuable on its own. There is no clear motivation > why to do that, I suggested it exactly in the conjunction of not > introducing two ways of fwnode type check. That said, you probably > want to elaborate the motivation in the commit message if you want to > keep it separate. > I am really tempted to just drop this, its not necessary for my changes and changes something that is unrelated to them. At the least it belongs in a separate patch. > ... > > > +#include <linux/fwnode.h> > > This header is not supposed to be included by the end users. property.h is. > Fair enough will update, although I really feel these headers could use some annotation if they are not supposed to be directly included. Either include everything you use or just include a top level header makes sense but this weird mixture we seem to use is very confusing and I don't have a big enough brain to remember every header. Thanks, Charles
On Thu, Apr 11, 2024 at 7:56 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > On Thu, Apr 11, 2024 at 04:30:13PM +0300, Andy Shevchenko wrote: > > On Thu, Apr 11, 2024 at 12:06 PM Charles Keepax > > <ckeepax@opensource.cirrus.com> wrote: > > > > > > Use the more modern is_acpi_device_node() rather than checking > > > ACPI_COMPANION(). > > > > I don't think it's valuable on its own. There is no clear motivation > > why to do that, I suggested it exactly in the conjunction of not > > introducing two ways of fwnode type check. That said, you probably > > want to elaborate the motivation in the commit message if you want to > > keep it separate. > > I am really tempted to just drop this, its not necessary for my > changes and changes something that is unrelated to them. At the > least it belongs in a separate patch. Okay, just elaborate in the commit message that this is done to make new comer not to invent its own way for fwnode type check, ... > > > +#include <linux/fwnode.h> > > > > This header is not supposed to be included by the end users. property.h is. > > Fair enough will update, although I really feel these headers > could use some annotation if they are not supposed to be directly > included. Either include everything you use or just include a top > level header makes sense but this weird mixture we seem to use is > very confusing and I don't have a big enough brain to remember > every header. Rough hint is to look into the contents. But I agree, that is a complete mess.