>
> > +
> > + if (read_buf & AW8695_SYSINT_DONEI)
> > + dev_dbg(dev, "Received playback done interrupt\n");
> > + /* FIFO mode is not (yet) implemented in this driver */
> > + if (read_buf & AW8695_SYSINT_FF_AEI)
> > + dev_dbg(dev, "Received FIFO almost empty interrupt\n");
> > + if (read_buf & AW8695_SYSINT_FF_AFI)
> > + dev_dbg(dev, "Received FIFO almost full interrupt\n");
> > +
> > + err = regmap_read(haptics->regmap, AW8695_DBGSTAT, &read_buf);
> > + if (err) {
> > + dev_err(dev, "Failed to read DBGSTAT register: %d\n", err);
> > + return err;
>
> Here you are returning an int, but the function is of type irqreturn_t.
> Since the device appears to implement clear-on-read interrupt registers,
> consider returning IRQ_NONE in this case (i.e. interrupt was not handled).
Makes sense, will update the error returns in this function.
>
> > + }
> > + dev_dbg(dev, "Interrupt: DBGSTAT=0x%x\n", read_buf);
> > +
> > + err = regmap_read(haptics->regmap, AW8695_SYSST, &read_buf);
> > + if (err) {
> > + dev_err(dev, "Failed to read SYSST register: %d\n", err);
> > + return err;
> > + }
> > + dev_dbg(dev, "Interrupt: SYSST=0x%x\n", read_buf);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static const struct regmap_config aw8695_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > +
> > + .max_register = AW8695_MAX_REG,
> > + .cache_type = REGCACHE_NONE,
> > +};
> > +
> > +static int aw8695_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct device *dev = &client->dev;
> > + struct aw8695_data *haptics;
> > + int err;
> > +
> > + haptics = devm_kzalloc(dev, sizeof(*haptics), GFP_KERNEL);
> > + if (!haptics)
> > + return -ENOMEM;
> > +
> > + err = of_property_read_u32(dev->of_node, "awinic,f0-preset", &haptics->f0_preset);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to read awinic,f0-preset\n");
>
> I don't think dev_err_probe() makes sense here; none of these functions
> should be returning -EPROBE_DEFER. In fact it seems you are using this
> simply for every function that may fail during probe.
The documentation for the function mentions it can also be used for
these cases:
* Note that it is deemed acceptable to use this function for error
* prints during probe even if the @err is known to never be -EPROBE_DEFER.
* The benefit compared to a normal dev_err() is the standardized format
* of the error code and the fact that the error code is returned.
Let me know.
>
> > +
> > + err = of_property_read_u32(dev->of_node, "awinic,f0-coefficient",
> > + &haptics->f0_coefficient);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to read awinic,f0-coefficient\n");
> > +
> > + err = of_property_read_u32(dev->of_node, "awinic,f0-calibration-percent",
> > + &haptics->f0_cali_percent);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to read awinic,f0-coefficient\n");
> > +
> > + err = of_property_read_u32(dev->of_node, "awinic,drive-level", &haptics->drive_level);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to read awinic,drive-level\n");
> > +
> > + err = of_property_read_u32(dev->of_node, "awinic,f0-detection-play-time",
> > + &haptics->f0_det_play);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to read awinic,f0-detection-play-time\n");
> > +
> > + err = of_property_read_u32(dev->of_node, "awinic,f0-detection-wait-time",
> > + &haptics->f0_det_wait);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to read awinic,f0-detection-wait-time\n");
> > +
> > + err = of_property_read_u32(dev->of_node, "awinic,f0-detection-repeat",
> > + &haptics->f0_det_repeat);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to read awinic,f0-detection-repeat\n");
> > +
> > + err = of_property_read_u32(dev->of_node, "awinic,f0-detection-trace",
> > + &haptics->f0_det_trace);
> > + if (err)
> > + dev_err_probe(dev, err, "Failed to read awinic,f0-detection-trace\n");
> > +
> > + err = of_property_read_u8_array(dev->of_node, "awinic,boost-debug",
> > + haptics->boost_debug, ARRAY_SIZE(haptics->boost_debug));
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to read awinic,boost-debug\n");
>
> Just to echo Krzysztof's comments from the binding: you should not expect
> platform engineers to populate raw register values directly. Instead, it
> is the responsibility of the driver to translate human-readable boolean or
> scalar properties into raw register values.
>
> Ideally one should be able to populate dts without having to refer to the
> datasheet in the first place.
As mentioned in the dt-bindings email, the datasheet doesn't mention
what these values are used for :(
>
> > +
> > + err = of_property_read_u8(dev->of_node, "awinic,tset", &haptics->tset);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to read awinic,tset\n");
>
> Are all of these properties truly required?
As I don't know what they do, I don't know whether they're required..
>
> > +
> > + err = of_property_read_u8(dev->of_node, "awinic,r-spare", &haptics->r_spare);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to read awinic,r-spare\n");
> > +
> > + err = of_property_read_u32(dev->of_node, "awinic,bemf-upper-threshold",
> > + &haptics->bemf_vthh);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to read awinic,bemf-upper-threshold\n");
> > +
> > + err = of_property_read_u32(dev->of_node, "awinic,bemf-lower-threshold",
> > + &haptics->bemf_vthl);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to read awinic,bemf-lower-threshold\n");
> > +
> > + haptics->input_dev = devm_input_allocate_device(dev);
> > + if (!haptics->input_dev)
> > + return -ENOMEM;
> > +
> > + haptics->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(haptics->reset_gpio))
> > + return dev_err_probe(dev, PTR_ERR(haptics->reset_gpio),
> > + "Failed to get reset gpio\n");
>
> Is a reset GPIO truly required, or can it simply be optional?
The datasheet doesn't explicly mention that it can be always connected
to high/low as I've seen in other datasheets so I think for now we can
assume it's always connected to a GPIO.
Of course if someone comes across hardware where there's no reset gpio
then the driver can be adjusted.
Regards
Luca
>
> > +
> > + err = devm_request_threaded_irq(dev, client->irq, NULL, aw8695_irq,
> > + IRQF_ONESHOT, NULL, haptics);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to request interrupt\n");
> > +
> > + INIT_WORK(&haptics->play_work, aw8695_haptics_play_work);
> > +
> > + haptics->input_dev->name = "aw8695-haptics";
> > + haptics->input_dev->close = aw8695_close;
> > +
> > + input_set_drvdata(haptics->input_dev, haptics);
> > + input_set_capability(haptics->input_dev, EV_FF, FF_RUMBLE);
> > +
> > + err = input_ff_create_memless(haptics->input_dev, NULL,
> > + aw8695_haptics_play);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to create FF dev\n");
> > +
> > + haptics->client = client;
> > + i2c_set_clientdata(client, haptics);
> > +
> > + haptics->regmap = devm_regmap_init_i2c(client, &aw8695_regmap_config);
> > + if (IS_ERR(haptics->regmap))
> > + return dev_err_probe(dev, PTR_ERR(haptics->regmap),
> > + "Failed to allocate register map\n");
> > +
> > + err = aw8695_init(haptics);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to init aw8695\n");
> > +
> > + err = aw8695_ram_init(haptics);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to init aw8695 sram\n");
> > +
> > + err = input_register_device(haptics->input_dev);
> > + if (err)
> > + return dev_err_probe(dev, err, "Failed to register input device\n");
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id aw8695_of_id[] = {
> > + { .compatible = "awinic,aw8695", },
> > + { /* sentinel */ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, aw8695_of_id);
> > +
> > +static struct i2c_driver aw8695_driver = {
> > + .driver = {
> > + .name = "aw8695-haptics",
> > + .of_match_table = aw8695_of_id,
> > + },
> > + .probe = aw8695_probe,
> > +};
> > +
> > +module_i2c_driver(aw8695_driver);
> > +
> > +MODULE_AUTHOR("Luca Weiss <luca.weiss@xxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("AW8695 LRA Haptic Driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.35.1
> >
>
> Kind regards,
> Jeff LaBundy
