Re: [PATCH 1/2] reset: hisilicon: Add hi6220 media part of reset

[Chen Feng <puck.chen@hisilicon.com>]

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.chen@hisilicon.com>
>> Signed-off-by: Xinliang Liu <xinliang.liu@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
> 
> 
> .
>