On 03/03/2016 03:18 PM, Philipp Zabel wrote:
> Hi Neil,
>
>> +config RESET_OXNAS
>> + bool
>> + select MFD_SYSCON
>
> I'd prefer not to select MFD_SYSCON here, but rather let ARCH_OXNAS do
> that.
>
OK.
>> +#include <linux/io.h>
>
> Is there any need to include linux/io.h ?
No, dropping.
>> +static int oxnas_reset_reset(struct reset_controller_dev *rcdev,
>> + unsigned long id)
>> +{
>> + struct oxnas_reset *data =
>> + container_of(rcdev, struct oxnas_reset, rcdev);
>> +
>> + regmap_write(data->regmap, RST_SET_REGOFFSET, BIT(id));
>> + msleep(50);
>
> Is this the right delay for all of the resets in this register?
> If not, I'd drop the .reset callback.
>
The delay is not strictly necessary, but better to avoid any HW issues.
And the .reset callback is needed since reset_control_reset
does not assert -> deassert as fallback.
>> + regmap_write(data->regmap, RST_CLR_REGOFFSET, BIT(id));
>> +
>> + return 0;
>> +}
>> +
>> +static int oxnas_reset_assert(struct reset_controller_dev *rcdev,
>> + unsigned long id)
>> +{
>> + struct oxnas_reset *data =
>> + container_of(rcdev, struct oxnas_reset, rcdev);
>> +
>> + regmap_write(data->regmap, RST_SET_REGOFFSET, BIT(id));
>> +
>> + return 0;
>> +}
>> +
>> +static int oxnas_reset_deassert(struct reset_controller_dev *rcdev,
>> + unsigned long id)
>> +{
>> + struct oxnas_reset *data =
>> + container_of(rcdev, struct oxnas_reset, rcdev);
>> +
>> + regmap_write(data->regmap, RST_CLR_REGOFFSET, BIT(id));
>> +
>> + return 0;
>> +}
>> +
>> +static struct reset_control_ops oxnas_reset_ops = {
>
> const
>
Something checkpatch should report...
>> + .reset = oxnas_reset_reset,
>> + .assert = oxnas_reset_assert,
>> + .deassert = oxnas_reset_deassert,
>> +};
>> +
>> +static const struct of_device_id oxnas_reset_dt_ids[] = {
>> + { .compatible = "plxtech,nas782x-reset", },
>> + { /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, oxnas_reset_dt_ids);
>> +
>> +static int oxnas_reset_probe(struct platform_device *pdev)
>> +{
>> + struct oxnas_reset *data;
>> + struct device *parent;
>> +
>> + parent = pdev->dev.parent;
>> + if (!parent) {
>> + dev_err(&pdev->dev, "no parent\n");
>
> Can this even happen?
>
It's to make sure parent->of_node is valid for syscon_node_to_regmap.
>> + return -ENODEV;
>> + }
>> +
>> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + data->regmap = syscon_node_to_regmap(parent->of_node);
>> + if (IS_ERR(data->regmap)) {
>> + dev_err(&pdev->dev, "failed to get parent regmap\n");
>> + return -ENODEV;
>
> Better print the error code and return it.
>
Good point.
>> + }
>> +
>> + data->rcdev.owner = THIS_MODULE;
>> + data->rcdev.nr_resets = 32;
>> + data->rcdev.ops = &oxnas_reset_ops;
>> + data->rcdev.of_node = pdev->dev.of_node;
>> + reset_controller_register(&data->rcdev);
>
> Move this down a bit:
>
>> +
>> + platform_set_drvdata(pdev, data);
>> +
>> + return 0;
>
> and
> return reset_controller_register(&data->rcdev);
> here.
>
Yes, sound better...
>> +static struct platform_driver oxnas_reset_driver = {
>> + .probe = oxnas_reset_probe,
>> + .remove = oxnas_reset_remove,
>> + .driver = {
>> + .name = "oxnas-reset",
>> + .owner = THIS_MODULE,
>
> The .owner field is overwritten by __platform_driver_register() anyway,
> just drop it.
OK
>> +/*
>> + * Reset controller does not support probe deferral, so it has to be
>> + * initialized before any user, in particular, PCIE uses subsys_initcall.
>> + */
>> +arch_initcall(oxnas_reset_init);
>
> That doesn't sound right. (of_)reset_control_get return -EPROBE_DEFER if
> the rcdev isn't found in the list. Could you elaborate on this?
It was an old change, I will put back the generic module platform init.
>
> regards
> Philipp
>
Thanks,
Neil
