On Mon, Feb 1, 2021 at 1:49 PM Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > On Mon, Feb 01, 2021 at 11:59:31AM +0100, Bartosz Golaszewski wrote: > > On Mon, Feb 1, 2021 at 11:28 AM Andy Shevchenko > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > > On Sat, Jan 30, 2021 at 09:37:55PM +0100, Bartosz Golaszewski wrote: > > > > On Fri, Jan 29, 2021 at 4:57 PM Andy Shevchenko > > > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > > > > On Fri, Jan 29, 2021 at 02:46:24PM +0100, Bartosz Golaszewski wrote: > > ... > > > > > > > +static int gpio_sim_set_config(struct gpio_chip *gc, > > > > > > + unsigned int offset, unsigned long config) > > > > > > +{ > > > > > > + struct gpio_sim_chip *chip = gpiochip_get_data(gc); > > > > > > + > > > > > > + switch (pinconf_to_config_param(config)) { > > > > > > > > > > > + case PIN_CONFIG_BIAS_PULL_UP: > > > > > > + return gpio_sim_apply_pull(chip, offset, 1); > > > > > > + case PIN_CONFIG_BIAS_PULL_DOWN: > > > > > > + return gpio_sim_apply_pull(chip, offset, 0); > > > > > > > > > > But aren't we got a parameter (1 or 0) from config? And hence > > > > > > > > > > case PIN_CONFIG_BIAS_PULL_UP: > > > > > case PIN_CONFIG_BIAS_PULL_DOWN: > > > > > return gpio_sim_apply_pull(chip, offset, <param>); > > > > > > > > > > ? > > > > > > > > I believe this is more explicit and so easier to read if you don't > > > > know the GPIO and pinctrl internals. > > > > > > If we ever go to change meanings of the values in param, it will require to fix > > > this occurrence which seems to me suboptimal. > > > > > > > Why would we do it? This is internal to this driver. > > > > > > > > + default: > > > > > > + break; > > > > > > + } > > > > > > + > > > > > > + return -ENOTSUPP; > > > > > > +} > > Up to you. > My personal vote for using the embedded param, because it makes code consistent > and if anybody takes this driver as an example for something, it will be better > example in such case.. > > ... > > > > > > > +static ssize_t gpio_sim_sysfs_line_store(struct device *dev, > > > > > > + struct device_attribute *attr, > > > > > > + const char *buf, size_t len) > > > > > > +{ > > > > > > + struct gpio_sim_attribute *line_attr = to_gpio_sim_attr(attr); > > > > > > + struct gpio_sim_chip *chip = dev_get_drvdata(dev); > > > > > > + int ret, val; > > > > > > > > > > > + ret = kstrtoint(buf, 0, &val); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + if (val != 0 && val != 1) > > > > > > + return -EINVAL; > > > > > > > > > > kstrtobool() ? > > > > > > > > > > > > > No, we really only want 0 or 1, no yes, Y etc. > > > > > > Side note: But you allow 0x00001, for example... > > > > Good point. In that case we should check if len > 2 and if buf[0] == > > '1' or '0' and that's all we allow. > > Up to you also. I don't like such a strictness. OTOH we can relax afterwards if > needed. > > > > Then why not to use unsigned type from the first place and add a comment? > > > > > > > > > + ret = gpio_sim_apply_pull(chip, line_attr->offset, val); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + return len; > > > > > > +} > > ... > > > > > > > +struct gpio_sim_chip_config { > > > > > > + struct config_item item; > > > > > > + > > > > > > + /* > > > > > > + * If pdev is NULL, the item is 'pending' (waiting for configuration). > > > > > > + * Once the pointer is assigned, the device has been created and the > > > > > > + * item is 'live'. > > > > > > + */ > > > > > > + struct platform_device *pdev; > > > > > > > > > > Are you sure > > > > > > > > > > struct device *dev; > > > > > > > > > > is not sufficient? > > > > > > > > > > > > > It may be but I really prefer those simulated devices to be on the platform bus. > > > > > > My point here is that there is no need to keep specific bus devices type, > > > because you may easily derive it from the struct device pointer. Basically if > > > you are almost using struct device in your code (seems to me the case), you > > > won't need to carry bus specific one and dereference it each time. > > > > But don't we need a bus to even register a device? I haven't checked > > in a long time but IIRC it's mandatory. > > > > Let me give you a different argument - the platform device offers a > > very simple API for registering devices with properties being > > duplicated behind the scenes etc. It seems to me that registering a > > bare struct device * would take more boiler-plate code for not much > > gain. > > Yes, I'm not objecting the platform bus choice. I'm objecting the keeping of > the pointer to the bus specific structure. > > There are helpers like to_platform_device() which make the bus specific > pointers go away from the structures and easier code when you use exactly > pointer to struct device rather than bus specific one. > Ok I get it. We almost never dereference it though. We do it in probe, but there's no way around it. In sysfs callbacks we already get a pointer to struct device. And when unregistering the platform device, we need to pass it as struct platform_device anyway. I don't see any gain from that and would prefer to keep it as is. Bart