Hi,
Thanks for review.
On 2016/2/22 18:23, Philipp Zabel wrote:
> Hi Chen,
>
> Am Montag, den 22.02.2016, 10:45 +0800 schrieb Chen Feng:
>> Signed-off-by: Chen Feng <puck.c...@hisilicon.com>
>> Signed-off-by: Xinliang Liu <xinliang....@linaro.org>
>> ---
>> drivers/reset/hisilicon/hi6220_reset.c | 122
>> ++++++++++++++++++++++++---------
>> 1 file changed, 90 insertions(+), 32 deletions(-)
>> +/* peritheral ctrl regs
>
> I assume this is supposed to mean "peripheral" ...
>
>> + */
>> +#define PERITH_ASSERT_OFFSET 0x300
>
> ... and this PERIPH_ASSERT_OFFSET, and so on.
>
Yes...
>> +#define PERITH_DEASSERT_OFFSET 0x304
>> +#define PERITH_MAX_INDEX 0x509
>> +}
>
> If you make these take the struct reset_controller_dev *rc_dev as first
> parameter, you can drop the two functions below an just add a second set
> of const struct reset_control_ops for the media resets.
>
okay.
>> static int hi6220_reset_assert(struct reset_controller_dev *rc_dev,
>> unsigned long idx)
>> {
>> struct hi6220_reset_data *data = to_reset_data(rc_dev);
>> + struct regmap *regmap = data->regmap;
>>
>> data->rc_dev.ops = &hi6220_reset_ops;
>
> With media and periph resets in separate ops structures, you could chose
> here already:
>
> if (type == MEDIA)
> data->rc_dev.ops = &hi6220_media_reset_ops;
> else
> data->rc_dev.ops = &hi6220_peripheral_reset_ops;
>
I will add two ops next version.
>> - data->rc_dev.of_node = pdev->dev.of_node;
>> + data->rc_dev.of_node = np;
>> + if (type == MEDIA)
>> + data->rc_dev.nr_resets = MEDIA_MAX_INDEX;
>> + else
>> + data->rc_dev.nr_resets = PERITH_MAX_INDEX;
>>
>> reset_controller_register(&data->rc_dev);
>>
>> @@ -89,9 +139,17 @@ static int hi6220_reset_probe(struct platform_device
>> *pdev)
>> }
>>
>> static const struct of_device_id hi6220_reset_match[] = {
>> - { .compatible = "hisilicon,hi6220-sysctrl" },
>> - { },
>> + {
>> + .compatible = "hisilicon,hi6220-sysctrl",
>> + .data = (void *)PERITHERAL,
>> + },
>> + {
>> + .compatible = "hisilicon,hi6220-mediactrl",
>> + .data = (void *)MEDIA,
>> + },
>> + { /* sentinel */ },
>> };
>> +MODULE_DEVICE_TABLE(of, hi6220_reset_match);
>>
>> static struct platform_driver hi6220_reset_driver = {
>> .probe = hi6220_reset_probe,
>
> regards
> Philipp
>
>
> .
>
